1435092 - Keep a different set of prefs for the browser console and the web console filter state

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Brian Grinstead [:bgrins]]

We currently keep separate prefs for the two UIs so that if you change a filter in one it doesn't affect the other. This is done by having a copy of each pref, but prefixed with 'browserconsole' instead of 'webconsole': https://dxr.mozilla.org/mozilla-central/rev/7b46ef2ae1412b15ed45e7d2367ca491344729f7/devtools/client/preferences/devtools.js#262.

Comment 1

[Brian Grinstead [:bgrins]]

For the new frontend we currently keep the filter prefs names in constants.js (https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/new-console-output/constants.js#31).

I'm not sure where the right place to inject the variable pref names would be (see callers here: https://dxr.mozilla.org/mozilla-central/search?q=PREFS.FILTER&redirect=true). We could keep another set of constants for the browser console and then have all the callers check hud.isBrowserConsole before accessing the pref value, or have some way to inject prefs into the calling code rather than having them access constants directly. Or, something else I'm not thinking of. Nicolas, any thoughts?

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Ideally, I'd like us to use the same constants in the callers code, no matter if we are in the webconsole or the browserconsole.
Could we imagine something like a pref prefix that would reflect whether or not we are in the browser console ?
All the webconsole pref start with "devtools.webconsole.", so we could have a 

const PREF_PREFIX = isBrowserConsole() ? "devtools.browserconsole" : "devtools.webconsole.";
const prefs = {
  PREFS: {
    FILTER: {
      ERROR: `${PREF_PREFIX}filter.error`,
      WARN: `${PREF_PREFIX}filter.warn`,
      …

What do you think ?

Comment 3

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1435092 - Add a util object to manage preferences; .

This allow us to manage different sets of preferences for
the console and the browser console. The util function is
passed to the actions via the thunk middleware.

Review commit: https://reviewboard.mozilla.org/r/220080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220080/

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1435092 - Fix mocha tests broken due to changes to prefs; .

The mocha test are broken because we mock Services.pref,
and the assumptions we make in the mock are no longer valid.
Since we do have a fully functional replacement for Services
in devtools-modules, let's use it in our test.
Which means we need to introduce a mock for localStorage,
since it's what the devtools-modules Services uses, and helper
functions to replace what was in the Services mock.

Review commit: https://reviewboard.mozilla.org/r/220228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/220228/

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950846 [details]
Bug 1435092 - Add a util object to manage preferences; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220080/diff/1-2/

Comment 6

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950968 [details]
Bug 1435092 - Fix mocha tests broken due to changes to prefs; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220228/diff/1-2/

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8950846 [details]
Bug 1435092 - Add a util object to manage preferences; .

https://reviewboard.mozilla.org/r/220080/#review226108

::: devtools/client/webconsole/new-console-output/store.js:82
(Diff revision 2)
>        enableNetProvider(hud)
>      )
>    );
>  }
>  
> +function customThunkMiddleware(options = {}, { dispatch, getState }) {

We are the only consumer of the shared 'thunk' module. We should either delete the module or modify it to suit our needs and continue to use it. Given the size of the module I'd suggest to delete it and then rename the function defined here from `customThunkMiddleware` to just `thunk`

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8950968 [details]
Bug 1435092 - Fix mocha tests broken due to changes to prefs; .

https://reviewboard.mozilla.org/r/220228/#review226112

Makes sense to unify with devtools-modules Services shim

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950846 [details]
Bug 1435092 - Add a util object to manage preferences; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220080/diff/2-3/

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950968 [details]
Bug 1435092 - Fix mocha tests broken due to changes to prefs; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220228/diff/2-3/

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950846 [details]
Bug 1435092 - Add a util object to manage preferences; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220080/diff/3-4/

Comment 12

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950968 [details]
Bug 1435092 - Fix mocha tests broken due to changes to prefs; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220228/diff/3-4/

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950968 [details]
Bug 1435092 - Fix mocha tests broken due to changes to prefs; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220228/diff/4-5/

Comment 14

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950846 [details]
Bug 1435092 - Add a util object to manage preferences; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220080/diff/4-5/

Comment 15

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950968 [details]
Bug 1435092 - Fix mocha tests broken due to changes to prefs; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220228/diff/5-6/

Comment 16

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950846 [details]
Bug 1435092 - Add a util object to manage preferences; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220080/diff/5-6/

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8950968 [details]
Bug 1435092 - Fix mocha tests broken due to changes to prefs; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/220228/diff/6-7/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0460e52172b
Add a util object to manage preferences; r=bgrins.
https://hg.mozilla.org/integration/autoland/rev/804713551b70
Fix mocha tests broken due to changes to prefs; r=bgrins.

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b0460e52172b
https://hg.mozilla.org/mozilla-central/rev/804713551b70