Closed Bug 1316630 Opened 8 years ago Closed 8 years ago

remove pref-changed from gDevTools

Categories

(DevTools :: Framework, enhancement, P1)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Iteration:
53.2 - Dec 12
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

Details

(Whiteboard: [devtools-html])

Attachments

(2 files)

Bug 1316630 - move PrefObserver to devtools/shared/prefs.js;
58 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1316630 - don't emit pref-changed event on gDevTools;
58 bytes, text/x-review-board-request
jdescottes
: review+
Details 
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: nobody → ttromey
Status: NEW → ASSIGNED
Iteration: --- → 52.3 - Nov 14
Priority: P2 → P1
Flags: qe-verify? → qe-verify-
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.
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 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 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+
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.
Iteration: 53.1 - Nov 28 → 53.2 - Dec 12
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
https://hg.mozilla.org/mozilla-central/rev/5a083b4e11bf
https://hg.mozilla.org/mozilla-central/rev/3a4bb81e6ef5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: