Closed Bug 653065 Opened 9 years ago Closed 4 years ago

Make the lightweight theme web installer ready for e10s

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
e10s m8+ ---
firefox42 --- fixed

People

(Reporter: dao, Assigned: jimicy)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 7 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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).
Attachment #528549 - Flags: feedback?(gavin.sharp)
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?)
Attachment #528549 - Flags: feedback?(felipc)
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.
Attachment #528549 - Flags: feedback?(felipc) → feedback+
(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
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)
Attachment #528549 - Flags: feedback?(gavin.sharp) → feedback+
> > 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.
Looks like you also need to update the IDL for nsIFrameMessageListener.
Attached patch updated patch (obsolete) — Splinter Review
For some reason I can't get browser_bug592338.js to work.
Attachment #528549 - Attachment is obsolete: true
Assignee: dao → nobody
Status: ASSIGNED → NEW
Flags: qe-verify?
Flags: in-testsuite+
Flags: firefox-backlog+
Duplicate of this bug: 1120297
Duplicate of this bug: 1148363
Re-nomming, since while not a critical feature, it is a feature, and it's very much broken with e10s.
Duplicate of this bug: 1105131
Duplicate of this bug: 1154788
Is there a chance themes won't make it into e10s Firefox or is this definitely going to get fixed at some point?
This is milestoned at M8 under tracking-e10s, and will be fixed before we ship with e10s enabled.
Duplicate of this bug: 1165847
Assignee: nobody → jimmyw22
Status: NEW → ASSIGNED
Attached patch Bug653065-lwt-e10s (obsolete) — Splinter Review
Attachment #8629026 - Flags: review?(felipc)
Duplicate of this bug: 1180426
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?
Attachment #8629026 - Flags: review?(felipc)
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?
Attachment #8629026 - Flags: review?(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.
Attachment #8629026 - Flags: review?(mconley) → review-
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!
Attachment #8632963 - Flags: review?(mconley)
Attachment #8629026 - Attachment is obsolete: true
Attachment #8629026 - Attachment is obsolete: false
Comment on attachment 8632963 [details] [diff] [review]
mconley-patch-suggestions-revised-v1

Hey Jimmy - can you please fold your two patches together?
Attachment #8632963 - Flags: review?(mconley) → review-
Attachment #8632963 - Attachment is obsolete: true
Attached patch Bug653065-lwt-e10s-v2 (obsolete) — Splinter Review
Attachment #8633033 - Flags: review?(mconley)
Attached patch Bug653065-lwt-e10s-v3 (obsolete) — Splinter Review
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.
Attachment #8633033 - Attachment is obsolete: true
Attachment #8633033 - Flags: review?(mconley)
Attachment #8633345 - Flags: review?(mconley)
Attachment #8629026 - Attachment is obsolete: true
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?
Attachment #8633345 - Flags: review?(mconley) → review-
(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.
(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?
(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.
(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.
Attached patch Bug653065-lwt-e10s-v4 (obsolete) — Splinter Review
Attachment #8633345 - Attachment is obsolete: true
Attachment #8633584 - Flags: review?(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.
Attachment #8633584 - Flags: review?(mconley) → feedback+
> 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?
Attachment #8633584 - Attachment is obsolete: true
Attachment #8633623 - Flags: review?(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!
Attachment #8633623 - Flags: review?(mconley) → review+
Try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=d730d0bc1057. The reds and oranges do not seem related to this patch.
Attachment #564517 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/83cc3090b16c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.