1316630 - remove pref-changed from gDevTools

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1310417#c3:

> We should switch this to use a normal pref observer (or pull away a lighter weight helper like styleeditor/utils.js into a shared folder and use that).  We should actually completely get rid of the pref-changed event on gDevTools I believe - it's buggy in that it only fires when the pref is changed from the options panel.

Comment 1

[Tom Tromey :tromey]

I looked a bit into merging PrefsHelper and PrefObserver, but they are different
enough that changing the uses of one into the other uglifies the code somewhat.
So I'm just planning to move PrefObserver out of StyleEditor into shared code and
leave both in place.

Comment 2

[Tom Tromey :tromey]

There's also an implementation in devtools/client/shared/options-view.js that I
probably won't merge.

Comment 3

[Tom Tromey :tromey]

Attachment: Bug 1316630 - move PrefObserver to devtools/shared/prefs.js;

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

Comment 4

[Tom Tromey :tromey]

Attachment: Bug 1316630 - don't emit pref-changed event on gDevTools;

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

Comment 5

[Julian Descottes [:jdescottes]]

Comment on attachment 8813170 [details]
Bug 1316630 - don't emit pref-changed event on gDevTools;

https://reviewboard.mozilla.org/r/94702/#review95228

r+ assuming try is green.
A bit worried about that after looking at browser_webconsole_expandable_timestamps.js, but let's see how it goes :)

::: devtools/client/framework/test/browser_toolbox_options.js:162
(Diff revision 1)
>      let changeEvent = new Event("change");
>      select.dispatchEvent(changeEvent);
>  
>      yield deferred.promise;
> +
> +    ok(changeSeen, "Correct pref was changed");

Isn't changeSeen redundant since we yield on a promise that resolves only if the expected pref is updated? 

We can leave it here though, doesn't hurt either.

::: devtools/client/shared/theme.js:72
(Diff revision 1)
> - * Mimics selecting the theme selector in the toolbox;
> + * Set the theme preference.
> - * sets the preference and emits an event on gDevTools to trigger
> - * the themeing.
>   */
>  const setTheme = exports.setTheme = (newTheme) => {
>    let oldTheme = getTheme();

delete unused oldTheme variable.

::: devtools/client/webconsole/test/browser_webconsole_expandable_timestamps.js:24
(Diff revision 1)
>    yield loadTab(TEST_URI);
>  
>    hud = yield openConsole();
>    let panel = yield consoleOpened();
>  
> -  yield onOptionsPanelSelected(panel);
> +  let observer = new PrefObserver("devtools.");

new PrefObserver("devtools."); should be new PrefObserver("");

::: devtools/client/webconsole/test/browser_webconsole_expandable_timestamps.js:47
(Diff revision 1)
>  }
>  
> -function onOptionsPanelSelected(panel) {
> +function onOptionsPanelSelected(panel, observer) {
>    info("options panel opened");
>  
> -  let prefChanged = gDevTools.once("pref-changed", onPrefChanged);
> +  let prefChanged = observer.once("pref-changed", onPrefChanged);

Two things:

1. I think we should remove the onPrefChanged callback here. It gets called before the actual observer attached in console-output.js and fails.

I guess it was just left by mistake before, just didn't happen to fail.

2. Replace "pref-changed" by PREF_MESSAGE_TIMESTAMP

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8813169 [details]
Bug 1316630 - move PrefObserver to devtools/shared/prefs.js;

https://reviewboard.mozilla.org/r/94700/#review95238

Looks good, thanks for the cleanup Tom!

::: devtools/client/styleeditor/utils.js:9
(Diff revision 1)
>  "use strict";
>  
> -const Services = require("Services");
> -const EventEmitter = require("devtools/shared/event-emitter");
> -
>  exports.PREF_ORIG_SOURCES = "devtools.styleeditor.source-maps-enabled";

Shouldn't we remove this file? Not sure it's worth having a utils.js that only contains a string.

Comment 7

[Tom Tromey :tromey]

Comment on attachment 8813170 [details]
Bug 1316630 - don't emit pref-changed event on gDevTools;

https://reviewboard.mozilla.org/r/94702/#review95228

> delete unused oldTheme variable.

It's peculiar that eslint didn't flag this.

Comment 8

[Tom Tromey :tromey]

Comment on attachment 8813169 [details]
Bug 1316630 - move PrefObserver to devtools/shared/prefs.js;

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

Comment 9

[Tom Tromey :tromey]

Comment on attachment 8813170 [details]
Bug 1316630 - don't emit pref-changed event on gDevTools;

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

Comment 10

[Tom Tromey :tromey]

Comment on attachment 8813169 [details]
Bug 1316630 - move PrefObserver to devtools/shared/prefs.js;

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

Comment 11

[Tom Tromey :tromey]

Comment on attachment 8813170 [details]
Bug 1316630 - don't emit pref-changed event on gDevTools;

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

Comment 12

[Tom Tromey :tromey]

Comment on attachment 8813169 [details]
Bug 1316630 - move PrefObserver to devtools/shared/prefs.js;

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

Comment 13

[Tom Tromey :tromey]

Comment on attachment 8813170 [details]
Bug 1316630 - don't emit pref-changed event on gDevTools;

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5a083b4e11bf
move PrefObserver to devtools/shared/prefs.js; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/3a4bb81e6ef5
don't emit pref-changed event on gDevTools; r=jdescottes

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5a083b4e11bf
https://hg.mozilla.org/mozilla-central/rev/3a4bb81e6ef5