Mike de Boer [:mikedeboer] Reporter
	Description • 7 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.

	Andrew Swan [:aswan]
	Comment 2 • 6 years ago

mass move of existing themes bugs to new WebExtensions: Themes component

	Tim Nguyen :ntim Assignee
	Comment 3 • 6 years 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

	Ed Lee :Mardak
	Comment 4 • 6 years 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_* ?

	Tim Nguyen :ntim Assignee
	Comment 5 • 6 years 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.

	Code Review Bot [:reviewbot]
	Comment 8 • 6 years 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]

	Code Review Bot [:reviewbot]
	Comment 10 • 6 years 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]

	Mike Conley (:mconley) (:⚙️)
	Comment 11 • 6 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 12 • 6 years 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.

	Tim Nguyen :ntim Assignee
	Comment 21 • 6 years 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()

	Mike Conley (:mconley) (:⚙️)
	Comment 22 • 6 years 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.

	Mike Conley (:mconley) (:⚙️)
	Comment 31 • 6 years 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.
```

	Code Review Bot [:reviewbot]
	Comment 39 • 6 years 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]

	Code Review Bot [:reviewbot]
	Comment 40 • 6 years 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]

	Code Review Bot [:reviewbot]
	Comment 41 • 6 years 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]

	Andrei Oprea [:andreio]
	Comment 51 • 6 years 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!

	Andrei Oprea [:andreio]
	Comment 52 • 6 years 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

	Mike Conley (:mconley) (:⚙️)
	Comment 53 • 6 years 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.

	Andrei Oprea [:andreio]
	Comment 57 • 6 years 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.

	Andrei Oprea [:andreio]
	Comment 61 • 6 years ago
mozreview-review

Comment on attachment 8987487 [details]
Bug 1347204 - Activity stream changes.

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

Great! Thank you.

	Mozilla Autoland
	Comment 62 • 6 years 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)

	Pulsebot
	Comment 65 • 6 years 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

	Cosmin Sabou [:CosminS]
	Comment 66 • 6 years ago

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

	Pulsebot
	Comment 69 • 6 years 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

	Eliza Balazs [:ebalazs_]
	Comment 70 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/ca8e1213f5ed
https://hg.mozilla.org/mozilla-central/rev/57eb4ec50bfb

	[github robot]
	Comment 71 • 6 years ago

Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/307d9ccad8e139accd8904baaf761e4d4f5deb89
Port Bug 1347204 - Implement Google Chrome New Tab Page color properties

	Vlad Jiman
	Comment 72 • 6 years 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!

	Tim Nguyen :ntim Assignee
	Comment 73 • 6 years ago

This is covered by automated testing.

	Irene Smith
	Comment 74 • 6 years 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.

	Tim Nguyen :ntim Assignee
	Comment 75 • 6 years 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!

	Irene Smith
	Comment 76 • 6 years 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.