The default bug view has changed. See this FAQ.

datetimepopup.xml should not be initialized during browser startup

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM
RESOLVED FIXED
24 days ago
10 days 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

24 days 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

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

Comment 2

23 days 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

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

Comment 5

13 days 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

13 days ago
Attachment #8843192 - Flags: review?(mconley)
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

10 days 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

10 days 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

10 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0816e8fc07a
Status: NEW → RESOLVED
Last Resolved: 10 days 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.