We have a few graphs rendered in canvas, so we are currently hardcoding the colors. With adding the ability to toggle how these graphs get rendered based off of themes, we should have a module that manages access and has the colors listed in one place.

Comment on attachment 8535767 [details] [diff] [review] 1110952-devtools-color-module.patch Review of attachment 8535767 [details] [diff] [review]: ----------------------------------------------------------------- Good idea - I'd like to not have these values duplicated, so I have a few ideas on how we could avoid that and use the CSS as the single place of truth ::: browser/devtools/shared/theme.js @@ +9,5 @@ > + > + > +/** > + * Colors for themes taken from: > + * https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors We actually aren't fully using all of these colors yet (bug 947242). In order to make sure that we are always in sync, and to prevent duplicated colors, I'd prefer that we somehow access the CSS to get a single place of truth on this data. 1) In a document with the stylesheet loaded, apply the variable to a dummy element and return the computed style like this: http://jsfiddle.net/bgrins/r46u6x6h/. 2) Use the CSSOM to read the variables in the first block of light / dark theme: http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/light-theme.css#9 3) Fetch the CSS text and either parse it or use some rough regex matching to get the values I'd lean toward 1 or 3 personally @@ +99,5 @@ > + * Mimics selecting the theme selector in the toolbox; > + * sets the preference and emits an event on gDevTools to trigger > + * the themeing. > + */ > +const setTheme = exports.setTheme = (newTheme) => { Is this something that will be used by one of the blocked bugs? If not, we could probably skip implementing it here and instead just wait until it's needed

Comment on attachment 8535767 [details] [diff] [review] 1110952-devtools-color-module.patch Review of attachment 8535767 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/theme.js @@ +9,5 @@ > + > + > +/** > + * Colors for themes taken from: > + * https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors Hm ok will check out these solutions, good idea @@ +99,5 @@ > + * Mimics selecting the theme selector in the toolbox; > + * sets the preference and emits an event on gDevTools to trigger > + * the themeing. > + */ > +const setTheme = exports.setTheme = (newTheme) => { `setTheme` is used in a few tests that require repainting a graph to test that something has changed (timeline, performance, webaudio), so having this in one place will reduce extraneous `head.js` code

> @@ +99,5 @@ > > + * Mimics selecting the theme selector in the toolbox; > > + * sets the preference and emits an event on gDevTools to trigger > > + * the themeing. > > + */ > > +const setTheme = exports.setTheme = (newTheme) => { > > `setTheme` is used in a few tests that require repainting a graph to test > that something has changed (timeline, performance, webaudio), so having this > in one place will reduce extraneous `head.js` code We should probably just change how this works from the gDevTools / toolbox level. Right now the theme-switching.js file waits for a gDevTools notification (that only gets fired from the options panel): http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/theme-switching.js#115. This has the disadvantage of: 1) Extra boilerplate for stuff like this 2) The toolbox doesn't actually switch themes when the pref is changed through about:config I'd suggest that we add a normal pref listener in gDevTools for the theme that emits the pref-changed handler on itself, then modify the options panel entry to not emit the event so that we don't emit twice. Then all we would need to do for tests is set the pref. Not positive if this refactor would break existing tests that rely on the buggy behavior in 2, but it would make this more straightforward.

Really, I think we should have a branch listener for "browser.devtools.*" in gDevTools that does all of this and get rid of any emitting from the options panel so that all prefs will be better handled. I'm not sure that we will want to try to tackle all of that here, though.

I agree that the pref stuff currently in toolbox is a bit of a mess, but I don't want to refactor all the listeners for the toolbox options just to share the theme colors to our canvas graphs. Should open another bug for that I think (which will have several components I'd imagine)

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #6) > I agree that the pref stuff currently in toolbox is a bit of a mess, but I > don't want to refactor all the listeners for the toolbox options just to > share the theme colors to our canvas graphs. Should open another bug for > that I think (which will have several components I'd imagine) OK, that's fine - will you please file a bug for this?

Now parses the theme variables from the appropriate CSS files. Is there any reason that these aren't all updated to what's on that MDN page?

Created bug 1111047 for theme switching improvement

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #8) > Created attachment 8535870 [details] [diff] [review] > 1110952-devtools-color-module.patch > > Now parses the theme variables from the appropriate CSS files. Is there any > reason that these aren't all updated to what's on that MDN page? Not really - we got stuck on disagreements about which colors should be used for what in the themes. I think just swapping to the new colors caused things to look bad in the markup view and other places. There is a WIP on Bug 947242 that may be worth revisiting at this point now that the other variables have landed. If you are interested, then by all means :)

Comment on attachment 8535870 [details] [diff] [review] 1110952-devtools-color-module.patch Review of attachment 8535870 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/test/browser_theme.js @@ +41,5 @@ > + is(Services.prefs.getCharPref("devtools.theme"), "dark", "setTheme() correctly sets dark theme."); > + setTheme("light"); > + is(Services.prefs.getCharPref("devtools.theme"), "light", "setTheme() correctly sets light theme."); > + setTheme("unknown"); > + is(Services.prefs.getCharPref("devtools.theme"), "unknown", "setTheme() correctly sets an unknown theme."); nit : 2 spaces between "an" "unknown"

Reupped patch with ntim's comment

Comment on attachment 8536209 [details] [diff] [review] 1110952-devtools-color-module.patch Review of attachment 8536209 [details] [diff] [review]: ----------------------------------------------------------------- r+ on the API and test. I have a suggestion on the implementation that I think will make it simpler - let me know if it makes sense ::: browser/devtools/shared/theme.js @@ +83,5 @@ > + themeName = "light"; > + } > + > + // Use the cached theme, or generate it and save it in the cache. > + let themeData = cachedThemes[themeName] = cachedThemes[themeName] || parseTheme(themeURIs[themeName]); I think we could get rid of some code if we take a slightly different approach here. Instead of having the parseTheme function and returning themeData[type], we could do something similar to this: let themeFile = cachedThemes[themeName] = cachedThemes[themeName] || readURI(themeURIs[themeName]); let matches = themeFile.match(new RegExp("--theme-" + type + ": (.*);")); if (!matches) { console.error("Could not find match for " + type + " in " + themeURIs[themeName]); } return matches && matches[1]; Unless if we do full-fledged parsing, nothing is going to be perfect (both solutions could get screwed up with commented out lines or weird formatting), but since this is on such a small part of the CSS and we have test coverage to make sure that each variables exists in both themes I think this will be fine.

That could work, I just wanted to avoid storing a (possibly large) CSS file as a string in memory. I can go either way though

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=433c3f47a10b

Comment on attachment 8536209 [details] [diff] [review] 1110952-devtools-color-module.patch Review of attachment 8536209 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/shared/theme.js @@ +69,5 @@ > + */ > +const getTheme = exports.getTheme = () => Services.prefs.getCharPref("devtools.theme"); > + > +/** > + * Returns a color indicated by `type` (like "tabtoolbar", or "red"), Please update the example variables in this comment to use some actual ones in the css file

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #14) > That could work, I just wanted to avoid storing a (possibly large) CSS file > as a string in memory. I can go either way though Good point. We could just store the relevant lines using something like: let themeFile = cachedThemes[themeName] = cachedThemes[themeName] || readURI(themeURIs[themeName]).match(/--theme-.*: .*;/g).join("\n"); let matches = themeFile.match(new RegExp("--theme-" + type + ": (.*);")); if (!matches) { console.error("Could not find match for " + type + " in " + themeURIs[themeName]); } return matches && matches[1];

with bgrins revisions and comments https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1a596d8b3e13