1343707 - datetimepopup.xml should not be initialized during browser startup

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

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.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

:smaug, :scottwu, you guys should know more if I'm right.

Comment 2

[Scott Wu [:scottwu]]

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?

Comment 3

[Scott Wu [:scottwu]]

Attachment: Bug 1343707 - Make datetimepopup.xml binding attach lazily.

Review commit: https://reviewboard.mozilla.org/r/116970/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/116970/

Comment 4

[Zibi Braniecki [:zbraniecki][:gandalf]]

:scottwu, is it ready for review?

Comment 5

[Scott Wu [:scottwu]]

I think this is the solution I can come up with now. Let me ask :mconley if he could take a look.

Comment 6

[Mike Conley (:mconley) (:⚙️) (PTO: May 24 - June 2nd)]

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.

Comment 7

[Scott Wu [:scottwu]]

Comment on attachment 8843192 [details]
Bug 1343707 - Make datetimepopup.xml binding attach lazily.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/116970/diff/1-2/

Comment 8

[Scott Wu [:scottwu]]

Thanks for the suggestions Mike! I've modified the commit message and changed the event name to "DateTimePickerBindingReady".

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0816e8fc07a
Make datetimepopup.xml binding attach lazily. r=mconley

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f0816e8fc07a