datetimepopup.xml should not be initialized during browser startup

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: gandalf, Assigned: scottwu)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
While working on bug 1339892, I noticed that datetimepopup.xml is being executed during browser startup.

When I changed the API of mozIntl it shows an error about mozIntl API not working.

It seems to me that since there's no datetimepopup in the main UI, that's a missed opportunity to lazy initialize it on the first call, instead of at the browser startup.
(Reporter)

Comment 1

6 months ago
:smaug, :scottwu, you guys should know more if I'm right.
Flags: needinfo?(scwwu)
Flags: needinfo?(bugs)
(Assignee)

Comment 2

6 months ago
You are right that currently the datetimepopup binding is attached during browser startup. Thanks for bringing this to my attention because I didn't really think about the performance implication until now.

I could change it so that the binding is not attached until it is first used. Though I wonder if there's a standard approach?
Flags: needinfo?(scwwu)
Comment hidden (mozreview-request)
(Reporter)

Comment 4

5 months ago
:scottwu, is it ready for review?
Flags: needinfo?(scwwu)
(Assignee)

Comment 5

5 months ago
I think this is the solution I can come up with now. Let me ask :mconley if he could take a look.
Assignee: nobody → scwwu
Flags: needinfo?(scwwu)
(Assignee)

Updated

5 months ago
Attachment #8843192 - Flags: review?(mconley)

Comment 6

5 months ago
mozreview-review
Comment on attachment 8843192 [details]
Bug 1343707 - Make datetimepopup.xml binding attach lazily.

https://reviewboard.mozilla.org/r/116970/#review121528

This might even give us a little bit of a win on the tpaint and ts_paint talos tests. Thanks!

::: commit-message-e91de:1
(Diff revision 1)
> +Bug 1343707 - datetimepopup.xml binding is delayed until needed

We should probably make this commit message clearer. Specifically, we should say what we're changing, perhaps like:

"Bug 1343707 - Make datetimepopup.xml binding attach lazily. r?mconley"

::: toolkit/content/widgets/datetimepopup.xml:32
(Diff revision 1)
>          const mozIntl = Components.classes["@mozilla.org/mozintl;1"]
>                            .getService(Components.interfaces.mozIMozIntl);
>          mozIntl.addGetCalendarInfo(this.l10n);
>          mozIntl.addGetDisplayNames(this.l10n);
> +        // Notify DateTimePickerHelper.jsm that binding is done.
> +        this.dispatchEvent(new CustomEvent("DateTimePickerBindingDone"));

Nit: I wonder if "DateTimePickerBindingReady" or "DateTimePickerBindingAttached" is a more accurate event name. "Done" is a little ambiguous, imo.
Attachment #8843192 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 8

5 months ago
Thanks for the suggestions Mike! I've modified the commit message and changed the event name to "DateTimePickerBindingReady".
Flags: needinfo?(bugs)
Keywords: checkin-needed

Comment 9

5 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0816e8fc07a
Make datetimepopup.xml binding attach lazily. r=mconley
Keywords: checkin-needed

Comment 10

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0816e8fc07a
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.