653065 - Make the lightweight theme web installer ready for e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Dão Gottwald [:dao]]

Attachment: patch

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).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

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?

Comment 2

[Dão Gottwald [:dao]]

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.

Comment 3

[Dão Gottwald [:dao]]

(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 4

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 5

[Dão Gottwald [:dao]]

(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.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

(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

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 528549 [details] [diff] [review]
patch

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

> 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").

>+      case "lightweight-theme-web-installer:reset-preview":
>+        this._resetPreview(msg.json.srcURI);

This needs to handle a null msg.json (for the pagehide case).

>-  _installRequest: function (event) {
>-    var node = event.target;

There's another reference to "node" further down (lwthemeInstallRequest.message) that you forgot to update.

>diff --git a/browser/base/content/content.js b/browser/base/content/content.js

>+var LightWeightThemeWebInstallBroker = {
>+  handleEvent: function (event) {
>+    if (event.type == "pagehide") {

I think it might be clearer if you handled this next to the ResetBrowserThemePreview case (you can set param to null and fall through, with a comment) and then inlined _resetPreviewWindow.

(oops, I didn't realize this was a feedback request until I completed a review... general approach seems fine)

Comment 8

[:Felipe Gomes (needinfo for replies!)]

> > 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.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

Looks like you also need to update the IDL for nsIFrameMessageListener.

Comment 10

[Dão Gottwald [:dao]]

Attachment: updated patch

For some reason I can't get browser_bug592338.js to work.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Re-nomming, since while not a critical feature, it is a feature, and it's very much broken with e10s.

Comment 16

[Mike Kaply [:mkaply]]

Is there a chance themes won't make it into e10s Firefox or is this definitely going to get fixed at some point?

Comment 17

[Mike Conley (:mconley) (:⚙️)]

This is milestoned at M8 under tracking-e10s, and will be fixed before we ship with e10s enabled.

Comment 19

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug653065-lwt-e10s

Comment 21

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8629026 [details] [diff] [review]
Bug653065-lwt-e10s

Review of attachment 8629026 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Jimmy, I'd love to help with this review but I'm going on pto for the next 12 days and won't be able to look at it. Can you redirect the review to Dão?

Comment 22

[Dão Gottwald [:dao]]

Comment on attachment 8629026 [details] [diff] [review]
Bug653065-lwt-e10s

(In reply to :Felipe Gomes [away Jul 8 - 21, will reply to needinfos when back] from comment #21)
> Comment on attachment 8629026 [details] [diff] [review]
> Bug653065-lwt-e10s
> 
> Review of attachment 8629026 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Jimmy, I'd love to help with this review but I'm going on pto for the
> next 12 days and won't be able to look at it. Can you redirect the review to
> Dão?

I wrote the original patch which I think Jimmy updated. So to some extent I'd be reviewing my own code.

How about mconley?

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8629026 [details] [diff] [review]
Bug653065-lwt-e10s

Review of attachment 8629026 [details] [diff] [review]:
-----------------------------------------------------------------

Hey jimicy,

This is a good looking patch, and it's definitely on the right track! There are a few changes, style nits, indentation things below. Please check those out, and let me know if you have any questions.

Thanks so much!

::: browser/base/content/browser-addons.js
@@ +402,5 @@
> +    if (msg.target != gBrowser.selectedBrowser)
> +      return;
> +
> +    switch (msg.name) {
> +      case "lightweight-theme-web-installer:install":

So far, we've had a convention of capitalized message names, so I think I'd prefer this to be:

LightweightThemeWebInstaller:Install, etc etc

@@ +403,5 @@
> +      return;
> +
> +    switch (msg.name) {
> +      case "lightweight-theme-web-installer:install":
> +      this._installRequest(msg.json.srcURI, msg.json.baseURI, msg.json.themeData);

I would suggest you use .data instead of .json - I'm pretty sure .json is deprecated.

Also, indentation for these cases should be as follows:

case "LightweightThemeWebInstaller:Install": {
  this._installRequest(...);
  break;
}

@@ +406,5 @@
> +      case "lightweight-theme-web-installer:install":
> +      this._installRequest(msg.json.srcURI, msg.json.baseURI, msg.json.themeData);
> +      break;
> +      case "lightweight-theme-web-installer:preview":
> +      this._preview(msg.json.srcURI, msg.json.baseURI, msg.json.themeData);

Same as above, re: .json vs .data

@@ +409,5 @@
> +      case "lightweight-theme-web-installer:preview":
> +      this._preview(msg.json.srcURI, msg.json.baseURI, msg.json.themeData);
> +      break;
> +      case "lightweight-theme-web-installer:reset-preview":
> +      this._resetPreview(msg.json && msg.json.srcURI);

Same as above, re: .json vs .data

@@ +420,2 @@
>        case "TabSelect":
> +      this._resetPreview();

Nit - indentation.

@@ +429,5 @@
>      delete this._manager;
>      return this._manager = temp.LightweightThemeManager;
>    },
>  
> +  _installRequest: function (srcURI, baseURI, dataString) {

I difference between srcURI and baseURI is not clear to me. Can you please add some documentation to tell us about the difference?

@@ +430,5 @@
>      return this._manager = temp.LightweightThemeManager;
>    },
>  
> +  _installRequest: function (srcURI, baseURI, dataString) {
> +    var data = this._manager.parseTheme(dataString, baseURI);

let, not var

@@ +436,2 @@
>      if (!data)
> +     return;

Busted indentation

@@ +438,3 @@
>  
> +    if (this._isAllowed(srcURI)) {
> +     this._install(data);

Indentation

@@ +442,3 @@
>      }
>  
> +    var allowButtonText      = gNavigatorBundle.getString("lwthemeInstallRequest.allowButton");

I think the old formatting allowed us to stay under 80 characters - can we please go back to that?

@@ +448,2 @@
>      var buttons = [{
> +     label: allowButtonText,

Indentation

@@ +486,5 @@
>            timeout: Date.now() + 30000
>          };
>  
>          PopupNotifications.show(gBrowser.selectedBrowser, "addon-theme-change",
> +          messageString, "addons-notification-icon",

Please put these back.

@@ +549,3 @@
>        return;
>  
> +    var data = this._manager.parseTheme(dataString, baseURI);

let, not var

@@ +568,2 @@
>      var pm = Services.perms;
> +    return pm.testPermission(makeURI(srcURIString), "install") == pm.ALLOW_ACTION;

We used to ensure that the scheme was https. We should keep doing that.

makeURI might also fail if srcURIString is a nonsense URI, which we should probably account for (as in, if we cannot create a URI, we should return false).

::: browser/base/content/browser.js
@@ +1364,5 @@
>      placesContext.addEventListener("popupshowing", updateEditUIVisibility, false);
>      placesContext.addEventListener("popuphiding", updateEditUIVisibility, false);
>  #endif
>  
> +    window.messageManager.addMessageListener("lightweight-theme-web-installer:install", LightWeightThemeWebInstaller);

While we're here, can we have LightWeightThemeWebInstaller have an init function that sets these message listeners? That way, the list of messages we listen for and the handlers for those messages are all in one place.

::: browser/base/content/content.js
@@ +760,5 @@
>    sendAsyncMessage("ContextMenu:SearchFieldBookmarkData:Result",
>                     { spec, title, description, postData, charset });
>  });
>  
> +let LightWeightThemeWebInstallBroker = {

I don't think we call anything else a Broker down here. I think Listener is the convention.

Let's call this LightWeightThemeWebInstallListener. (What a mouthful!)

@@ +761,5 @@
>                     { spec, title, description, postData, charset });
>  });
>  
> +let LightWeightThemeWebInstallBroker = {
> +  handleEvent: function (event) {

Let's have a _previewWindow set by default to null up here, instead of relying on expando.

@@ +762,5 @@
>  });
>  
> +let LightWeightThemeWebInstallBroker = {
> +  handleEvent: function (event) {
> +    if (event.type == "pagehide") {

Why not put this in the switch statement?

@@ +768,5 @@
> +      this._resetPreviewWindow();
> +      return;
> +    }
> +
> +    var param = { srcURI: event.target.ownerDocument.documentURIObject.spec };

let, not var. Also, let's call this data, and not param (as it will be accessed via the data property off of the message).

@@ +798,5 @@
> +    this._previewWindow = null;
> +  }
> +};
> +
> +addEventListener("InstallBrowserTheme", LightWeightThemeWebInstallBroker, false, true);

Let's put these inside of an .init function on the LightWieghtThemeWebInstallListener, and just call init from here.

::: browser/base/content/test/general/browser_bug592338.js
@@ +29,5 @@
>  
>      gBrowser.selectedBrowser.removeEventListener("pageshow", arguments.callee, false);
>  
>      executeSoon(function() {
> +      BrowserTestUtils.synthesizeMouse("#theme-install", 2, 2, {}, gBrowser.selectedBrowser);

We'll also need to re-enable the test inside of the browser.ini file in browser/base/content/test/general/. Just remove the skip-if line.

@@ +50,5 @@
>    pm.add(makeURI("http://example.com/"), "install", pm.ALLOW_ACTION);
>  
>    gBrowser.selectedTab = gBrowser.addTab("https://example.com/browser/browser/base/content/test/general/bug592338.html");
>    gBrowser.selectedBrowser.addEventListener("pageshow", function() {
> +  if (gBrowser.contentDocument.location.href == "about:blank")

Busted indentation in here.

@@ +58,3 @@
>  
> +   executeSoon(function() {
> +    messageManager.addMessageListener("lightweight-theme-web-installer:install",

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.

Comment 24

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: mconley-patch-suggestions-revised-v1

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!

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8632963 [details] [diff] [review]
mconley-patch-suggestions-revised-v1

Hey Jimmy - can you please fold your two patches together?

Comment 26

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug653065-lwt-e10s-v2

Comment 27

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug653065-lwt-e10s-v3

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.

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8633345 [details] [diff] [review]
Bug653065-lwt-e10s-v3

Review of attachment 8633345 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good jimicy, thanks!

::: browser/base/content/browser-addons.js
@@ +398,5 @@
>  
>  var LightWeightThemeWebInstaller = {
> +  init: function () {
> +    let mm = window.messageManager;
> +    mm.addMessageListener("LightweightThemeWebInstaller:Install", this);

Note the inconsistency between the name of this thing, "LightWeightThemeWebInstaller" vs the message pseudonamespace's "LightweightThemeWebInstaller".

We should probably make those consistent. Sorry I missed that first time around.

@@ +421,5 @@
> +        this._preview(data.baseURI, data.themeData);
> +        break;
> +      }
> +      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.

@@ +444,5 @@
>      return this._manager = temp.LightweightThemeManager;
>    },
>  
> +  _installRequest: function (baseURI, dataString) {
> +    let data = this._manager.parseTheme(dataString, baseURI);

Nit - it seems a little strange for the manager to accept the arguments in the order of "dataString, baseURI", and for the other methods in this function to use "baseURI, dataString".

Can you make the methods of this function follow the "dataString, baseURI" order instead?

@@ +589,5 @@
> +      uri = makeURI(srcURIString);
> +    }
> +    catch(e) {
> +      //makeURI fails if srcURIString is a nonsense URI
> +      console.log("nonsense");

We can't do this sort of thing.

If we want to log an error message for this, we should use Cu.reportError, and we should say something a bit more comprehensive.

@@ +600,3 @@
>  
> +    let pm = Services.perms;
> +    return pm.testPermission(makeURI(srcURIString), "install") == pm.ALLOW_ACTION;

We shouldn't need to makeURI again - just use uri.

::: browser/base/content/content.js
@@ +726,5 @@
> +    switch (event.type) {
> +      case "InstallBrowserTheme": {
> +        data.baseURI = event.target.baseURI;
> +        data.themeData = event.target.getAttribute("data-browsertheme");
> +        sendAsyncMessage("LightweightThemeWebInstaller:Install", data);

If you're going to arrange the switch like this, then let's get rid of the data variable all together and just do:

sendAsyncMessage("LightWeightThemeWebInstaller:Install", {
  baseURI: event.target.baseURI,
  themeData: event.target.getAttribute("data-browsertheme"),
});

same goes for the other cases.

::: browser/base/content/test/general/browser_bug592338.js
@@ +56,5 @@
>  
>      gBrowser.selectedBrowser.removeEventListener("pageshow", arguments.callee, false);
>  
>      executeSoon(function() {
> +      messageManager.addMessageListener("LightweightThemeWebInstaller:Install",

So did waiting for the notification not work out?

Comment 29

[Dão Gottwald [:dao]]

(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.

Comment 30

[Mike Conley (:mconley) (:⚙️)]

(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?

Comment 31

[Dão Gottwald [:dao]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> (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 || {};

This is taking advantage of the very same semantics, i.e. it doesn't make data boolean. I don't think this is something we need to worry about, it's a well-established pattern in our code base and elsewhere.

Comment 32

[Mike Conley (:mconley) (:⚙️)]

(In reply to Dão Gottwald [:dao] from comment #31)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #30)
> > (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 || {};
> 
> This is taking advantage of the very same semantics, i.e. it doesn't make
> data boolean. I don't think this is something we need to worry about, it's a
> well-established pattern in our code base and elsewhere.

Fair point. I guess I'm more used to seeing the || manifestation of this.

I'm willing to relent on this point.

Comment 33

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug653065-lwt-e10s-v4

Comment 34

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8633584 [details] [diff] [review]
Bug653065-lwt-e10s-v4

Review of attachment 8633584 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/content.js
@@ +720,5 @@
> +let LightWeightThemeWebInstallListener = {
> +  _previewWindow: null,
> +
> +  handleEvent: function (event) {
> +

Nit - remove empty newline

@@ +723,5 @@
> +  handleEvent: function (event) {
> +
> +    switch (event.type) {
> +      case "InstallBrowserTheme": {
> +        sendAsyncMessage("LightWeightThemeWebInstaller:Install", {

I'd prefer this indentation style for these:

sendAsyncMessage("LightWeightThemeWebInstaller:Install", {
  baseURI: event.target.baseURI,	
  themeData: event.target.getAttribute("data-browsertheme"),
});

(Note the trailing comma - added so that we're not forced to modify blame on the line if we add more stuff to this later)

@@ +758,5 @@
> +    this._previewWindow = null;
> +  }
> +};
> +
> +addEventListener("InstallBrowserTheme", LightWeightThemeWebInstallListener, false, true);

Let's put these in an init function in LightWeightThemeWebInstallListener instead, call that method from here, and then do:

addEventListener("InstallBrowserTheme", this, false, true);
etc

::: browser/base/content/test/general/browser_bug592338.js
@@ +56,5 @@
>  
>      gBrowser.selectedBrowser.removeEventListener("pageshow", arguments.callee, false);
>  
>      executeSoon(function() {
> +      messageManager.addMessageListener("LightWeightThemeWebInstaller:Install",

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.

Comment 35

[Jimmy Wang (:jimicy) - works on e10s stuff]

Attachment: Bug653065-lwt-e10s-v5

> 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?

Comment 36

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8633623 [details] [diff] [review]
Bug653065-lwt-e10s-v5

Review of attachment 8633623 [details] [diff] [review]:
-----------------------------------------------------------------

Assuming a green try run, this looks good to me. Thanks jimicy!

Comment 37

[Jimmy Wang (:jimicy) - works on e10s stuff]

Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=d730d0bc1057. The reds and oranges do not seem related to this patch.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/83cc3090b16c

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/83cc3090b16c