Details

Reviewers

mconley
hjones

Group Reviewers

reusable-components-reviewers
Restricted Project

Commits

rMOZILLACENTRAL1d43359dfa1d: Bug 1896837 - Only sync import widget modules after DOMContentLoaded r=reusable…

Bugzilla Bug ID

1896837

Summary

Co-author/investigator: Tim Giles <[email protected]>

Delay sychronous loading of ESM based custom elements until the
DOMContentLoaded event. With D212190--which this patch depends on--the
components that have already been used on the page will be synchronously
loaded when customElements.setElementCreationCallback is registered.

Diff Detail

Repository

rMOZILLACENTRAL mozilla-central

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Build Status

Buildable 670626
Build 769711: arc lint + arc unit

Event Timeline

mstriemer created this revision.Thu, May 30, 8:57 PM

Herald added a project: secure-revision. · View Herald TranscriptThu, May 30, 8:57 PM

Harbormaster completed remote builds in B666561: Diff 869892.Thu, May 30, 8:57 PM

phab-bot published this revision for review.Thu, May 30, 8:57 PM

phab-bot changed the visibility from "Custom Policy" to "Public (No Login Required)".

phab-bot changed the edit policy from "Custom Policy" to "Restricted Project (Project)".

phab-bot removed a project: secure-revision.

Herald added a reviewer: Restricted Project. · View Herald TranscriptThu, May 30, 8:57 PM

Code analysis found 1 defect in diff 869892:

1 defect found by eslint (Mozlint)

IMPORTANT: Found 1 defect (error level) that must be fixed before landing.

You can run this analysis locally with:

./mach lint --warnings --outgoing

If you see a problem in this automated review, please report it here.

You can view these defects in the Diff Detail section of Phabricator diff 869892.

The compare link at https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=fc4791bc2c3d5223c8736eaed3da303e0c1720e7&newProject=try&newRevision=b14b2813aeef84b179e1c1308c54c00d0fd1744e&framework=1&page=1 (from the bug's https://bugzilla.mozilla.org/show_bug.cgi?id=1896837#c7 ) shows this as not having a clear benefit - possibly a regression. Is that the most up-to-date perf compare?

This is the last perfherder comparison for these two patches https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=3ae8110fbebc14284f62842548cd92089d9a2001&newProject=try&newRevision=46f166555d7da7a1a0a1d01a57d6836c50952525&framework=1&page=1

mstriemer updated this revision to Diff 870887.Mon, Jun 3, 2:35 PM

mstriemer edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B667304: Diff 870887.Mon, Jun 3, 2:35 PM

1 defect closed compared to the previous diff 869892.

If you see a problem in this automated review, please report it here.

1 defect closed compared to the previous diff 869892.

If you see a problem in this automated review, please report it here.

Thanks for investigating the performance issue! This may require some documentation updates somewhere... maybe in https://firefox-source-docs.mozilla.org/browser/components/storybook/docs/README.reusable-widgets.stories.html?

toolkit/content/customElements.js
817–819	I'm a little confused... what this comment recommends is to use `<script type="module">` if you have a component that you want to use on the startup path... but we're removing just such tags from preferences.xhtml. Is this meant to say something different?

This revision is now accepted and ready to land.Mon, Jun 3, 5:21 PM

Thanks for this! Ditto what @mconley said, we should update these docs, but fine to do that as a follow-up if that's easier

toolkit/content/customElements.js
808–812	Curious if we would see some perf improvements by waiting for `DOMContentLoaded` for all of these elements as well - was tested at some point in iterations of this change? If not it could make for an interesting follow up bug/investigation.

mstriemer updated this revision to Diff 871685.Tue, Jun 4, 7:30 PM

mstriemer marked 2 inline comments as done.

Harbormaster completed remote builds in B667894: Diff 871685.Tue, Jun 4, 7:30 PM

mstriemer added inline comments.Tue, Jun 4, 7:30 PM

toolkit/content/customElements.js
808–812	I was wondering the same thing. Could be interesting to investigate. I'd be somewhat curious if the immediate import is being depended on by code that runs before DOMContentLoaded in the browser chrome though. Could kick off a try build and see if it passes the tests I guess 🤔
817–819	This was written before the update to `setElementCreationCallback()` in the parent revision. Good catch, thanks