1459245 - Migrate <stringbundle> from XBL binding to Custom Element to prove out the script loading plan for future Custom Elements

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task)

Description

[Brian Grinstead [:bgrins]]

Since the <findbar> conversion (Bug 1411707) is tied in with some Fluent work and is generally just a more complex binding, I'd like to prove out the plan in MainProcessSingleton (which was also discussed in Bug 1442006) with a trivial binding, like <stringbundle>.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: Bug 1459245 - Migrate <stringbundle> from a XBL binding to a Custom Element;

This loads a JS file to register the Custom Element whenever XUL documents
get created. This is a pattern we plan to follow for future elements as well -
stringbundle is just an easy target to prove it out with.

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

Comment 2

[Brian Grinstead [:bgrins]]

Requesting review now, but definitely don't want to land anything here until after the merge

Comment 3

[Brian Grinstead [:bgrins]]

Jorg, after this patch in order for stringbundle to work, stringbundle.js will need to be loaded in all XUL documents. Does mail already load toolkit/components/processsingleton/MainProcessSingleton.js? If so, I don't think there will need to be any changes in comm-central. If not, then the new lines from that file will have to be copied across to whatever the equivalent file is for mail.

Consumers of <stringbundle> won't need to change as long as the Custom Element gets registered properly.

The patch is still subject to change, but just wanted to give a heads up that this is likely to be the first XBL to Custom Element migration to land so hoping for a smooth transition.

Comment 4

[Brian Grinstead [:bgrins]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c72c0b8ff676d147b3b73f20d5b3fbee357bb9ee
Talos compare: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=423653c8894bd6d3f424b95f9e914a9c6d566d04&newProject=try&newRevision=c72c0b8ff676d147b3b73f20d5b3fbee357bb9ee&framework=1&showOnlyImportant=1

Comment 5

[Jorg K (CEST = GMT+2)]

Thanks, I'll look into it.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8973271 [details]
Bug 1459245 - Migrate <stringbundle> from a XBL binding to a Custom Element;

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

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 8973271 [details]
Bug 1459245 - Migrate <stringbundle> from a XBL binding to a Custom Element;

https://reviewboard.mozilla.org/r/241726/#review248016

Comment 8

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8e33cc69bd594235dc4dbe74ca55c08071f44f3

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/80fc39b7c5ec
Migrate <stringbundle> from a XBL binding to a Custom Element;r=mossop

Comment 10

[Jorg K (CEST = GMT+2)]

It looks like you forgot to cover the profile manager :-(
JavaScript error: chrome://mozapps/content/profile/profileSelection.js, line 41:
 TypeError: gProfileManagerBundle.getFormattedString is not a function

Comment 11

[Jorg K (CEST = GMT+2)]

I also see:
JavaScript error: chrome://global/content/elements/stringbundle.js, line 61: Not
SupportedError: Operation is not supported

That comes from
customElements.define("stringbundle", MozStringbundle);

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/80fc39b7c5ec

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+1) from comment #10)
> It looks like you forgot to cover the profile manager :-(
> JavaScript error: chrome://mozapps/content/profile/profileSelection.js, line
> 41:
>  TypeError: gProfileManagerBundle.getFormattedString is not a function

Hm, I guess we don't have any tests covering that :(. I'll take a look.

Comment 14

[Brian Grinstead [:bgrins]]

If I copy the contents of stringbundle.js into profileSelection.js then the CE does get registered. It looks like the profile picker is loaded before MainProcessSingleton is loaded. Mossop, is there a different place we could set up observer notifications that will detect them for the profile manager window? Alternatively, we could move the block

```
for (let script of [
  "chrome://global/content/elements/stringbundle.js",
]) {
  Services.scriptloader.loadSubScript(script, doc.ownerGlobal);
}
```

into a new JS file and then subscript load it from mainprocesssingleton and explicitly include it as a <script> at the top of the profile manager document(s).

Comment 15

[Brian Grinstead [:bgrins]]

Or a temporary fix would be to include:

  <script type="application/x-javascript"
          src="chrome://global/content/elements/stringbundle.js"/>

at the top of profileSelection.xul and createProfileWizard.xul.

Comment 16

[Jorg K (CEST = GMT+2)]

Please also fix the error from comment #11.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+1) from comment #16)
> Please also fix the error from comment #11.

When do you see that error, in the profile manager? At least in m-c we aren't even loading the script before opening the profile manager.

Comment 18

[ExE Boss]

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Or a temporary fix would be to include:
> 
>   <script type="application/x-javascript"
>           src="chrome://global/content/elements/stringbundle.js"/>
> 
> at the top of profileSelection.xul and createProfileWizard.xul.

Shouldn’t we be using the unprefixed `application/javascript` MIME type instead?

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to Brian Grinstead [:bgrins] from comment #17)
> When do you see that error, in the profile manager? At least in m-c we
> aren't even loading the script before opening the profile manager.
Sorry, afk right now. I saw it in the debug console when starting TB without the p/m. Don't you see it in FF? What does that line do?

Comment 20

[Dave Townsend [:mossop]]

(In reply to Brian Grinstead [:bgrins] from comment #14)
> If I copy the contents of stringbundle.js into profileSelection.js then the
> CE does get registered. It looks like the profile picker is loaded before
> MainProcessSingleton is loaded. Mossop, is there a different place we could
> set up observer notifications that will detect them for the profile manager
> window?

Not really no. We could hardcode loading that component into NS_InitXPCOM2 but that's probably more effort than it is worth. The profile manager (and related profile startup UI) are something of a special snowflake here, I can't think of anything else that shows UI before the app-startup notification so I suggest we handle it specially rather than trying to fix for the general case.

> Alternatively, we could move the block
> 
> ```
> for (let script of [
>   "chrome://global/content/elements/stringbundle.js",
> ]) {
>   Services.scriptloader.loadSubScript(script, doc.ownerGlobal);
> }
> ```
> 
> into a new JS file and then subscript load it from mainprocesssingleton and
> explicitly include it as a <script> at the top of the profile manager
> document(s).

I wonder if we'd be better preprocessing all the CE implementations into a single file so it is just one load, then we can just use a script tag in the profile manager UI.

Comment 21

[Brian Grinstead [:bgrins]]

(In reply to Dave Townsend [:mossop] from comment #20)
> (In reply to Brian Grinstead [:bgrins] from comment #14)
> > If I copy the contents of stringbundle.js into profileSelection.js then the
> > CE does get registered. It looks like the profile picker is loaded before
> > MainProcessSingleton is loaded. Mossop, is there a different place we could
> > set up observer notifications that will detect them for the profile manager
> > window?
> 
> Not really no. We could hardcode loading that component into NS_InitXPCOM2
> but that's probably more effort than it is worth. The profile manager (and
> related profile startup UI) are something of a special snowflake here, I
> can't think of anything else that shows UI before the app-startup
> notification so I suggest we handle it specially rather than trying to fix
> for the general case.

OK, I'll file a new bug and get a patch up.

> > Alternatively, we could move the block
> > 
> > ```
> > for (let script of [
> >   "chrome://global/content/elements/stringbundle.js",
> > ]) {
> >   Services.scriptloader.loadSubScript(script, doc.ownerGlobal);
> > }
> > ```
> > 
> > into a new JS file and then subscript load it from mainprocesssingleton and
> > explicitly include it as a <script> at the top of the profile manager
> > document(s).
> 
> I wonder if we'd be better preprocessing all the CE implementations into a
> single file so it is just one load, then we can just use a script tag in the
> profile manager UI.

My preference would be to not do this until we have to for perf reasons, since it makes it harder to debug (line numbers / file names don't map back to the source files).

Comment 22

[Jorg K (CEST = GMT+2)]

I see
  JavaScript error: chrome://global/content/elements/stringbundle.js, line 61: Not Supported
  Error: Operation is not supported
when starting TB with |-p Testing| where "Testing" is my testing profile.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+1) from comment #22)
> I see
>   JavaScript error: chrome://global/content/elements/stringbundle.js, line
> 61: Not Supported
>   Error: Operation is not supported
> when starting TB with |-p Testing| where "Testing" is my testing profile.

Hm, I don't see that on m-c (locally or on logs from bc tests in treeherder). Do stringbundles not work at all? Or, I wonder if this is happening only on a single window - could you `console.log(window.location.toString());` right before the call to customElements.define?

Comment 24

[Jorg K (CEST = GMT+2)]

Hmm, with this code:
dump(window.location.toString()+"================\n");
customElements.define("stringbundle", MozStringbundle);
dump("after CE ================\n");

I see:
chrome://messenger/content/newmailalert.xul================
after CE ================
chrome://messenger/content/newmailalert.xul================
JavaScript error: chrome://global/content/elements/stringbundle.js, line 62: Not
SupportedError: Operation is not supported

chrome://messenger/content/msgAccountCentral.xul================
after CE ================
chrome://messenger/content/msgAccountCentral.xul================
JavaScript error: chrome://global/content/elements/stringbundle.js, line 62: Not
SupportedError: Operation is not supported

chrome://messenger/content/messenger.xul================
after CE ================
chrome://messenger/content/messenger.xul================
JavaScript error: chrome://global/content/elements/stringbundle.js, line 62: Not
SupportedError: Operation is not supported

Almost looks like this is called twice and the second one fails.

Maybe we got this wrong:
https://hg.mozilla.org/comm-central/rev/b00e830b9e5e3a006102519c34b8fc8e862e9cdf

Comment 25

[Brian Grinstead [:bgrins]]

(In reply to Jorg K (GMT+1) from comment #24)
> Hmm, with this code:
> dump(window.location.toString()+"================\n");
> customElements.define("stringbundle", MozStringbundle);
> dump("after CE ================\n");
> 
> I see:
> chrome://messenger/content/newmailalert.xul================
> after CE ================
> chrome://messenger/content/newmailalert.xul================
> JavaScript error: chrome://global/content/elements/stringbundle.js, line 62:
> Not
> SupportedError: Operation is not supported
> 
> chrome://messenger/content/msgAccountCentral.xul================
> after CE ================
> chrome://messenger/content/msgAccountCentral.xul================
> JavaScript error: chrome://global/content/elements/stringbundle.js, line 62:
> Not
> SupportedError: Operation is not supported
> 
> chrome://messenger/content/messenger.xul================
> after CE ================
> chrome://messenger/content/messenger.xul================
> JavaScript error: chrome://global/content/elements/stringbundle.js, line 62:
> Not
> SupportedError: Operation is not supported
> 
> Almost looks like this is called twice and the second one fails.
> 
> Maybe we got this wrong:
> https://hg.mozilla.org/comm-central/rev/
> b00e830b9e5e3a006102519c34b8fc8e862e9cdf

Oh OK. Yes, if it's already defined it will throw that (not very helpfully named) exception - for example:

data:text/html,<script>customElements.define("foo-bar", class extends HTMLElement {}); customElements.define("foo-bar", class extends HTMLElement {})</script>

Are you sure that MainProcessSingleton isn't running already? Maybe you don't need to port it to mail glue. You could check with `console.trace();` right before your logs.

Comment 26

[Jorg K (CEST = GMT+2)]

OK, let's continue the discussion in bug 1459743.