remove pref-changed from gDevTools

RESOLVED FIXED in Firefox 53

Status

enhancement
P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: tromey, Assigned: tromey)

Tracking

unspecified
Firefox 53
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [devtools-html])

Attachments

(2 attachments)

Assignee

Description

3 years ago
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.
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [devtools-html]
Assignee

Updated

3 years ago
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 52.3 - Nov 14
Priority: P2 → P1
Flags: qe-verify? → qe-verify-
Assignee

Comment 1

3 years ago
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.
Assignee

Comment 2

3 years ago
There's also an implementation in devtools/client/shared/options-view.js that I
probably won't merge.
Iteration: 52.3 - Nov 14 → 53.1 - Nov 28
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

3 years ago
mozreview-review
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
Attachment #8813170 - Flags: review?(jdescottes) → review+

Comment 6

3 years ago
mozreview-review
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.
Attachment #8813169 - Flags: review?(jdescottes) → review+
Assignee

Comment 7

3 years ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Iteration: 53.1 - Nov 28 → 53.2 - Dec 12
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

3 years ago
Pushed by ttromey@mozilla.com:
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

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5a083b4e11bf
https://hg.mozilla.org/mozilla-central/rev/3a4bb81e6ef5
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

a year ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.