Make the lightweight theme web installer ready for e10s

Categories

(Firefox :: General, defect)

For bugs in Firefox Desktop, the Mozilla Foundation's web browser. For Firefox user interface issues in menus, bookmarks, location bar, and preferences. Many Firefox bugs will either be filed here or in the Core product. Bugs for developer tools (F12) should be filed in the DevTools product. (more info)

This depends on the patch in bug 652510.
Not sure if I should put this in content.js, add a separate content-lwt.js or a content-misc.js for random small chunks (whereas content.js would be reserved for central code pieces).

Last I heard Brendan was advocating for a different approach to E10S for front-end code, and that still hadn't been settled. I'm not sure where the motivation for these patches comes from - are they just arbitrary starting points based on Fennec's approach?

Arbitrary starting points based on the fact that we have the message managers. The motivation was the idea that we have to start sooner rather than later. Is there something more concrete available about the different approach? I know nothing about it.

(In reply to comment #1)
> Last I heard Brendan was advocating for a different approach to E10S for
> front-end code, and that still hadn't been settled.
Brendan, could you please shed some light on this? Should this work wait? (How long should it wait?)

Comment on attachment 528549[details][diff][review]
patch
>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1614,19 +1614,19 @@ function delayedStartup(isLoadingBlank,
> switch (event.type) {
>- case "InstallBrowserTheme":
>- case "PreviewBrowserTheme":
>- case "ResetBrowserThemePreview":
>- // ignore requests from background tabs
>- if (event.target.ownerDocument.defaultView.top != content)
>- return;
>- }
(...)
>+ receiveMessage: function (msg) {
>+ // ignore requests from background tabs
>+ if (msg.target != gBrowser.selectedBrowser)
>+ return;
>+
Is it expected that pages inside iframes are also allowed to install LW themes? From the code removed it seems so, but if not, the check for msg.target != selectedBrowser won't be enough. The way that Fennec is handling cases like this is to send the contentWindowId (nsIDOMWindowUtils.currentInnerWindowID) together with the json data of the message. Then on the parent side's browser binding they have cached the id for the top window from content, so it's possible to tell if the msg came from the top or from an inner window.
>- this._previewWindow = event.target.ownerDocument.defaultView;
>- this._previewWindow.addEventListener("pagehide", this, true);
> gBrowser.tabContainer.addEventListener("TabSelect", this, false);
>
>- this._previewWindow.removeEventListener("pagehide", this, true);
>- this._previewWindow = null;
(...)
>diff --git a/browser/base/content/content.js b/browser/base/content/content.js
>--- a/browser/base/content/content.js
>+++ b/browser/base/content/content.js
>+ case "PreviewBrowserTheme":
>+ param.baseURI = event.target.baseURI;
>+ param.themeData = event.target.getAttribute("data-browsertheme");
>+ sendAsyncMessage("lightweight-theme-web-installer:preview", param);
>+ this._previewWindow = event.target.ownerDocument.defaultView;
>+ this._previewWindow.addEventListener("pagehide", this, true);
>+ break;
some food for thought: the pageshow/pagehide events are also used in other instances of our code (TabsProgressListener and Inspector), and in various add-ons. For cases like this we might want to add a generic listener in content (instead of one related to the LW feature) that will send a message and redispatch this event in the parent, so that the original listeners can remain intact.
This would also help in that a pagehide event will generate 1 inter-process message instead of 1 for each feature that is manually listening to it in content.
(This complicates the case of when e10s is not enabled, where we'll need to avoid generating the event twice)
The usefulness of this compatibility shim will vary case by case, and it will also depend on how the event is usually used. For example, the Inspector uses the target.ownerDocument of the event http://mxr.mozilla.org/mozilla-central/source/browser/base/content/inspector.js#1050 , so keeping it intact (and other add-ons that do the same thing) will depend if CPOWs will be used or not (still undecided)
>- gBrowser.mPanelContainer.addEventListener("InstallBrowserTheme", LightWeightThemeWebInstaller, false, true);
>- gBrowser.mPanelContainer.addEventListener("PreviewBrowserTheme", LightWeightThemeWebInstaller, false, true);
>- gBrowser.mPanelContainer.addEventListener("ResetBrowserThemePreview", LightWeightThemeWebInstaller, false, true);
>+ window.messageManager.addMessageListener("lightweight-theme-web-installer:install", LightWeightThemeWebInstaller);
>+ window.messageManager.addMessageListener("lightweight-theme-web-installer:preview", LightWeightThemeWebInstaller);
>+ window.messageManager.addMessageListener("lightweight-theme-web-installer:reset-preview", LightWeightThemeWebInstaller);>diff --git a/browser/base/content/content.js b/browser/base/content/content.js
>--- a/browser/base/content/content.js
>+++ b/browser/base/content/content.js
>@@ -1,8 +1,49 @@
>+addEventListener("InstallBrowserTheme", LightWeightThemeWebInstallBroker, false, true);
>+addEventListener("PreviewBrowserTheme", LightWeightThemeWebInstallBroker, false, true);
>+addEventListener("ResetBrowserThemePreview", LightWeightThemeWebInstallBroker, false, true);
I think we should make sure to add the listeners and load the framescripts through the same message manager, for good balance and correctness. I think the window MM is the best choice here, but as content.js is being loaded by the tabbrowser, the frame script is right now loaded through the browser MM
You said in comment 0:
> Not sure if I should put this in content.js, add a separate content-lwt.js or a content-misc.js for random small chunks (whereas content.js would be reserved for central code pieces).
I'm still torn about this too. Here's my view:
we should have different files for things that are related to the tabbrowser iface, and things that are related to UI features that access content. The one for UI would be loaded via window.mm.loadFrameScript(..., true) in browser.js (@ delayedStartup if possible). The one for tabbrowser would be loaded in the tabbrowser constructor like you did in bug 652510.
We'll also end with an extra one to share <browser> code with Fennec, and the initial code will come from http://mxr.mozilla.org/mozilla-central/source/mobile/chrome/content/bindings/browser.js (to be moved to toolkit soon)
For the UI parts specifically: although I like the catch-all of our browser.js, as it's possible to load various framescripts we should also split more things in different files (maybe parity with the browser-*.js files, maybe even more). We don't need to use the #include preprocessor at all, we can just load all the different scripts. This also gives the advantage that some features may be lazily loaded, loaded only for specific frames, etc.. (e.g. if Personas required a website to declare a <meta> tag to announce support for this feature, we then could load content-lwt.js only for that frame)
I also believe that we should create a new folder to put all these content side scripts, instead of mixing them all in our main folder and prepend their names with content-*. Naming the folder will be awkward as "content" is such an overloaded word in our codebase, but having these files in a dedicated place sounds better for clarity in the long run.

(In reply to comment #4)
Thanks!
> Is it expected that pages inside iframes are also allowed to install LW
> themes?
Yes, our first-run page utilizes this for instance.
> some food for thought: the pageshow/pagehide events are also used in other
> instances of our code (TabsProgressListener and Inspector), and in various
> add-ons. For cases like this we might want to add a generic listener in
> content (instead of one related to the LW feature) that will send a message
> and redispatch this event in the parent, so that the original listeners can
> remain intact.
Yeah, I expect this stuff to evolve quite a bit. I don't think this needs to happen in this bug.
> I think we should make sure to add the listeners and load the framescripts
> through the same message manager, for good balance and correctness. I think
> the window MM is the best choice here, but as content.js is being loaded by
> the tabbrowser, the frame script is right now loaded through the browser MM
I'm not sure how loading the frame script through the window MM would work out. Would it be loaded automatically for each new <browser>?
> For the UI parts specifically: although I like the catch-all of our
> browser.js, as it's possible to load various framescripts we should also
> split more things in different files (maybe parity with the browser-*.js
> files, maybe even more). We don't need to use the #include preprocessor at
> all, we can just load all the different scripts. This also gives the
> advantage that some features may be lazily loaded, loaded only for specific
> frames, etc.. (e.g. if Personas required a website to declare a <meta> tag
> to announce support for this feature, we then could load content-lwt.js only
> for that frame)
I think we might want to do it lazily where it makes sense, but I don't think we should require sites to add a meta tag just so that we can load the script lazily. And when we don't do it lazily, I tend to think we should stick with the preprocessing. Loading separate scripts just because we can seems ... not compelling enough?
> I also believe that we should create a new folder to put all these content
> side scripts, instead of mixing them all in our main folder and prepend
> their names with content-*. Naming the folder will be awkward as "content"
> is such an overloaded word in our codebase, but having these files in a
> dedicated place sounds better for clarity in the long run.
I'm generally sympathetic with this, but I really didn't want to create a content folder inside the content folder, so content-*.js seemed like the next best solution.

(In reply to comment #5)
> Yeah, I expect this stuff to evolve quite a bit. I don't think this needs to
> happen in this bug.
>
> > I think we should make sure to add the listeners and load the framescripts
> > through the same message manager, for good balance and correctness. I think
> > the window MM is the best choice here, but as content.js is being loaded by
> > the tabbrowser, the frame script is right now loaded through the browser MM
>
> I'm not sure how loading the frame script through the window MM would work
> out. Would it be loaded automatically for each new <browser>?
Yes, scripts loaded through the window/global MM with the second parameter of loadFrameScript set to true will be loaded automatically for all new <browser>s.
> I think we might want to do it lazily where it makes sense, but I don't
> think we should require sites to add a meta tag just so that we can load the
> script lazily. And when we don't do it lazily, I tend to think we should
> stick with the preprocessing. Loading separate scripts just because we can
> seems ... not compelling enough?
yeah, the meta tag was just an hypothetical example.
One advantage of loading them separately would be to make it easier to debug these files. specially syntax errors, see how the fennec remote files all have a dump() at the beginning because it's already hard to tell when the script loaded successfully.
Although of course syntax errors only is not an important use case.
still, it would be easier to toggle scripts on and off
I don't have any strong preference here though
>
> > I also believe that we should create a new folder to put all these content
> > side scripts, instead of mixing them all in our main folder and prepend
> > their names with content-*. Naming the folder will be awkward as "content"
> > is such an overloaded word in our codebase, but having these files in a
> > dedicated place sounds better for clarity in the long run.
>
> I'm generally sympathetic with this, but I really didn't want to create a
> content folder inside the content folder, so content-*.js seemed like the
> next best solution.
"content-scripts/" maybe?, if the folder is still wanted

> > var LightWeightThemeWebInstaller = {
> >+ receiveMessage: function (msg) {
> >+ // ignore requests from background tabs
> >+ if (msg.target != gBrowser.selectedBrowser)
> >+ return;
>
> Isn't this broken? AFAICT messages don't have "target" properties, though
> "this" might be usable according to the IDL (not sure exactly what it means
> by "the target of the message").
They do have the target property. I just updated the wiki because it was missing that info (https://developer.mozilla.org/en/The_message_manager)
The target of the message is the <browser> element from where that message came from.

srcURI and baseURI are the same thing. We can just use baseURI. I didn't add comments for it since there's only 1 variable now and it's clear to see from content.js what is it.
> Instead of listening for the message, can we not wait for the install
> notification instead, which is a consequence of the message being sent?
> That way, I think we can get rid of some of these executeSoons.
I will work on that test. But in the meantime can you review the changes I made to match your comments. Thanks!

I tested the timings and it seems that one of the executeSoon() was not needed.
> function test_install_lwtheme()
I think is more readable now.
I tried experimenting with the code for waitForCondition to get the waiting for notification box working. It's not working properly and I don't think it's worth the effort. So I will leave it as it is.
Let me know if there's any other changes I should make.

(In reply to Dão Gottwald [:dao] from comment #29)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #28)
> > > + case "LightweightThemeWebInstaller:ResetPreview": {
> > > + this._resetPreview(data && data.baseURI);
> >
> > It looks like _resetPreview takes a baseURI... but
> >
> > data && data.baseURI will resolve to either true or false.
>
> No, it will be null / an empty string (whichever the default for
> message.data is) or data.baseURI.
Ugh. Bitten by JS again. You're right. :)
I do think, however, we can make this more clear. Instead of taking advantage of weird JS coercion semantics, let's make this more explicit.
First, let's define data on line 413 such that if message.data is null, data = {}:
let data = message.data || {};
Then we don't need do check for the presence of data elsewhere. Then we do:
this._resetPreview(data.baseURI);
Which will be false-y if the baseURI is not defined. Sound good?

> Still haven't heard about whether or not it's impossible to wait for the
> notification bar instead. Listening for this message is too presumptuous of
> this test, imo.
I see your point. Yes it is possible to waitForNotificationBar. I had some trouble getting it to work before and I figured it wasn't worth the effort.
But you're right it's better to waitForNotificationBar. Can you take a look at my new test that waitForNotification?