Via @SaraSoueidan: https://twitter.com/SaraSoueidan/status/910567472604024832 STR: - Inspect Grid - Change color for a grid - Don't check the grid to be shown - Reload ER: Changed color should be restored AR: Color is reset to default This STR works as expected when the grid is actually checked to be visible; but not when the grid is unchecked.

Yes, the changes a user makes to a color should always save state — always remain changed.

We really need to fix this bug.

Hiya, I am currently looking for bugs to fix as part of my Open Source Development module at Coventry University and I am interested in developing this bug. Please could you assign this task to me and give me more information. This is my first bug fix and any help would be appreciated. Thank you.

Hi Rayann, Take a look at https://bugzilla.mozilla.org/show_bug.cgi?id=1342310 which implemented the persistence of the grid highlighter on reload. You will mainly be working in https://searchfox.org/mozilla-central/source/devtools/client/inspector/shared/highlighters-overlay.js#48. I would store the state of all grids in the page and restore if the corresponding grid node is still on the page after reload. You can see that we currently restore the highlighter checked state. It should be similar for restoring the color. My current recommended setup is here - https://github.com/gabrielluong/devtools-getting-started. Thanks, Gabriel

Hi Rayann, Just wanted to see how you have been doing on this bug. There's no pressure if you haven't gotten started yet. Just wanted to see if there's anyway I can help. Thanks, Gabriel

Gonna assign this to myself since I haven't heard back.

After discussion with :jensimmons, We'd like be able to save a palette per domain. In essence, on one website, the user picks a custom color, or a few, and those settings stay forever for that domain. On a new website, it resets to default.

Comment on attachment 8951637 [details] Bug 1401847 - Custom layout grid colors should be restored per domain. https://reviewboard.mozilla.org/r/220926/#review226864 I think we should start adding unit tests ::: devtools/client/inspector/grids/grid-inspector.js:157 (Diff revision 1) > * @param {String} fallbackColor > * The color to use if no color could be found for the node front. > * @return {String} color > * The color to use. > */ > - getInitialGridColor(nodeFront, fallbackColor) { > + getInitialGridColor(nodeFront, customColor, fallbackColor) { Update the JSDoc with the customColor param ::: devtools/client/inspector/grids/grid-inspector.js:294 (Diff revision 1) > async updateGridPanel() { > // Stop refreshing if the inspector or store is already destroyed. > if (!this.inspector || !this.store) { > return; > } > + let hostname = extractHostname(this.inspector.target.url); Add a new line before this. ::: devtools/client/inspector/grids/grid-inspector.js:332 (Diff revision 1) > // closing. > return; > } > } > > + let customColors = await asyncStorage.getItem("devtoolsGridColors"); We should put this with the hostname declaration ::: devtools/client/inspector/grids/grid-inspector.js:332 (Diff revision 1) > // closing. > return; > } > } > > + let customColors = await asyncStorage.getItem("devtoolsGridColors"); Looking at our async storage usage, I think we can drop the devtools prefix and maybe use gridInspectorHostColors as the key instead. ::: devtools/client/inspector/grids/grid-inspector.js:468 (Diff revision 1) > */ > - onSetGridOverlayColor(node, color) { > + async onSetGridOverlayColor(node, color) { > this.store.dispatch(updateGridColor(node, color)); > let { grids } = this.store.getState(); > + let hostname = extractHostname(this.inspector.target.url); > + let customGridColors = await asyncStorage.getItem("devtoolsGridColors"); I am wondering if we could do await blah() || {}; ::: devtools/client/inspector/grids/grid-inspector.js:483 (Diff revision 1) > + } > > - // If the grid for which the color was updated currently has a highlighter, update > + // If the grid for which the color was updated currently has a highlighter, update > - // the color. > + // the color. > - for (let grid of grids) { > if (grid.nodeFront === node && grid.highlighted) { This can actually go into the previous if statement and be simplified to if (grid.highlighted) { }

To clarify, the request was for all pages on a site to have the same palette. However subdomains are considered their own site, and have their own palette. `this.example.com` will have a different palette than `example.com`. But `example.com/users/` will have the same palette as `example.com/posts/` Also, the way this is written, all data urls result in a hostname of "data", so they will also have the same palette.

Comment on attachment 8951637 [details] Bug 1401847 - Custom layout grid colors should be restored per domain. https://reviewboard.mozilla.org/r/220926/#review228966 Looks good and works well, thanks for the patch Erica! I would like to see another version without the new url helper and with a more complete test. ::: devtools/client/inspector/grids/grid-inspector.js:297 (Diff revision 2) > // Stop refreshing if the inspector or store is already destroyed. > if (!this.inspector || !this.store) { > return; > } > > + let hostname = extractHostname(this.inspector.target.url); You should be able to use parseURL from source-utils here: https://searchfox.org/mozilla-central/source/devtools/client/shared/source-utils.js#52 let { hostname } = parseURL(this.inspector.target.url); The only issue is that the hostname for data:// urls is an empty string when using parseURL. So you would either need to adapt your test or to fallback on the protocol if hostname is empty. ::: devtools/client/inspector/grids/grid-inspector.js:336 (Diff revision 2) > // closing. > return; > } > } > > + let colorsForHost = customColors[hostname] ? customColors[hostname][i] : undefined; nit: singular? we are passing a single color string, colorsForHost -> customColor (to avoid colorForHostAndForIndex :) ) ::: devtools/client/inspector/grids/test/browser_grids_persist-color-palette.js:23 (Diff revision 2) > + </div> > +`; > + > +add_task(function* () { > + // "data" ends up as the hostname of the test_uri when written as a data url > + yield asyncStorage.setItem("gridInspectorHostColors", {"data": ["#333000"]}); This test checks that we restore colors from storage when opening the panel. We should also cover: - persisting color updates (open page, change a grid color, close and reopen inspector, check the color is still correct) - different domains use different colors For the first one I think we should expand your test. Look at browser_grids_grid-list-color-picker-on-RETURN.js for setting the color. And at browser_grids_accordion-state.js for closing/reopening the toolbox. This way we can avoid relying on asyncStorage.setItem(...), which links the test too much with the current implementation. For the second one we could use a page served on the test server that responds to http://example.com and another served on a subdomain, but that can be done in a followup bug. ::: devtools/client/inspector/grids/test/head.js:45 (Diff revision 2) > info("Applying the change"); > spectrum.updateUI(); > spectrum.onChange(); > }); > + > +registerCleanupFunction(function* () { nit use async rather than function* ::: devtools/client/inspector/grids/utils/utils.js:52 (Diff revision 2) > +function extractHostname(url) { > + let hostname; > + > + if (url.indexOf("://") > -1) { > + hostname = url.split("/")[2]; > + } else { > + hostname = url.split("/")[0]; > + } > + > + hostname = hostname.split(":")[0]; > + hostname = hostname.split("?")[0]; > + return hostname; > +} > + You should be able to remove this if you use parseURL as suggested above.

Comment on attachment 8951637 [details] Bug 1401847 - Custom layout grid colors should be restored per domain. https://reviewboard.mozilla.org/r/220926/#review228966 > nit use async rather than function* Do you know the syntax for that? I can't seem to find examples. `registerCleanupFunction(async function () {...}` fails.

Sure have a look at https://searchfox.org/mozilla-central/search?q=registerCleanupFunction(async&case=false&regexp=false&path=devtools

Comment on attachment 8951637 [details] Bug 1401847 - Custom layout grid colors should be restored per domain. https://reviewboard.mozilla.org/r/220926/#review228966 > Do you know the syntax for that? I can't seem to find examples. `registerCleanupFunction(async function () {...}` fails. :julian never mind, I figured it out, it needed an `await` instead of a `yield`. Can you explain why you prefer `async` over `function*`? And would you like the `add_task` functions to use `await/async` too? All the other tests save one are written using `function*` - are there plans to change these?

Comment on attachment 8951637 [details] Bug 1401847 - Custom layout grid colors should be restored per domain. https://reviewboard.mozilla.org/r/220926/#review230742 ::: devtools/client/inspector/grids/test/head.js:45 (Diff revision 2) > info("Applying the change"); > spectrum.updateUI(); > spectrum.onChange(); > }); > + > +registerCleanupFunction(function* () { In general we should avoid adding new code that uses function* / yield if we can. We are moving our code to use async/await instead of generator functions + yield. function*/yield are in general used in conjunction with Task.jsm which is a custom piece of code to emulate async/await. async/await is a native language feature, so it makes sense to try to migrate to this now that it is available to us. There have been a few bugs logged already to migrate part of the code such as https://bugzilla.mozilla.org/show_bug.cgi?id=1437828. Of course this will take time so not all the code under migration yet. Good point about the add_task in the test, it would be great to use async/await there too.

Comment on attachment 8951637 [details] Bug 1401847 - Custom layout grid colors should be restored per domain. https://reviewboard.mozilla.org/r/220926/#review230990 The patch looks good, thanks Erica! Unfortunately it makes grid tests fail intermittently. We need to fix the test helpers to wait for the grid panel to be fully initialized. ::: devtools/client/inspector/grids/grid-inspector.js:301 (Diff revision 3) > } > + let currentUrl = this.inspector.target.url; > + // Get the hostname, if there is no hostname, fall back on protocol > + // ex: `data:` uri, and `about:` pages > + let hostname = parseURL(currentUrl).hostname || parseURL(currentUrl).protocol; > + let customColors = await asyncStorage.getItem("gridInspectorHostColors") || {}; Unfortunately, the patchs add more asynchronous work in this method, which makes several grid tests fail intermittently. To fix is we need to wait for "updateGridPanel" to be completely finished before starting the tests. We already do something similar for the BoxModel at https://searchfox.org/mozilla-central/rev/d2b4b40901c15614fad2fa34718eea428774306e/devtools/client/inspector/test/shared-head.js#56-58 . The goal here would be to fire an event at the end of updateGridPanel (this.inspector.emit("grid-panel-updated") for instance) and to listen for this event in the helper linked above. You can use `./mach test browser_grids_persist-color-palette.js --headless --verify` in order to run a single test a lot of times. It usually helps detecting intermittent issues such as this one. This architectural issue existed before your patch, but the async work was probably too fast too make the tests fail. ::: devtools/client/inspector/grids/grid-inspector.js:480 (Diff revision 3) > + // ex: `data:` uri, and `about:` pages > + let hostname = parseURL(currentUrl).hostname || parseURL(currentUrl).protocol; > + let customGridColors = await asyncStorage.getItem("gridInspectorHostColors") || {}; > + > + for (let grid of grids) { > + // Update the custom color for the grid in this position. nit: we could move this comment above line 482 or 485

Comment on attachment 8951637 [details] Bug 1401847 - Custom layout grid colors should be restored per domain. https://reviewboard.mozilla.org/r/220926/#review231056 Looks good Erica, thanks! One last nit to address, then let's push to try and lang if try is green. ::: devtools/client/inspector/test/shared-head.js:59 (Diff revision 4) > if (id === "computedview" || id === "layoutview") { > // The layout and computed views should wait until the box-model widget is ready. > let onBoxModelViewReady = inspector.once("boxmodel-view-updated"); > + // The layout view also needs to wait for the grid panel to be fully updated. > + let onGridPanelReady = id === "layoutview" ? > + inspector.once("grid-panel-updated") : Promise.resolve(); nit: could we indent with 2 additional spaces here?

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/6e462b0ef169 Custom layout grid colors should be restored per domain. r=jdescottes