1217153 - GCLI `tools reload` command resets devtools.theme to light

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Tim Nguyen :ntim]

STR:
- switch to dark theme
- do tools reload from GCLI

AR: devtools turn light

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1217153 - Clean up old paths that landed post-migration. r=bgrins

Bug 1217153 - Clean up old paths that landed post-migration. r=bgrins

Comment 2

[Brian Grinstead [:bgrins]]

Comment on attachment 8677084 [details]
MozReview Request: Bug 1217153 - Clean up old paths that landed post-migration. r=bgrins

https://reviewboard.mozilla.org/r/22875/#review20399

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I used the wrong bug number for the review above, it was meant for bug 1216592.

Comment 4

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca0bf52e9086

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Do not reset devtools theme when hot reloading the tools.

It was quite fun to debug a hot reload bug in matter of minutes thanks to hot reload :)
The issue was that we were considering the hot reload cleanup as an addon cleanup
and were resetting the theme...
I tweaked that codepath a bit.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8680566 [details] [diff] [review]
Do not reset devtools theme when hot reloading the tools.

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

::: devtools/client/framework/gDevTools.jsm
@@ +272,5 @@
>     * Needed so that add-ons can remove themselves when they are deactivated
>     *
>     * @param {string|object} theme
>     *        Definition or the id of the theme to unregister.
> +   * @param {Boolean} isCoreUnload

I don't get this name / comment.  This is being passed as true inside a loop:

`let definition of gDevTools.getThemeDefinitionArray()`, which includes addon themes.

If you really want default themes they can be gotten from require("devtools/client/definitions") and we can get rid of the extra parameter altogether.  But I suspect then this bug would hit anyone who's using an addon theme, right?

Comment 7

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #6)
> Comment on attachment 8680566 [details] [diff] [review]
> Do not reset devtools theme when hot reloading the tools.
> 
> Review of attachment 8680566 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/framework/gDevTools.jsm
> @@ +272,5 @@
> >     * Needed so that add-ons can remove themselves when they are deactivated
> >     *
> >     * @param {string|object} theme
> >     *        Definition or the id of the theme to unregister.
> > +   * @param {Boolean} isCoreUnload
> 
> I don't get this name / comment.  This is being passed as true inside a loop:
> 
> `let definition of gDevTools.getThemeDefinitionArray()`, which includes
> addon themes.
> 
> If you really want default themes they can be gotten from
> require("devtools/client/definitions") and we can get rid of the extra
> parameter altogether.  But I suspect then this bug would hit anyone who's
> using an addon theme, right?

I think this is still correct. I think it is fine not reseting to default theme when we were on an addon theme while we reload devtools. The very precise codepath where we want to reset is when the addon itself is disable/uninstalled. The codepath I changed in main.js is fired only when we shutdown firefox or reload devtools via gcli or the key binding.

Having said that, I understand the complexity of this change, but don't really know how to make it simplier. Actually anything I can think about would make it more complex, but may be easier to follow:

 - make registerTheme accept another "isCoreTheme" parameter and flag the theme as core or addon. Or have a "type" parameter which would be 'core' or 'theme',
 - or, do similar thing, but instead of having a parameter in registerTheme, have it directly on the Theme object passed to registerTheme,
 - or, in unregisterTheme, do not add a parameter, but instead compare the `theme` object to the list of themes from require(...definition).defaultThemes. If that's a default theme, bypass the pref reset.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Alexandre Poirot [:ochameau] from comment #7
> > If you really want default themes they can be gotten from
> > require("devtools/client/definitions") and we can get rid of the extra
> > parameter altogether.  But I suspect then this bug would hit anyone who's
> > using an addon theme, right?
> 
> I think this is still correct. I think it is fine not reseting to default
> theme when we were on an addon theme while we reload devtools. The very
> precise codepath where we want to reset is when the addon itself is
> disable/uninstalled. The codepath I changed in main.js is fired only when we
> shutdown firefox or reload devtools via gcli or the key binding.

The situation I'm imagining is that someone has an addon theme installed and applied.  With the current patch, this behavior is actually consistent with default themes, because gDevTools.getThemeDefinitionArray() includes it, so `isCoreUnload` will be passed in as true to the call to unregisterTheme.  But the comments seem to indicate that we'd rather have this code path only run on 'core themes', so if the function was updated to match that then a `tools reload` would reset the addon theme to light theme.

> require(...definition).defaultThemes. If that's a default theme, bypass the
> pref reset.

If we really want this to only run on default themes, I think this would be the best solution.  But I'm not understanding why we'd want to make that the distinction, as opposed to isToolReload (which matches the current behavior of the patch)?

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: patch v2

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #8)
> If we really want this to only run on default themes, I think this would be
> the best solution.  But I'm not understanding why we'd want to make that the
> distinction, as opposed to isToolReload (which matches the current behavior
> of the patch)?

What's the reasoning for resetting the theme on a `tools reload` if an addon theme is applied?

Comment 11

[Alexandre Poirot [:ochameau]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to Brian Grinstead [:bgrins] from comment #8)
> > If we really want this to only run on default themes, I think this would be
> > the best solution.  But I'm not understanding why we'd want to make that the
> > distinction, as opposed to isToolReload (which matches the current behavior
> > of the patch)?
> 
> What's the reasoning for resetting the theme on a `tools reload` if an addon
> theme is applied?

It doesn't hurt. I don't want to be pixel perfect here.
I wish we could just avoid having to unregister anything (tool/theme) during reload!!
But all relates to gDevTools.jsm being a jsm. So it stays alive when we reload.
Instead it should be just another module, possibly devtools/client/main.js and be completely reloaded.
Instead, today, we have to ensure cleaning up states in it, but avoid some costly/buggy operations like emitting tool-unregistered or resetting theme. We could also avoid various others things... But I just want to fix the real bugs until we really solve the whole story with a significant refactoring.

Comment 12

[Alexandre Poirot [:ochameau]]

I'll drop a note about this reload issuse in bug 1188405 which is about this refactoring.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: patch v3

Fix indexof -> indexOf typo.

Comment 14

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4afe7c262984

Comment 15

[Brian Grinstead [:bgrins]]

Maybe I'm misunderstanding this - when exactly is unregisterTheme being called?  Is it when the browser shuts down and then also when `tools reload` is called, or only the latter?

Comment 16

[Alexandre Poirot [:ochameau]]

unregisterTheme is called from these two places:
# addon disable/uninstall
  http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/dev/toolbox.js#92
  But note that if I remember correctly, addons also get disabled during firefox shutdown,
  so I imagine it will be called during shutdown for all theme/toolbox SDK addons.
# on devtools reload
  http://mxr.mozilla.org/mozilla-central/source/devtools/client/main.js#37
  when you do tools reload or hit CTRL+ALT+R

unregisterTool is called _also_ during shutdown:
  http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/gDevTools.jsm#494

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: patch v4

- updated comment,
- still using shutdown check as we don't want to reset addon theme
on firefox shutdown (addons get disabled during ff shutdown)
- no longer check for DefaultTheme.include(theme) as theme objects
are different between two reloads. That because we reload
definitions module and so register new Theme objects
on each new load, whereas gDevtools keep the very first set of Theme objects.
(again this is going to be simplify if we convert gDevtools to a module
 that is reloaded as everything else!)

Comment 18

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e66512e4ca7b

Comment 19

[Brian Grinstead [:bgrins]]

Comment on attachment 8682243 [details] [diff] [review]
patch v4

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

Seems fine

::: devtools/client/framework/gDevTools.jsm
@@ +293,5 @@
> +    let isCoreTheme = DefaultThemes.some(t => t.id === themeId);
> +
> +    // Reset the theme if an extension theme that's currently applied
> +    // is being removed.
> +    // Ignore shutdown since addons get disabled during at that time.

"during at" -> "at"

Comment 20

[Alexandre Poirot [:ochameau]]

Attachment: patch v5

Fixed the comment.

Comment 21

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/71e0c1cfbb7b5e421b020d962902d359ab6e38cb
Bug 1217153 - Do not reset devtools theme when hot reloading the tools. r=bgrins

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71e0c1cfbb7b

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/71e0c1cfbb7b

Comment 24

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion