1444459 - Introduce a mechanism that broadcasts theme updates to content processes

Status: RESOLVED FIXED

(WebExtensions :: Themes, enhancement, P3)

Description

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

Bug 1347204 has us themeing about:newtab, which currently runs out of process. We should introduce a mechanism that allows us to broadcast theme updates to content processes.

How I envision this working:

LightweightThemeManager currently broadcasts state updates via the nsIObserverService. I think what we might want to do is have it instead broadcast its message via Services.ppmm.broadcastAsyncMessage. Then, in process-content.js, listen for that broadcast, and send out the nsIObserverService notification there.

A single copy of process-content.js runs in both the parent process and each content process. This means that upon receiving the message, the parent will get the observer notification (as LightweightThemeConsumer.jsm currently expects), and everything should continue to work on that front.

Each content process will also get the message, and what I imagine is that upon receipt of it, the process-content.js script will find each top-level window that matches some whitelist of about: pages that run in content, and for each one, dispatch an event with the theme information.

Supported pages, like about:newtab, will then be responsible for listening for that event, and updating appropriately.

There are other cases to consider:

1. A content process starts up after a theme change:

Note that once the parent receives an update in process-content.js, it should update Services.ppmm initProcessData. That way, new content processes can check that value, and update any existing whitelisted pages appropriately.

2. A new whitelisted page is opened in a pre-existing content process after a theme change

We'll probably want some mechanism where, upon load, an event is bubbled up, and the privileged scope responds with the latest theme data it's aware of (either via initProcessData, or after some update). This means we need to cache the update in the content process after it's been received.

This is all still a little hand-wavey, but this is where my thinking is currently taking me.

Comment 1

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

ntim talked to me about this a bit last week, and I think I have to make some modifications to this plan.

Critically, I failed to account for the fact that individual windows can have different themes. This doesn't fit in with the model I described above. So I'm going to modify my solution a bit:

Each window has a LightweightThemeConsumer instance that has information about its current theme. When a theme is applied, the Firefox LightweightThemeConsumer will broadcast a message to every browser within that window with the theme information.

Each tab-content.js within those browsers will receive and cache the payload of that message.

If a new tab is added to a window that's being themed, the theme data will need to be sent not long after the initial load URI message.

If a tab is torn into a new window, it'll need to have its theme updated if the theme applied to the new window is different from the theme of the old window.

Inside tab-content.js, I think we'll want to observe something like the load, DOMContentLoaded or pageshow events during capture for whitelisted about: pages. When we see them, I think we can pin the cached theme information on the event. The page will then be responsible for listening to the event and applying the CSS variables (hopefully in a requestAnimationFrame).

We'll also need to be able to send an event down to content anytime the theme updates so that the CSS variables can be re-tweaked.

So I suspect the work is in 3 areas: LightweightThemeConsumer.jsm, tab-content.js, and a utility script like contentSearchUI.js that about: pages can load to take care of listening for events from tab-content.js and setting CSS variables.

Does this sound more in-tune with how our theme-ing API works, ntim?

Comment 2

[Tim Nguyen :ntim]

(In reply to Mike Conley (:mconley) (:⚙️) (Totally backlogged on reviews and needinfos) from comment #1)
> ntim talked to me about this a bit last week, and I think I have to make
> some modifications to this plan.
> 
> Critically, I failed to account for the fact that individual windows can
> have different themes. This doesn't fit in with the model I described above.
> So I'm going to modify my solution a bit:
> 

> 
> Each tab-content.js within those browsers will receive and cache the payload
> of that message.
> 
> If a new tab is added to a window that's being themed, the theme data will
> need to be sent not long after the initial load URI message.
> 
> If a tab is torn into a new window, it'll need to have its theme updated if
> the theme applied to the new window is different from the theme of the old
> window.
> 
> Inside tab-content.js, I think we'll want to observe something like the
> load, DOMContentLoaded or pageshow events during capture for whitelisted
> about: pages. When we see them, I think we can pin the cached theme
> information on the event. The page will then be responsible for listening to
> the event and applying the CSS variables (hopefully in a
> requestAnimationFrame).
> 
> We'll also need to be able to send an event down to content anytime the
> theme updates so that the CSS variables can be re-tweaked.
> 
> So I suspect the work is in 3 areas: LightweightThemeConsumer.jsm,
> tab-content.js, and a utility script like contentSearchUI.js that about:
> pages can load to take care of listening for events from tab-content.js and
> setting CSS variables.
> 
> Does this sound more in-tune with how our theme-ing API works, ntim?

The approach looks generally fine to me and in-tune with how the theme API works.

> LightweightThemeConsumer will broadcast a message to every browser within
> that window with the theme information.

I'm concerned with how well this would perform if we broadcast a message to *every browser* (maybe I don't know enough about the message broadcasting mechanism though). Maybe pages should have to register themselves to receive the messages, and the messages would just be broadcasted to the registered browsers?

> If a tab is torn into a new window, it'll need to have its theme updated if the theme applied to the new window is different from the theme of the old window.

Great point. How would we handle that situation ? Is there an event that's emitted when the tab is torn ?

Comment 3

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

(In reply to Tim Nguyen :ntim from comment #2)
> 
> I'm concerned with how well this would perform if we broadcast a message to
> *every browser* (maybe I don't know enough about the message broadcasting
> mechanism though). Maybe pages should have to register themselves to receive
> the messages, and the messages would just be broadcasted to the registered
> browsers?
> 

The message managers use a pub/sub model, so the publisher doesn't really have really fine-grained control over who it's messaging - it just broadcasts to the subscribers. The other thing is that any of the other tabs that aren't at one of these about: pages could browse _to_ one of those about: pages, and then it'll need the information. So every tab-content.js kinda needs it. :/

Comment 4

[Bogdan Pozderca [:bogdan]]

Attachment: Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

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

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8967527 [details]
Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

https://reviewboard.mozilla.org/r/236208/#review241980


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/tab-content.js:109
(Diff revision 1)
> +  },
> +
> +  receiveMessage(message) {
> +    switch (message.name) {
> +      case "LightweightTheme:Update":
> +        console.log("we received the message!!!!!!!!!!!!!!")

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/tab-content.js:148
(Diff revision 1)
> +
> +  _update(msgName, msgData) {
> +    console.log(msgName, msgData);
> +    console.log("update called, that's cool");
> +    if (this._contentWhitelisted) {
> +      console.log("boom boom boom _update called and whitelisted page!")

Error: Missing semicolon. [eslint: semi]

Comment 6

[Zhengyi Lian]

Attachment: WIP: Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

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

Comment 7

[Zhengyi Lian]

Comment on attachment 8967600 [details]
WIP: Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

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

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8967600 [details]
WIP: Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

https://reviewboard.mozilla.org/r/236306/#review242076


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/base/content/contentTheme.js:15
(Diff revision 2)
> +  init() {
> +    addEventListener("LightweightTheme:Set", this);
> +  },
> +
> +  handleEvent(event) {
> +    console.log(event.detail.type)

Error: Missing semicolon. [eslint: semi]

::: browser/base/content/contentTheme.js:66
(Diff revision 2)
> +
> +   return ContentThemeController;
> +})();
> +*/
> +
> +//addEventListener("LightweightTheme:Set", function(aEvent){

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

::: browser/base/content/contentTheme.js:68
(Diff revision 2)
> +})();
> +*/
> +
> +//addEventListener("LightweightTheme:Set", function(aEvent){
> +//         sendAsyncMessage("LightweightTheme:Set");
> +//});

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]

Comment 9

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

Comment on attachment 8967600 [details]
WIP: Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

https://reviewboard.mozilla.org/r/236306/#review242292

Yeah, this is totally the right idea! You've just got to do the last step which is to wire the data that's being sent down to a CSS variable, and then update plugins.html to use that CSS variable. Maybe something simple, like the background colour.

And then we've got our proof of concept!

::: browser/base/content/contentTheme.js:7
(Diff revision 2)
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +console.log("pls work");

heh

::: browser/base/content/contentTheme.js:18
(Diff revision 2)
> +
> +  handleEvent(event) {
> +    console.log(event.detail.type)
> +    switch (event.detail.type) {
> +      case "LightweightTheme:Update":
> +        console.log("We have received the Set and");

This is the right idea!

Let's clean up some of the dead code in here, and then try to set a CSS variable from the data next.

Remember that the data doesn't need to be stringified first (see my other comment about that), so you should have direct access to the data object that LightweightThemConsumer sent down.

::: browser/base/content/tab-content.js:102
(Diff revision 2)
> +    "about:plugins",
> +    // "about:newtab",
> +  ]),
> +
> +  init() {
> +    addEventListener("pageshow", this, false);

I wonder if we should add a trusted, chrome-only LightweightThemeSupport event that gets fired by contentTheme.js as soon as the script is loaded. tab-content.js can then listen for that event, and if it sees it, then it adds the pageshow event handler (and removes it on pagehide, I think).

We do something similar for the AboutHomeListener with the AboutHomeLoad event here: https://searchfox.org/mozilla-central/source/browser/base/content/tab-content.js#95-119

That way, we don't have to handle the pageshow event for _every single_ page that is shown (which is a good thing).

::: browser/base/content/tab-content.js:107
(Diff revision 2)
> +    switch (message.name) {
> +      case "LightweightTheme:Update":
> +        this._update(message.name, message.data);
> +        break;
> +    }

Generally, if there's only a single message name we're listening for, we can skip the switch and just use a standard if condition to check the name.

::: toolkit/modules/LightweightThemeConsumer.jsm:161
(Diff revision 2)
>      else
>        root.removeAttribute("lwthemefooter");
>  
> +    let browserMessageManager = this._win.getGroupMessageManager("browsers");
> +    browserMessageManager.broadcastAsyncMessage("LightweightTheme:Update",
> +                                                JSON.stringify(aData));

Thankfully, you don't have to stringify the data - unlike postMessage, we use structured clone to automatically serialize and deserialize an object that you pass. So you can just pass aData.

Comment 10

[Bogdan Pozderca [:bogdan]]

Comment on attachment 8967527 [details]
Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

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

Comment 11

[Bogdan Pozderca [:bogdan]]

Comment on attachment 8967527 [details]
Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

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

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8967527 [details]
Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

https://reviewboard.mozilla.org/r/236208/#review242518

This is a great start!

Theme updates are successfully broadcast, so this successfully works:
- Open about:plugins
- Apply a theme

but the opposite doesn't work:
- Apply a theme
- Open about:plugins

What you can do to fix this is storing the last per-window theme somewhere, and firing the update event every time a whitelisted page is loaded.

::: browser/base/content/tab-content.js:97
(Diff revision 3)
>  });
>  
> +var LightweightThemeListener = {
> +  whitelist: new Set([
> +    // "about:home",
> +    "about:plugins",

I know this is just a proof of concept, but I would prefer if we used pages that already have CSS variables (like about:newtab, about:about, about:preferences, about:addons, ...).

::: browser/base/content/tab-content.js:119
(Diff revision 3)
> +    if (event.originalTarget.defaultView != content) {
> +      return;
> +    }
> +
> +    switch (event.type) {
> +      case "LightweightTheme:Support":

I don't understand how this custom   "LightweightTheme:Support" is different from just the "pageshow" event.

Comment 13

[Tim Nguyen :ntim]

Comment on attachment 8967527 [details]
Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

https://reviewboard.mozilla.org/r/236208/#review242518

> I don't understand how this custom   "LightweightTheme:Support" is different from just the "pageshow" event.

Ah, I just read mconley's previous review. Makes sense to have a chrome only event :) Would be nice to clarify this with a comment though.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8967527 [details]
Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

https://reviewboard.mozilla.org/r/236208/#review242828

(clearing review since ntim and mconley have already looked at this)

Comment 15

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

Cc'ing ckerschb, since I think he has interest in this kind of thing, and probably wants to know where we're trying to head here

Comment 16

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

Comment on attachment 8967527 [details]
Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

https://reviewboard.mozilla.org/r/236208/#review242518

> I know this is just a proof of concept, but I would prefer if we used pages that already have CSS variables (like about:newtab, about:about, about:preferences, about:addons, ...).

Yeah, about:plugins was just a proof of concept. We shouldn't ship with it.

Comment 17

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

Comment on attachment 8967527 [details]
Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content processes

https://reviewboard.mozilla.org/r/236208/#review243434

This is coming along nicely, but I have some suggestions. See below!

::: browser/base/content/contentTheme.js:7
(Diff revision 3)
> +const inContentVariableMap = [
> +  ["--plugins-bgcolor", {
> +    lwtProperty: "toolbarColor"
> +  }],
> +  ["--plugins-accent-color", {
> +    lwtProperty: "accentcolor",
> +    processColor(rgbaChannels, element) {
> +      if (!rgbaChannels) {
> +        return "white";
> +      }
> +      // Remove the alpha channel
> +      const {r, g, b} = rgbaChannels;
> +      return `rgb(${r}, ${g}, ${b})`;
> +    }
> +  }],
> +];

Since this is going to be loaded inside all themed in-content pages, the fewer globals we can expose, the better. If we need this mapping, can we hang it off of the well-named ContentThemeController "namespace" as a property? Ultimately, I want contentTheme.js to expose 1 or fewer globals to importers.

::: browser/base/content/contentTheme.js:32
(Diff revision 3)
> +    addEventListener("LightweightTheme:Set", this);
> +  },
> +
> +  handleEvent(event) {
> +    switch (event.detail.type) {
> +      case "LightweightTheme:Update":

Is this supposed to be LightweightTheme:Set? LightweightTheme:Update is the name of the _message_ that the parent sends the tab-content scripts.

::: browser/base/content/contentTheme.js:70
(Diff revision 3)
> +function _sanitizeCSSColor(doc, cssColor) {
> +  if (!cssColor) {
> +    return null;
> +  }
> +  // style.color normalizes color values and rejects invalid ones, so a
> +  // simple round trip gets us a sanitized color value.
> +  let span = doc.createElementNS("http://www.w3.org/1999/xhtml", "span");
> +  span.style.color = cssColor;
> +  cssColor = doc.defaultView.getComputedStyle(span).color;
> +  if (cssColor == "rgba(0, 0, 0, 0)") {
> +    return null;
> +  }
> +  return cssColor;
> +}
> +
> +function _parseRGBA(aColorString) {
> +  if (!aColorString) {
> +    return null;
> +  }
> +  var rgba = aColorString.replace(/(rgba?\()|(\)$)/g, "").split(",");
> +  rgba = rgba.map(x => parseFloat(x));
> +  return {
> +    r: rgba[0],
> +    g: rgba[1],
> +    b: rgba[2],
> +    a: rgba[3] || 1,
> +  };
> +}
> +
> +window.addEventListener("pageshow", function() {
> +  var event = new CustomEvent("LightweightTheme:Support", {bubbles: true});
> +  document.dispatchEvent(event);
> +});

See above - let's see if we can get all of this stuff inside the ContentThemeController namespace.

::: browser/base/content/contentTheme.js:99
(Diff revision 3)
> +window.addEventListener("pageshow", function() {
> +  var event = new CustomEvent("LightweightTheme:Support", {bubbles: true});
> +  document.dispatchEvent(event);
> +});

Now come to think of it, pageshow is fired in two cases:

1. The page is loaded
2. The page is gone back to in history

I don't think we necessarily need to re-fire this event in the latter case. Maybe we can just fire this LightweightTheme:Support event inside the init() function.

::: browser/base/content/tab-content.js:129
(Diff revision 3)
> +        break;
> +    }
> +  },
> +
> +  onPageLoad() {
> +    addMessageListener("LightweightTheme:Update", this);

We should probably add this listener in init().

The reason I say this is because we want to support the common-case scenario where a non-default theme is already applied, and we're opening some in-content page for the first time (like about:newtab). In that case, what we want to do is hear the LightweightTheme:Support event, and send down a cache of the theme data that we're holding onto in this LightweightThemeListener, rather than waiting until the parent tells us about theme updates and _then_ sending down the LightweightTheme:Update event.

::: browser/base/content/tab-content.js:133
(Diff revision 3)
> +  onPageHide(aEvent) {
> +    removeMessageListener("LightweightTheme:Update", this);
> +    removeEventListener("pagehide", this, true);
> +  },

I don't actually think we need to do this - we shouldn't depend on the loading of the page to be listening for things. Instead, we should registers the listeners on init, cache any updates we receive from the parent, and when we receive the LightweightTheme:Support event from content, immediately update the theme in content.

::: browser/base/jar.mn:122
(Diff revision 3)
>          content/browser/web-panels.js                 (content/web-panels.js)
>  *       content/browser/web-panels.xul                (content/web-panels.xul)
>          content/browser/webext-panels.js              (content/webext-panels.js)
>  *       content/browser/webext-panels.xul             (content/webext-panels.xul)
>          content/browser/nsContextMenu.js              (content/nsContextMenu.js)
> +        content/browser/contentTheme.js                (content/contentTheme.js)

Nit - alignment (for (content/contentTheme.js))

::: browser/base/jar.mn:134
(Diff revision 3)
>  # the following files are browser-specific overrides
>  *       content/browser/license.html                  (/toolkit/content/license.html)
>  % override chrome://global/content/license.html chrome://browser/content/license.html
>          content/browser/blockedSite.xhtml               (content/blockedSite.xhtml)
>  
> +

Nit: please remove this new newline.

::: toolkit/modules/LightweightThemeConsumer.jsm:159
(Diff revision 3)
> +    let browserMessageManager = this._win.getGroupMessageManager("browsers");
> +    browserMessageManager.broadcastAsyncMessage("LightweightTheme:Update", aData);

We also have to broadcast a LightweightTheme:Update message anytime a new browser is inserted, or when a browser tab is dragged into the associated window. We should probably support that right out of the gate.

::: toolkit/modules/LightweightThemeConsumer.jsm:160
(Diff revision 3)
>        root.setAttribute("lwthemefooter", "true");
>      else
>        root.removeAttribute("lwthemefooter");
>  
> +    let browserMessageManager = this._win.getGroupMessageManager("browsers");
> +    browserMessageManager.broadcastAsyncMessage("LightweightTheme:Update", aData);

We might want to consider having a whitelist of properties that are sent down to content, and just sending those rather than the whole aData object.

Can we examine Chromium's support for newtab theme-ing, and then once we map that to the properties that we support, create a whitelist here and construct an object with only those whitelisted properties to send down to content?

Comment 18

[Tim Nguyen :ntim]

(In reply to Tim Nguyen :ntim from comment #12)
> Comment on attachment 8967527 [details]
> Bug 1444459 - Introduce a mechanism that broadcasts theme updates to content
> processes
> 
> https://reviewboard.mozilla.org/r/236208/#review242518
> 
> This is a great start!
> 
> Theme updates are successfully broadcast, so this successfully works:
> - Open about:plugins
> - Apply a theme
> 
> but the opposite doesn't work:
> - Apply a theme
> - Open about:plugins
> 
> What you can do to fix this is storing the last per-window theme somewhere,

(Just a minor note, since I just noticed this now) This is already stored under LWTConsumer.jsm under this._lastData;.

Comment 19

[Tim Nguyen :ntim]

I'm landing this work in bug 1347204.

Comment 20

[CosminB]

Is manual testing required on this bug? If yes, please provide some STR and the proper extension(if required) or set the “qe-verify -“ flag.

Thanks!