Implement Google Chrome New Tab Page color properties

RESOLVED FIXED in Firefox 63

Status

P5
normal
RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: mikedeboer, Assigned: ntim)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-complete, meta})

unspecified
mozilla63
dev-doc-complete, meta
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: triaged [ntim-intern-project], URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
See (again) https://docs.google.com/spreadsheets/d/1YScpOVL5WdNDhQY2Nngh4YkK0bOpkfwJvpRjpMSxMWo/edit#gid=0

Constants as defined in https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/themes/theme_properties.h are:
 * COLOR_NTP_BACKGROUND,
 * COLOR_NTP_TEXT,
 * COLOR_NTP_LINK,
 * COLOR_NTP_LINK_UNDERLINE,
 * COLOR_NTP_HEADER,
 * COLOR_NTP_SECTION,
 * COLOR_NTP_SECTION_TEXT,
 * COLOR_NTP_SECTION_LINK,
 * COLOR_NTP_SECTION_LINK_UNDERLINE,
 * COLOR_NTP_SECTION_HEADER_TEXT,
 * COLOR_NTP_SECTION_HEADER_TEXT_HOVER,
 * COLOR_NTP_SECTION_HEADER_RULE,
 * COLOR_NTP_SECTION_HEADER_RULE_LIGHT,
 * COLOR_NTP_TEXT_LIGHT

It's important here to first implement the properties in the order and with names that fit with Firefox well and try to match these up with the Google equivalents above.

Since the styles of our New Tab Page are hardcoded in CSS files, they need to be changed to CSS variables first.

Updated

2 years ago
Priority: -- → P5
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1391912

Updated

a year ago
See Also: → bug 1402312
mass move of existing themes bugs to new WebExtensions: Themes component
Component: WebExtensions: Frontend → WebExtensions: Themes
(Assignee)

Comment 3

a year ago
The only properties that chrome supports that we probably want to support as well:
* colors.ntp_background
* colors.ntp_text
* colors.ntp_link

* images.theme_ntp_background
* properties.ntp_background_alignment
* properties.ntp_background_repeat

Other stuff chrome supports, but I don't think is worth supporting:
* images.theme_ntp_attribution
* colors.ntp_header
* properties.ntp_logo_alternate

Sources:
* current set of exposed properties: https://cs.chromium.org/chromium/src/chrome/browser/themes/browser_theme_pack.cc?rcl=9a5ca91778f04309cb6982697ae80a2fc89fd94b&l=249
* description of each property (contains some that have been removed since): https://sites.google.com/site/gsugsa/google-apps/google-chrome/how-to-create-a-theme
(Assignee)

Updated

a year ago
No longer depends on: 1443217

Comment 4

a year ago
mconley, are the properties in comment 0 the ones you were thinking of for "making it possible to theme Activity Stream (this would give us further Chrome parity on theme-ing APIs)", i.e., COLOR_NTP_* ?
(Assignee)

Comment 5

a year ago
Comment 3 would be more accurate. Comment 0 is taken from chrome's internal list, but doesn't reflect what's actually exposed to chrome themes.
Keywords: meta

Updated

9 months ago
Product: Toolkit → WebExtensions
Comment hidden (mozreview-request)

Comment 8

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review258854


Code analysis found 1 defect in this patch:
 - 1 defect 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


::: toolkit/modules/LightweightThemeConsumer.jsm:217
(Diff revision 1)
> +  if (!active) {
> +    return {};
> +  }
> +  let properties = {};
> +  for (let property in data) {
> +    if (ThemeContentPropertyList.includes(property)) {

Error: 'themecontentpropertylist' is not defined. [eslint: no-undef]
Comment hidden (mozreview-request)

Comment 10

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review258876


Code analysis found 1 defect in this patch:
 - 1 defect 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


::: toolkit/modules/LightweightThemeConsumer.jsm:217
(Diff revision 2)
> +  if (!active) {
> +    return {};
> +  }
> +  let properties = {};
> +  for (let property in data) {
> +    if (ThemeContentPropertyList.includes(property)) {

Error: 'themecontentpropertylist' is not defined. [eslint: no-undef]
Hey ntim, I'm going to review this but ignore all of the Activity Stream changes (those changes should probably be part of a separate patch, and merged by the Activity Stream when they do an "export").

Comment 12

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review258886

Hey ntim, thanks for the patch! In general I think this looks good. Great job! I have a few high-level suggestions, and some lower-level nits.

::: browser/base/content/contentTheme.js:41
(Diff revision 2)
> +
> +const ContentThemeController = {
> +  init() {
> +    addEventListener("LightweightTheme:Set", this);
> +
> +    var event = new CustomEvent("LightweightTheme:Support", {bubbles: true});

let, not var

::: browser/base/content/contentTheme.js:46
(Diff revision 2)
> +    switch (event.detail.type) {
> +      case "LightweightTheme:Update":

Since we only care about the one event, this can be a normal conditional.

::: browser/base/content/contentTheme.js:71
(Diff revision 2)
> +  _setProperties(root, themeData) {
> +    for (let [cssVarName, definition] of inContentVariableMap) {
> +      const {
> +        lwtProperty,
> +        processColor,
> +        isColor = true

What's the isColor for?

::: browser/base/content/tab-content.js:82
(Diff revision 2)
>  
>  addMessageListener("MixedContent:ReenableProtection", function() {
>    docShell.mixedContentChannel = null;
>  });
>  
> +var LightweightThemeListener = {

felipe has been working to reduce and / or minimize the amount of script we run in our frame scripts automatically.

I know this isn't a whole lot of code, but we should probably avoid adding things that don't need to run right away - and this thing doesn't need to run until either a a LightweightTheme:Support or LightweightTheme:Update message/event shows up.

There's this thing called a 
He has this thing called the defineLazyProxy that might be useful here. I know this isn't a whole lot of code, but we should aim for minimizing the scripts as much as possible.

Here are some examples on how to use defineLazyProxy: https://searchfox.org/mozilla-central/rev/93d2b9860b3d341258c7c5dcd4e278dea544432b/browser/base/content/content.js#36-57

This also has the benefit of splitting out the script into its own JSM, which might be useful for re-using this stuff in sidebars!

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

Are we likely to listen to more messages from the parent here in the future? If not, this can probably just be a normal conditional.

::: browser/base/content/tab-content.js:111
(Diff revision 2)
> +    switch (event.type) {
> +      case "LightweightTheme:Support":
> +        this._update("LightweightTheme:Update", this._lastData);
> +    }

Remind me again - are we likely to support more events from the controller in the future? If not, we can probably just turn this into a normal conditional.

::: browser/base/content/tab-content.js:134
(Diff revision 2)
> +                                                  theEvent));
> +  },
> +
> +  _update(msgName, msgData) {
> +    if (this._contentWhitelisted) {
> +      this._fireEvent(msgName, msgData);

Maybe let's inline \_fireEvent to here. That way, we make it so that you _must_ pass the contentWhitelisted check in order to fire an event.

::: browser/base/jar.mn:104
(Diff revision 2)
>          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)

I guess that (content/contentTheme.js) should align to stick with convention (although it's a pretty weak convention).

::: browser/base/jar.mn:116
(Diff revision 2)
>  # 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)
>  
> +

Can remove this newline, I guess.

::: toolkit/content/plugins.css:68
(Diff revision 2)
>    color: HighlightText;
>  }
>  
>  th + th,
>  td + td {
> -  border-inline-start: 1px dotted ThreeDShadow; 
> +  border-inline-start: 1px dotted ThreeDShadow;

Is this part of a separate patch or bug?

::: toolkit/modules/LightweightThemeConsumer.jsm:90
(Diff revision 2)
>    }],
>  ];
>  
>  // Get the theme variables from the app resource directory.
>  // This allows per-app variables.
>  ChromeUtils.import("resource:///modules/ThemeVariableMap.jsm");

I wonder if we should make the imports clearer with:

```js
ChromeUtils.defineModuleGetter(this, "ThemeContentPropertyList",
  "resource:///modules/ThemeVariableMap.jsm");
ChromeUtils.defineModuleGetter(this, "ThemeVariableMap.jsm",
  "resource:///modules/ThemeVariableMap.jsm");
```

I also wonder if this is a good opportunity to rename ThemeVariableMap.jsm to ThemeVariableMaps or ThemeVariables, to help indicate that it exports two things and not just the one map.

Also, I wonder if these imports should be lazy.
Attachment #8987146 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8987259 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Attachment #8987261 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 21

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review260054

::: browser/base/content/tab-content.js:85
(Diff revision 6)
>  });
>  
> +XPCOMUtils.defineLazyProxy(this, "LightweightThemeChildListener",
> +                           "resource:///modules/LightweightThemeChildListener.jsm");
> +
> +LightweightThemeChildListener.content = content;

note to self: Use Cu.getWeakReference()

Comment 22

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review260116

Thanks, ntim! This is looking great. I have a few more suggestions, but this is looking pretty far along - mostly what I'm asking for is documentation, and some minor changes to how we refer to "content".

::: browser/base/content/contentTheme.js:5
(Diff revision 6)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * 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";

Similar to the LightweightThemeContent.jsm file, since we're creating a new file, let's get it started on the right foot, and document where possible.

Can you please add docstrings for each function and the ContentThemeController class, and also add a docstring at the top describing what this file should be used for and how to use it (in case we want more pages to use contentTheme.js)

::: browser/base/content/tab-content.js:85
(Diff revision 6)
>  });
>  
> +XPCOMUtils.defineLazyProxy(this, "LightweightThemeChildListener",
> +                           "resource:///modules/LightweightThemeChildListener.jsm");
> +
> +LightweightThemeChildListener.content = content;

Is the .content property so that you have a reference to the top-level global?

That's a little problematic - JSM's are process global. That means that if another tab opens in this process, it'll swap in its top-level content, and break your checks for those other tabs that have set their content on LightweightThemeChildListener already.

Since LightweightThemChildListener will always be reacting to either events or messages, and from those two things we can usually infer the top-level global. I'll comment in that file to help illustrate.

::: browser/modules/LightweightThemeChildListener.jsm:1
(Diff revision 6)
> +var EXPORTED_SYMBOLS = ["LightweightThemeChildListener"];

Please add an MPL2 license header to this file.

::: browser/modules/LightweightThemeChildListener.jsm:3
(Diff revision 6)
> +var EXPORTED_SYMBOLS = ["LightweightThemeChildListener"];
> +
> +var LightweightThemeChildListener = {

Please add a brief docstring above this object defining what it's responsible for, and what it generally does.

::: browser/modules/LightweightThemeChildListener.jsm:4
(Diff revision 6)
> +var EXPORTED_SYMBOLS = ["LightweightThemeChildListener"];
> +
> +var LightweightThemeChildListener = {
> +  whitelist: new Set([

Also, please add docstrings for each of these properties. Look at BrowserTestUtils.jsm for an example of the doc formatting that we use.

I ask this because this is a new file, and the tendency seems to be to always follow the conventions of the file you're in. If we're adding a new file, we should do our best to ensure that the conventions are to document.

::: browser/modules/LightweightThemeChildListener.jsm:6
(Diff revision 6)
> +var EXPORTED_SYMBOLS = ["LightweightThemeChildListener"];
> +
> +var LightweightThemeChildListener = {
> +  whitelist: new Set([
> +    "about:home",
> +    "about:newtab",

We should probably add about:welcome here too.

::: browser/modules/LightweightThemeChildListener.jsm:12
(Diff revision 6)
> +  ]),
> +
> +  _lastData: null,
> +
> +  receiveMessage({ name, data }) {
> +    if (name == "LightweightTheme:Update") {

Here, I think you can get at the content global via message.target.content (you're pulling name and data out of the message argument right now - I guess pull target out too?

::: browser/modules/LightweightThemeChildListener.jsm:19
(Diff revision 6)
> +      this._update(data);
> +    }
> +  },
> +
> +  handleEvent(event) {
> +    if (event.originalTarget.defaultView != this.content) {

Instead of holding a reference to this.content, we can just check that defaultView is the top-level document's global. I think you can do that like this:

```js
let content = event.originalTarget.defaultView;
if (content != content.top) {
  return;
}
```

::: browser/modules/LightweightThemeChildListener.jsm:24
(Diff revision 6)
> +    if (event.originalTarget.defaultView != this.content) {
> +      return;
> +    }
> +
> +    if (event.type == "LightweightTheme:Support") {
> +      this._update(this._lastData);

You could then pass the content variable through here, and make \_contentWhitelisted a function instead of a getter in order to ensure that the content is pointed at the right document.
Attachment #8987146 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review260704

Hey ntim,

This looks great! Code seems solid. I just have one last structural concern - and it's my fault, I should have identified this when I suggested that you move LightweightThemeChildListener to a JSM. See below.

::: browser/modules/LightweightThemeChildListener.jsm:23
(Diff revision 12)
> +  /**
> +   * The last theme data received from LightweightThemeConsumer
> +   */
> +  _lastData: null,

I'm just realizing that we have a bit of a problem here.

The LightweightThemeChildListener, now that it's a singleton within the content process, might hold a cache of the most recent data for a tab _that might not belong to the same window_. So if we have two tabs in different windows A and B that each are in the same process, and A receives a theme update, then the cache in B will be overwritten. New tabs in the same content process in B will then start with an invalid cache.

So what might be better is to have LightweightThemeChildListener expose a class that can be instantiated per tab, and have that class hold the cache.

And then we'd probably want that class to be instantiated lazily inside tab-content.js. We did something similar for PluginContent - see https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/browser/base/content/content.js#321-381

Sorry to run you around on this - I just noticed this. :/

::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:19
(Diff revision 12)
> +    is(doc.body.hasAttribute("lwt-newtab"), false,
> +       "New tab page should have lwt-newtab attribute");
> +    is(doc.body.hasAttribute("lwt-newtab-brighttext"), false,
> +       `New tab page should not have lwt-newtab-brighttext attribute`);

Alternatively (and maybe more common):

```js
ok(!doc.body.hasAttribute("lwt-newtab"),
   "New tab page should not have lwt-newtab attribute");
```

::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:20
(Diff revision 12)
> +    originalBackground,
> +    originalColor,
> +  } = await ContentTask.spawn(browser, {}, function() {
> +    let doc = content.document;
> +    is(doc.body.hasAttribute("lwt-newtab"), false,
> +       "New tab page should have lwt-newtab attribute");

should _not_ have.

::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:39
(Diff revision 12)
> +    background: hexToCSS(theme.colors.ntp_background),
> +    color: hexToCSS(theme.colors.ntp_text),
> +  }, function({isBrightText, background, color}) {
> +    let doc = content.document;
> +    is(doc.body.hasAttribute("lwt-newtab"), true,
> +       "New tab page should not have lwt-newtab attribute");

"should have". Also, see above about using ok()

::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:69
(Diff revision 12)
> +  for (let url of ["about:newtab", "about:home", "about:welcome"]) {
> +    info("Opening url: " + url);
> +    let tab = BrowserTestUtils.addTab(gBrowser, url);
> +    gBrowser.selectedTab = tab;
> +
> +    let browser = tab.linkedBrowser;
> +    await BrowserTestUtils.browserLoaded(browser);
> +
> +    await test_ntp_theme({
> +      colors: {
> +        accentcolor: ACCENT_COLOR,
> +        textcolor: TEXT_COLOR,
> +        ntp_background: "#add8e6",
> +        ntp_text: "#000",
> +      },
> +    }, false, url);
> +
> +    await test_ntp_theme({
> +      colors: {
> +        accentcolor: ACCENT_COLOR,
> +        textcolor: TEXT_COLOR,
> +        ntp_background: "#000",
> +        ntp_text: "#add8e6",
> +      },
> +    }, true, url);
> +
> +    BrowserTestUtils.removeTab(tab);
> +  }

You can simplify this a little bit by using `BrowserTestUtils.withNewTab`, which will take care of adding the tab, loading a page, and then closing the tab for you. Example:

```js

await BrowserTestUtils.withNewTab({
  gBrowser,
  url
}, browser => {
  // At this point, you have the browser tab loaded at url
  
  // Do your test
});

// Tab is automatically closed for you.
```
Attachment #8987146 - Flags: review?(mconley) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 39

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review260822


Code analysis found 1 defect in this patch:
 - 1 defect 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:82
(Diff revision 16)
>  
>  addMessageListener("MixedContent:ReenableProtection", function() {
>    docShell.mixedContentChannel = null;
>  });
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeChildListener",

Error: Please use chromeutils.definemodulegetter instead of xpcomutils.definelazymodulegetter [eslint: mozilla/use-chromeutils-import]

Comment 40

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review260824


Code analysis found 1 defect in this patch:
 - 1 defect 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:82
(Diff revision 17)
>  
>  addMessageListener("MixedContent:ReenableProtection", function() {
>    docShell.mixedContentChannel = null;
>  });
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeChildListener",

Error: Please use chromeutils.definemodulegetter instead of xpcomutils.definelazymodulegetter [eslint: mozilla/use-chromeutils-import]

Comment 41

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review260826


Code analysis found 1 defect in this patch:
 - 1 defect 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:82
(Diff revision 18)
>  
>  addMessageListener("MixedContent:ReenableProtection", function() {
>    docShell.mixedContentChannel = null;
>  });
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "LightweightThemeChildListener",

Error: Please use chromeutils.definemodulegetter instead of xpcomutils.definelazymodulegetter [eslint: mozilla/use-chromeutils-import]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 51

9 months ago
mozreview-review
Comment on attachment 8987487 [details]
Bug 1347204 - Activity stream changes.

https://reviewboard.mozilla.org/r/252738/#review261062

This looks great and fixes the behavior issues we discussed. Thanks!
Attachment #8987487 - Flags: review?(andrei.br92) → review+

Comment 52

9 months ago
mozreview-review
Comment on attachment 8987487 [details]
Bug 1347204 - Activity stream changes.

https://reviewboard.mozilla.org/r/252738/#review261230

There is some more code that needs to be removed that I missed on my first review.
You should look for `THEME_UPDATE` references in AS code and remove those as well because they were triggered by the `ThemeFeed` which you removed.
You also need to remove the `Reducer.Theme` branch and then look for `Theme` and `state.Theme` references
Attachment #8987487 - Flags: review+ → review-

Comment 53

9 months ago
mozreview-review
Comment on attachment 8987146 [details]
Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties.

https://reviewboard.mozilla.org/r/252376/#review261418

Thanks, ntim! This looks good to me. Great work!

One thing we should start testing, though, is how per-window themes behave when doing tab tearing / dragging in. I don't think we're handling that case here. But that can probably be fixed in a follow-up.

::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:5
(Diff revision 24)
> +"use strict";
> +
> +// This test checks whether the new tab page color properties work.
> +
> +async function test_ntp_theme(theme, isBrightText) {

Please document this function.

::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:71
(Diff revision 24)
> +}
> +
> +add_task(async function test_support_ntp_colors() {
> +  // about:newtab is preloaded and BrowserTestUtils.withNewTab waits for about:newtab to load
> +  // so we unload about:newtab in order to allow this test to still run properly.
> +  gBrowser.removePreloadedBrowser();

To avoid flake-y tests, we should probably disable preloading as well as getting rid of the preloaded browser here. I'm not certain that the preloaded browser could "reload" before the next run through the loop, but it wouldn't surprise me.

browser.newtab.preload is the pref to set to false.

::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors_perwindow.js:5
(Diff revision 24)
> +"use strict";
> +
> +// This test checks whether the new tab page color properties work per-window.
> +
> +function test_ntp_theme(browser, theme, isBrightText) {

Please document this and the other function.

::: toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors_perwindow.js:153
(Diff revision 24)
> +      browser.test.notifyPass("perwindow-ntp-theme");
> +    },
> +  });
> +
> +  extension.onMessage("check-window", async ({theme, isBrightText}) => {
> +    let win = Services.wm.getMostRecentWindow("navigator:browser");

Same as in the other test - maybe disable preloading for this test.
Attachment #8987146 - Flags: review?(mconley) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

9 months ago
Depends on: 1473270

Comment 57

9 months ago
mozreview-review
Comment on attachment 8987487 [details]
Bug 1347204 - Activity stream changes.

https://reviewboard.mozilla.org/r/252738/#review261664

This looks really good, there's just one thing to fix and rebase (some changes landed in Github in the meantime).

::: browser/extensions/activity-stream/content-src/components/Base/Base.jsx
(Diff revision 8)
>  
> -  updateTheme(Theme) {
> -    const bodyClassName = [
> -      "activity-stream",
> -      Theme.className,
> -      this.props.isFirstrun ? "welcome" : ""

The function name is deceiving, it's actually also used for the about:welcome transition to Activity Stream. Only the code related to setting `Theme.className` should be removed. You should check the current Base.jsx file in github because that particular function was recently changed to fix a bug and we don't want to reintroduce it again with this landing in m-c.
You can check that the behaviour is correct by going to about:welcome, clicking `Skip this step` and have the page transition to Activity Stream.
Attachment #8987487 - Flags: review?(andrei.br92) → review-
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 61

9 months ago
mozreview-review
Comment on attachment 8987487 [details]
Bug 1347204 - Activity stream changes.

https://reviewboard.mozilla.org/r/252738/#review261998

Great! Thank you.
Attachment #8987487 - Flags: review?(andrei.br92) → review+

Comment 62

9 months ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8068cb14325a4fadfe20f00580a515ad0bcfc013 -d fda31bd96fa1: rebasing 471418:8068cb14325a "Bug 1347204 - Activity stream changes. r=andreio"
rebasing 471419:0c7571389c55 "Bug 1347204 - Implement the colors.ntp_background and colors.ntp_text properties. r=mconley" (tip)
merging browser/base/content/tab-content.js
merging browser/base/content/test/performance/browser_startup_content.js
merging browser/base/jar.mn
merging browser/components/nsBrowserGlue.js
warning: conflicts while merging browser/base/jar.mn! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 65

9 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/46922541d170
Activity stream changes. r=andreio
https://hg.mozilla.org/integration/autoland/rev/29caa8009784
Implement the colors.ntp_background and colors.ntp_text properties. r=mconley
Backed out for ESlint failure on browser_ext_themes_ntp_colors.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=29caa80097848b216f59025e77fdbc1e89ee410a&tochange=2093b8e55daf72f8a03fd420dfccbfd23078e4b1&filter-searchStr=Linting%20opt%20source-test-mozlint-eslint%20(ES)&selectedJob=186716330

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=186716330&repo=autoland&lineNumber=265

Backout link: https://hg.mozilla.org/integration/autoland/rev/2093b8e55daf72f8a03fd420dfccbfd23078e4b1

[task 2018-07-05T22:02:27.050Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-07-05T22:02:27.050Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-07-05T22:02:27.050Z] 
[task 2018-07-05T22:02:27.050Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-07-05T22:07:59.529Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:7:4 | Missing JSDoc parameter type for 'theme'. (valid-jsdoc)
[task 2018-07-05T22:07:59.529Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors.js:8:4 | Missing JSDoc parameter type for 'isBrightText'. (valid-jsdoc)
[task 2018-07-05T22:07:59.529Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors_perwindow.js:5:1 | Missing JSDoc @returns for function. (valid-jsdoc)
[task 2018-07-05T22:07:59.529Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors_perwindow.js:7:4 | Missing JSDoc parameter type for 'browser'. (valid-jsdoc)
[task 2018-07-05T22:07:59.529Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors_perwindow.js:8:4 | Missing JSDoc parameter type for 'theme'. (valid-jsdoc)
[task 2018-07-05T22:07:59.530Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors_perwindow.js:9:4 | Missing JSDoc parameter type for 'isBrightText'. (valid-jsdoc)
[task 2018-07-05T22:07:59.530Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors_perwindow.js:30:1 | Missing JSDoc @returns for function. (valid-jsdoc)
[task 2018-07-05T22:07:59.530Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/browser/browser_ext_themes_ntp_colors_perwindow.js:32:4 | Missing JSDoc parameter type for 'browser'. (valid-jsdoc)
[taskcluster 2018-07-05 22:07:59.898Z] === Task Finished ===
[taskcluster 2018-07-05 22:07:59.898Z] Unsuccessful task run with exit code: 1 completed in 588.459 seconds
Flags: needinfo?(ntim.bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 69

9 months ago
Pushed by ntim.bugs@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ca8e1213f5ed
Activity stream changes. r=andreio
https://hg.mozilla.org/integration/autoland/rev/57eb4ec50bfb
Implement the colors.ntp_background and colors.ntp_text properties. r=mconley
(Assignee)

Updated

9 months ago
Flags: needinfo?(ntim.bugs)

Comment 70

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ca8e1213f5ed
https://hg.mozilla.org/mozilla-central/rev/57eb4ec50bfb
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
(Assignee)

Updated

9 months ago
Blocks: 1473270
No longer depends on: 1473270
Depends on: 1473920
(Assignee)

Updated

9 months ago
Blocks: 1444459
No longer depends on: 1444459
(Assignee)

Updated

9 months ago
Keywords: dev-doc-needed
Depends on: 1474163

Comment 72

8 months ago
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!
Flags: needinfo?(ntim.bugs)
(Assignee)

Comment 73

8 months ago
This is covered by automated testing.
Flags: needinfo?(ntim.bugs) → qe-verify-

Comment 74

6 months ago
From what I can see, only ntp_text and ntp_background were implemented. Is that correct?

I have updated the browser compatibility for those two properties, but will create another pull request if any of the other ntp properties were implements. (i.e. ntp_link and ntp_header)

Pull request: https://github.com/mdn/browser-compat-data/pull/2818

The properties are already documented on MDN.
Flags: needinfo?(ntim.bugs)
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Comment 75

6 months ago
> From what I can see, only ntp_text and ntp_background were implemented. Is that correct?

Yes, that’s correct.

> The properties are already documented on MDN.

Is it possible to get screenshots of each of the properties being set to red similarly to the existing “See example” expanders?

Thanks!
Flags: needinfo?(ntim.bugs) → needinfo?(ismith)

Comment 76

6 months ago
(In reply to Tim Nguyen :ntim from comment #75)
> > From what I can see, only ntp_text and ntp_background were implemented. Is that correct?
> 
> Yes, that’s correct.
> 
> > The properties are already documented on MDN.
> 
> Is it possible to get screenshots of each of the properties being set to red
> similarly to the existing “See example” expanders?
> 
> Thanks!

Done! Since the colors are related, I used the same image for both. The image shows white text on a red background.
Flags: needinfo?(ismith)
You need to log in before you can comment on or make changes to this bug.