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

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Tom Tromey :tromey]

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.

Comment 1

[Tom Tromey :tromey]

I have a patch for this, but maybe this shouldn't really be done until the debugger is
updated to use the unified pref.

Comment 3

[Tom Tromey :tromey]

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.

Comment 4

[Tom Tromey :tromey]

Step 1: https://github.com/devtools-html/debugger.html/pull/4028

Comment 5

[Tom Tromey :tromey]

The common area was removed in bug 1307881, but I'm going to resurrect it.

Comment 6

[Tom Tromey :tromey]

Attachment: Bug 1371849 - remove style editor's source-map pref;

This changes the style editor and inspector to use the same pref that
the other tools are using.  This pref is a bit of a misnomer for now,
because the inspector and style editor aren't actually using the
client-side service; but I think the name is not very important, and
unifying the prefs is worthwhile.

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

Comment 7

[Tom Tromey :tromey]

Attachment: Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

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

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 8908688 [details]
Bug 1371849 - remove style editor's source-map pref;

https://reviewboard.mozilla.org/r/180328/#review185490

Comment 9

[Tom Tromey :tromey]

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 10

[Julian Descottes [:jdescottes]]

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 :) )

Comment 11

[Julian Descottes [:jdescottes]]

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 12

[Brian Grinstead [:bgrins]]

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 13

[Brian Grinstead [:bgrins]]

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

Comment 14

[Tom Tromey :tromey]

(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 15

[Tom Tromey :tromey]

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 16

[Tom Tromey :tromey]

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 17

[Tom Tromey :tromey]

Comment on attachment 8908688 [details]
Bug 1371849 - remove style editor's source-map pref;

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

Comment 18

[Tom Tromey :tromey]

Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

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

Comment 19

[Brian Grinstead [:bgrins]]

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

Comment 20

[Tom Tromey :tromey]

Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

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

Comment 21

[Brian Grinstead [:bgrins]]

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`

Comment 22

[Tom Tromey :tromey]

Comment on attachment 8908689 [details]
Bug 1371849 - move source map pref checkbox into "advanced" area in options panel;

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

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9393990862b0
https://hg.mozilla.org/mozilla-central/rev/0e015b03e234