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.

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

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

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

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.

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

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

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

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

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

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.

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.

Comment on attachment 8987487 [details] Bug 1347204 - Activity stream changes. https://reviewboard.mozilla.org/r/252738/#review261998 Great! Thank you.

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)

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

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

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

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

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!

This is covered by automated testing.

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.

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

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