Closed Bug 1257532 Opened 4 years ago Closed 4 years ago

Reload addon should automatically set development prefs

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [btpp-backlog])

Attachments

(1 file, 1 obsolete file)

Today, you have to manually set various development prefs whereas the addon could do that for you.
Attached patch patch v1 (obsolete) — Splinter Review
I thought it was already doing that...
I've been adding various prefs, we may add more (like all experimental features? like storage and others?)
and may be not add the hotreload? I don't know if it is ready?
It seems to work fine with the addon.
Attachment #8731697 - Flags: review?(jryans)
Comment on attachment 8731697 [details] [diff] [review]
patch v1

Review of attachment 8731697 [details] [diff] [review]:
-----------------------------------------------------------------

I would like :bgrins thoughts on this as well.

::: devtools/bootstrap.js
@@ +163,5 @@
>  
> +let prefs = {
> +  // Enable dump as some errors are only printed on the stdout
> +  "browser.dom.window.dump.enabled": true,
> +  "devtools.loader.hotreload": true,

Add a comment about this one too, if you want to keep it.  It is known to not work on Windows though (bug 1252497), so maybe leave it out?
Attachment #8731697 - Flags: review?(jryans)
Attachment #8731697 - Flags: review?(bgrinstead)
Attachment #8731697 - Flags: review+
Comment on attachment 8731697 [details] [diff] [review]
patch v1

Review of attachment 8731697 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/bootstrap.js
@@ +198,5 @@
>    listener.stop();
>    listener = null;
> +
> +  // Restore preferences that used to be before the addon was installed
> +  for (let name in originalPrefValues) {

When is this function called exactly?  When browser is shut down?  When addon is unloaded?
Comment on attachment 8731697 [details] [diff] [review]
patch v1

Review of attachment 8731697 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/bootstrap.js
@@ +198,5 @@
>    listener.stop();
>    listener = null;
> +
> +  // Restore preferences that used to be before the addon was installed
> +  for (let name in originalPrefValues) {

When the addon is disabled/uninstalled/updated, but also when the browser shuts down.

https://developer.mozilla.org/fr/docs/Mozilla/Add-ons/Bootstrapped_extensions#shutdown

If you see value in this, I can move both set-pref and reset-pref code to install/uninstall callbacks.
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Comment on attachment 8731697 [details] [diff] [review]
> patch v1
> 
> Review of attachment 8731697 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/bootstrap.js
> @@ +198,5 @@
> >    listener.stop();
> >    listener = null;
> > +
> > +  // Restore preferences that used to be before the addon was installed
> > +  for (let name in originalPrefValues) {
> 
> When the addon is disabled/uninstalled/updated, but also when the browser
> shuts down.
> 
> https://developer.mozilla.org/fr/docs/Mozilla/Add-ons/
> Bootstrapped_extensions#shutdown
> 
> If you see value in this, I can move both set-pref and reset-pref code to
> install/uninstall callbacks.

In what way would moving it into install/uninstall callbacks change the behavior?
The preferences are only going to be set and reset when the addon is installed and uninstalled.
So that it would be done less frequently. But given that, for now, the addon is mostly installed as a temporary one, that is automatically uninstall on firefox shutdown. It won't change much.
Comment on attachment 8731697 [details] [diff] [review]
patch v1

Review of attachment 8731697 [details] [diff] [review]:
-----------------------------------------------------------------

Seems generally fine since this is a development only addon

::: devtools/bootstrap.js
@@ +203,5 @@
> +    let value = originalPrefValues[name];
> +    let userValue = Services.prefs.getBoolPref(name);
> +    // Only reset the pref if it hasn't changed
> +    if (userValue == prefs[name]) {
> +      Services.prefs.setBoolPref(name, value);

Nit: you could move the `value` declaration into this if block (or inline into the setBoolPref call)
Attachment #8731697 - Flags: review?(bgrinstead) → review+
Priority: -- → P3
Whiteboard: [btpp-backlog]
Attached patch patch v2Splinter Review
Attachment #8735792 - Flags: review+
Attachment #8731697 - Attachment is obsolete: true
https://hg.mozilla.org/integration/fx-team/rev/bc86e440410d9a6ce93c8f41f37fc4f94ee404da
Bug 1257532 - Toggle development prefs when installing the reload addon. r=jryans,bgrins
https://hg.mozilla.org/mozilla-central/rev/bc86e440410d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Thanks, looks great.

Note that I'm not sure the version history is much important?
I would imagine developer to be tracking fx-team or master versus Fx nightly.
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Thanks, looks great.
> 
> Note that I'm not sure the version history is much important?
> I would imagine developer to be tracking fx-team or master versus Fx nightly.

Oh yes, good point. I always include the version for things that aren't in release, but in this case it's not useful, so I'll take it out.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.