1473270 - Handle tab detaching for new tab page per-window themes

Status: RESOLVED FIXED

(WebExtensions :: Themes, enhancement)

Description

[Tim Nguyen :ntim]

When a new tab page per-window theme is applied, and a tab is detached away from that window, the tab will still preserve the per-window theme, even though it should instead match the theme of the new window it is in.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1473270 - Handle tab detaching for new tab page per-window themes.

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

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8990024 [details]
Bug 1473270 - Handle tab detaching for new tab page per-window themes.

https://reviewboard.mozilla.org/r/255054/#review261944

This looks good! What are the chances we could write a test for it?

Also, this bug should probably depend on bug 1347204, and not block it (since I imagine we'll want to land bug 1347204 first).

::: toolkit/modules/LightweightThemeConsumer.jsm:122
(Diff revision 1)
>    this._update(temp.LightweightThemeManager.currentThemeForDisplay);
>  
>    this._win.addEventListener("resolutionchange", this);
>    this._win.addEventListener("unload", this, { once: true });
>    this._win.messageManager.addMessageListener("LightweightTheme:Request", this);
>  

Let's put this with the other event listeners, above line 121.

::: toolkit/modules/LightweightThemeConsumer.jsm:167
(Diff revision 1)
>            this._update(this._lastData);
>          }
>          break;
>        case "unload":
>          Services.obs.removeObserver(this, "lightweight-theme-styling-update");
>          this._win.removeEventListener("resolutionchange", this);

I think we should remove the event handler here.

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8990024 [details]
Bug 1473270 - Handle tab detaching for new tab page per-window themes.

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9cf5aaf01772
Handle tab detaching for new tab page per-window themes. r=mconley

Comment 5

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9cf5aaf01772

Comment 6

[Vlad Jiman]

Is manual testing required on this bug? 

If yes, please provide the proper theme/addon, currently all tab themes I've tested work as expected on Nightly and do not change upon detaching, if manual testing is not needed please set the “qe-verify -“ flag.

Thanks!

Comment 7

[Tim Nguyen :ntim]

Bug 1473879 will provide automated tests for this.