Closed Bug 1459245 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: UI Widgets, task)

Product:
Toolkit
Component:
UI Widgets
Type:
task
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(1 file)

Bug 1459245 - Migrate <stringbundle> from a XBL binding to a Custom Element;
59 bytes, text/x-review-board-request
mossop
: review+
Details 
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>.
Requesting review now, but definitely don't want to land anything here until after the merge
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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.
Flags: needinfo?(jorgk)
Thanks, I'll look into it.
Comment on attachment 8973271 [details]
Bug 1459245 - Migrate <stringbundle> from a XBL binding to a Custom Element;

https://reviewboard.mozilla.org/r/241726/#review248016
Attachment #8973271 - Flags: review?(dtownsend) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/80fc39b7c5ec
Migrate <stringbundle> from a XBL binding to a Custom Element;r=mossop
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
Flags: needinfo?(jorgk) → needinfo?(bgrinstead)
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);
https://hg.mozilla.org/mozilla-central/rev/80fc39b7c5ec
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(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.
Flags: needinfo?(bgrinstead)
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).
Flags: needinfo?(dtownsend)
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.
Please also fix the error from comment #11.
(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.
(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?
(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?
(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.
Flags: needinfo?(dtownsend)
(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).
Depends on: 1459985
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.
(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?
Flags: needinfo?(jorgk)
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
Flags: needinfo?(jorgk)
(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.
OK, let's continue the discussion in bug 1459743.
Depends on: 1460334
Blocks: 1460334
No longer depends on: 1460334
Blocks: 1461388
No longer blocks: 1460334
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: