Closed Bug 1451176 Opened 2 years ago Closed 2 years ago

Tab-specific data is lost when tab is moved to another window

Categories

(WebExtensions :: Frontend, defect, P2)

defect

Tracking

(firefox62 verified)

VERIFIED FIXED
mozilla62
Tracking Status
firefox62 --- verified

People

(Reporter: Oriol, Assigned: Oriol)

References

Details

Attachments

(4 files)

Set some tab-specific browser/page/sidebar action data.
Move the tab into another window.

Result: the data is lost

This doesn't seem intuitive and doesn't happen on Chrome.
Attached file test.xpi
Steps to reproduce:

 1. Install attached add-on in about:debugging
 2. You need at least two open windows
 3. Click the browser action. This will display a badge only for the active tab
 4. Move that tab into another window

Result: the badge is lost.
Priority: -- → P2
A simple approach would be adding a "tab-detached" event listener which assigns the data of the nativeTab to the adoptedBy tab.

But bug 1390464 and bug 1419893 will make tab data to inherit from window data. Then a new data object should be created, inheriting from the new window (or use setPrototypeOf with awful performance implications).

I will probably take this after these bugs land.
Your patch doesn't fix the issue for me. And anyways I would need to rewrite it for bug 1390464 and bug 1419893, so I think it's better to wait until these are fixed.
Comment on attachment 8967988 [details]
Bug 1451176: Use Tab wrapper as context object for TabContext.

https://reviewboard.mozilla.org/r/236678/#review242744

Looks good but needs a test.
Let it be a pageAction test, since I will test sidebarAction and browserAction in bug 1390464 and bug 1419893.
This patch takes bug 1390464 and bug 1419893 into account.
Comment on attachment 8971564 [details]
Bug 1451176 - Preserve tab-specific data when tab is moved to another window

https://reviewboard.mozilla.org/r/240324/#review246542

I'm assuming this is a replacement of, not addition to, Kris' patch.  My testing of your str in comment 1 against Kris' patch showed that your issue was addressed even though you said it is not.

::: browser/components/extensions/parent/ext-browser.js:175
(Diff revision 1)
>  
> +  tabDetached(eventType, {nativeTab, adoptedBy}) {
> +    if (!this.tabData.has(nativeTab)) {
> +      return;
> +    }
> +    if (this.recreateOnDetached) {

Why is recreateOnDetached necessary at all?  (ie. why not do that always?)  What makes pageAction different?
Attachment #8971564 - Flags: review?(mixedpuppy)
> My testing of your str in comment 1 against Kris' patch showed that your issue was addressed even though you said it is not.
Kris updated the patch after I said it didn't work. I didn't test the update but it made sense to me.

> I'm assuming this is a replacement of, not addition to, Kris' patch.
Yes, his patch doesn't take inheritance from windows into account.

> Why is recreateOnDetached necessary at all?  (ie. why not do that always?)  What makes pageAction different?
pageAction doesn't need it because it doesn't accept windowId (this patch is on top of bug 1419893 which enables it in browserAction, please review bug 1419893 first).

Sure, it wouldn't be bad for pageAction if I used Object.assign anyway. But I thought that TabContext is generic enough to store the arbitrary value returned by getDefaults, possibly a primitive, so it would be bad practice to use Object.assign by default.

But if you prefer I can add a comment saying that getDefaults is supposed to return an object, and always use Object.assign. Or, other than using "tab-detached" event listeners, use Kris' patch but add a WeakMap which stores, for each tab data, the latest tab to which it was associated. In get(nativeTab), if the tabs are different, recreate the object. I would also need a WindowContext or something like this.
If TabContext values are supposed to be objects, it seems better to create them in the TabContext class.
Assignee: nobody → oriol-bugzilla
Comment on attachment 8967988 [details]
Bug 1451176: Use Tab wrapper as context object for TabContext.

https://reviewboard.mozilla.org/r/236678/#review250478
Attachment #8967988 - Flags: review?(mixedpuppy)
Comment on attachment 8971564 [details]
Bug 1451176 - Preserve tab-specific data when tab is moved to another window

https://reviewboard.mozilla.org/r/240324/#review250396

::: browser/components/extensions/test/browser/head_pageAction.js:48
(Diff revision 3)
>  
>        test(async expecting => {
> -        function finish() {
> -          // Check that the actual icon has the expected values, then
> -          // run the next test.
> -          browser.test.sendMessage("nextTest", expecting, tests.length);
> +        let [tab] = await browser.tabs.query({active: true, currentWindow: true});
> +        let {id: tabId, windowId, url} = tab;
> +
> +        browser.test.log(`Get details: tab={id: ${tabId}, url: ${JSON.stringify(url)}}`);

stringify of the url shouldn't be necessary
Attachment #8971564 - Flags: review?(mixedpuppy) → review+
Comment on attachment 8971564 [details]
Bug 1451176 - Preserve tab-specific data when tab is moved to another window

Some additional changes:
 - Rebased updates from bug 1419893
 - Removed useless extension parameter and property in TabContext
 - Edited android's TabContext for consistency with desktop
Attachment #8971564 - Flags: review+ → review?(mixedpuppy)
Attachment #8971564 - Flags: review?(mixedpuppy) → review+
Try is green now
Keywords: checkin-needed
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cf48264900e5
Preserve tab-specific data when tab is moved to another window r=mixedpuppy
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/cf48264900e5
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Depends on: 1463661
Depends on: 1463751
No longer depends on: 1463661
Attached image tab specific.gif
Reproduced in Firefox 61.0a1 (20180403220040).
Retested end verified in Firefox 62.0a1 (20180525102548)on Windows 10 64Bit and MacOS 10.13.3.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.