Closed Bug 1371849 Opened 7 years ago Closed 7 years ago

move source maps pref to "common" section of options panel

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: tromey, Assigned: tromey)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Bug 1371849 - remove style editor's source-map pref;
59 bytes, text/x-review-board-request
gl
: review+
Details
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;
59 bytes, text/x-review-board-request
bgrins
: review+
Details 
The toggle for the pref controlling source maps should move to the "common"
part of the options panel.  Currently there's one for the debugger (which
mentions the console in the tooltip) and one for the style editor.
I have a patch for this, but maybe this shouldn't really be done until the debugger is
updated to use the unified pref.
Assignee: nobody → ttromey
Depends on: 1371848
It might be a bit weird but I am also going to look into temporarily having
the inspector and style editor use the "client side" pref, even though they
don't use the client-side service yet.  I don't think it matters much what
the pref is named; and it's nicer to have a single setting.
The common area was removed in bug 1307881, but I'm going to resurrect it.
Comment on attachment 8908688 [details]
Bug 1371849 - remove style editor's source-map pref;

https://reviewboard.mozilla.org/r/180328/#review185490
Attachment #8908688 - Flags: review?(gl) → review+
We had a bit of a dependency clash here, which we're working around in https://github.com/devtools-html/debugger.html/pull/4039
Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

https://reviewboard.mozilla.org/r/180330/#review185544

::: devtools/client/framework/toolbox-options.xhtml:89
(Diff revision 1)
>                   data-pref="devtools.webconsole.timestampMessages"/>
>            <span>&options.timestampMessages.label;</span>
>          </label>
>        </fieldset>
>  
> -      <fieldset id="debugger-options" class="options-groupbox">
> +      <fieldset id="debugger-options" class="options-groupbox" style="display: none">

I guess we are setting display:none because of the dynamic pref to toggle between old and new debugger frontend.

Two options: 

either we keep this nightly-only option (I think its handy for testing, but less and less relevant)
In this case we should force the display to visible when we add the pref, ie in http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/devtools/client/framework/toolbox-options.js#307-351

we get rid of the pref and of the debugger fieldset here?

(maybe you kept it for another reason, feel free to ignore my comment in this case :) )
Tom: if you plan to cleanup devtools/client/framework/toolbox-process-window.js as discussed in Bug 1400352, we are also forcing the old "client" sourcemap pref

> Services.prefs.setBoolPref("devtools.debugger.client-source-maps-enabled", true);

http://searchfox.org/mozilla-central/rev/184f0c7888dd6abb32235377693b7d1fc0b75ac1/devtools/client/framework/toolbox-process-window.js#76

This line can also be removed. I don't think we need to force the new pref to true here? The only scenario where that would be useful would be if a profile had a userValue of false for this pref when creating their BT profile, but then flipped it back to true...
Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

https://reviewboard.mozilla.org/r/180330/#review186104

I think this would fit better in the "Advanced" section rather than re-adding a "Common Preferences" section. This is on by default, and there aren't any cases I'm aware of where we'd want to encourage turning this off.

::: devtools/client/framework/toolbox-options.js:436
(Diff revision 1)
>      }
>    },
>  
> +  updateSourceMapPref: function () {
> +    let enabled = GetPref("devtools.source-map.client-service.enabled");
> +    let box = this.panelDoc.getElementById("source-maps");

Nit: if we `querySelector("[data-pref=devtools.source-map.client-service.enabled]")` we could get rid of the id on the DOM node
Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

https://reviewboard.mozilla.org/r/180330/#review186106

Clearing my review based on comment 12 but feel free to re-request if you think it does belong in a Common section
Attachment #8908689 - Flags: review?(bgrinstead)
(In reply to Julian Descottes [:jdescottes] from comment #11)
> Tom: if you plan to cleanup
> devtools/client/framework/toolbox-process-window.js as discussed in Bug
> 1400352, we are also forcing the old "client" sourcemap pref
> 
> > Services.prefs.setBoolPref("devtools.debugger.client-source-maps-enabled", true);

It seems like now I shouldn't, because afaict the debugger bundle that landed
doesn't include the pref change.
So, this removal should wait for the next debugger bundle.
Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

https://reviewboard.mozilla.org/r/180330/#review185544

> I guess we are setting display:none because of the dynamic pref to toggle between old and new debugger frontend.
> 
> Two options: 
> 
> either we keep this nightly-only option (I think its handy for testing, but less and less relevant)
> In this case we should force the display to visible when we add the pref, ie in http://searchfox.org/mozilla-central/rev/2c9a5993ac40ec1db8450e3e0a85702fa291b9e2/devtools/client/framework/toolbox-options.js#307-351
> 
> we get rid of the pref and of the debugger fieldset here?
> 
> (maybe you kept it for another reason, feel free to ignore my comment in this case :) )

Seems resaonable to me.

The debugger fieldset is still used, see
https://dxr.mozilla.org/mozilla-central/rev/ae39864562c6048fdc2950c5dfedb48e247c3300/devtools/client/framework/toolbox-options.js#321-324

However, I see I forgot to reset the `display` style when needed (this was in an earlier patch and I mistakenly removed it during a rewrite).
Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

https://reviewboard.mozilla.org/r/180330/#review185544

> Seems resaonable to me.
> 
> The debugger fieldset is still used, see
> https://dxr.mozilla.org/mozilla-central/rev/ae39864562c6048fdc2950c5dfedb48e247c3300/devtools/client/framework/toolbox-options.js#321-324
> 
> However, I see I forgot to reset the `display` style when needed (this was in an earlier patch and I mistakenly removed it during a rewrite).

Oh, duh, you mentioned that.  I went with the "make it visible" approach.
Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

https://reviewboard.mozilla.org/r/180330/#review186148

Just a couple of points. Also please update the commit message from "common" to "advanced"

::: devtools/client/framework/toolbox-options.js:356
(Diff revision 2)
>      };
>  
>      for (let prefDefinition of prefDefinitions) {
>        let parent = this.panelDoc.getElementById(prefDefinition.parentId);
>        parent.appendChild(createPreferenceOption(prefDefinition));
> +      parent.style.display = "inline";

Is setting to "inline" really what we want here?  Maybe we should do `<fieldset hidden>` and parent.removeAttribute("hidden")?

::: devtools/client/locales/en-US/toolbox.dtd:179
(Diff revision 2)
>  <!ENTITY options.screenshot.audio.label      "Play camera shutter sound">
>  <!ENTITY options.screenshot.audio.tooltip    "Enables the camera audio sound when taking screenshot">
>  
> +<!-- LOCALIZATION NOTE (options.commonprefs): This is the label for the heading
> +      of all preferences that apply generally across all tools -->
> +<!ENTITY options.commonPrefs.label           "Common Preferences">

This entity can be removed now
Attachment #8908689 - Flags: review?(bgrinstead)
Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

https://reviewboard.mozilla.org/r/180330/#review186186

Looks good to me.  See the entity comment before landing.

::: devtools/client/locales/en-US/toolbox.dtd:150
(Diff revision 3)
>  <!ENTITY options.debugger.label            "Debugger">
>  
>  <!-- LOCALIZATION NOTE (options.sourceMap.label): This is the
> -   - label for the checkbox that toggles source maps in the Debugger -->
> +   - label for the checkbox that toggles source maps in all tools -->
>  <!ENTITY options.sourceMaps.label      "Enable Source Maps">
> -<!ENTITY options.sourceMaps.tooltip    "If you enable this option sources will be mapped in the Debugger and Console.">
> +<!ENTITY options.sourceMaps.tooltip    "If you enable this option sources will be mapped in the tools.">

Rev this entity name (and its reference in the xhtml) since it's changing - i.e. `options.sourceMaps.tooltip1`
Attachment #8908689 - Flags: review?(bgrinstead) → review+
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9393990862b0
remove style editor's source-map pref; r=gl
https://hg.mozilla.org/integration/autoland/rev/0e015b03e234
move source map pref checkbox into "advanced" area in options panel; r=bgrins
https://hg.mozilla.org/mozilla-central/rev/9393990862b0
https://hg.mozilla.org/mozilla-central/rev/0e015b03e234
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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: