Closed Bug 1551377 Opened 5 years ago Closed 5 years ago

WebExtension `tabId` should return a unique ID for each tab

Categories

(GeckoView :: Extensions, enhancement, P1)

Unspecified
Android
enhancement

Tracking

(firefox67 wontfix, firefox67.0.1 wontfix, firefox68 fixed, firefox69 fixed)

RESOLVED FIXED
mozilla69
Tracking Status
firefox67 --- wontfix
firefox67.0.1 --- wontfix
firefox68 --- fixed
firefox69 --- fixed

People

(Reporter: agi, Assigned: chrmod)

References

(Blocks 1 open bug)

Details

(Whiteboard: [geckoview:fenix:m7])

Attachments

(1 file)

Type: defect → enhancement

Agi says this should be easy to fix with a map of tabIds. Not needed for Fenix MVP.

OS: All → Android
Priority: -- → P2

Looks like we should be able to keep a WeakMap of all tabs the same way Desktop does, I'll try that tonight/tomorrow: https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-browser.js#320-331

csadilek says he work around this and we can fix later, but we should try to fix for Fenix MVP or soon after.

Whiteboard: [geckoview:fenix:m7]

Please review

Krzysztof, do you have hg permissions to land your fix (using "View Stack in Lando" in Phabricator)? Or do you need someone else to land it now?

Christian, should we need to uplift this fix to GeckoView 68 Beta? You said you have a workaround in the Reader View extension.

Assignee: nobody → krzysztof.modras
Flags: needinfo?(csadilek)

Chris, I don't have the right to land it.

Christian, my plan is to fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=1539144
https://bugzilla.mozilla.org/show_bug.cgi?id=1551378

  • tabs and webNavigation listeners ASAP. If you consider uplifting some changes to 68 Beta, what would be the latest date for you to accept patches?

Thanks, Krzysztof! Not my call re: timing.

Chris, yes, we have a workaround. The problem is that we can't send a message from a background script to a tab directly i.e.

browser.tabs.update({url: url}).then((tab) => {         
  browser.tabs.sendMessage(tab.id, message);
});

doesn't work. To work around this, we have the tab send a message to the background script, which responds with a message for that tab. This also forces us to save some state in the background script. This seems somewhat more complicated than this patch. So, if we consider this patch low risk it would be great to get it uplifted, but it's not a must-have.

Flags: needinfo?(csadilek)

(In reply to Krzysztof Jan Modras from comment #8)

Chris, I don't have the right to land it.

Looks like James already queued your patches to be landed.

Christian, my plan is to fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=1539144
https://bugzilla.mozilla.org/show_bug.cgi?id=1551378

  • tabs and webNavigation listeners ASAP. If you consider uplifting some changes to 68 Beta, what would be the latest date for you to accept patches?

Krzysztof, do we need the fixes for browser.tabs.create and tabs.query for this bug? Or for Fenix's Reader View extension?

(In reply to Christian Sadilek [:csadilek] from comment #9)

To work around this, we have the tab send a message to the background script, which responds with a message for that tab. This also forces us to save some state in the background script. This seems somewhat more complicated than this patch. So, if we consider this patch low risk it would be great to get it uplifted, but it's not a must-have.

Krzysztof's fix should be very safe to uplift. I can request beta uplift after the fix is merged to mozilla-central.

Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d47b2d152a7d
GeckoView unique tab id r=geckoview-reviewers,snorp
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

Comment on attachment 9068016 [details]
Bug 1551377 - GeckoView unique tab id

Beta/Release Uplift Approval Request

  • User impact if declined: The Fenix team wants this fix in 68 to avoid a cumbersome workaround for Fenix's Reader View extension.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a very small code change that only affects GeckoView.
  • String changes made/needed: No string changes
Attachment #9068016 - Flags: approval-mozilla-beta?

Comment on attachment 9068016 [details]
Bug 1551377 - GeckoView unique tab id

small geckoview fix for 68.0b7

Attachment #9068016 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

[geckoview:fenix:m7] bugs should be priority P1.

I'm editing a bunch of GeckoView bugs. If you'd like to filter all this bugmail, search and destroy emails containing this UUID:

e88a5094-0fc0-4b7c-b7c5-aef00a11dbc9

Priority: P2 → P1

Moving some WebExtension bugs to the GeckoView::Extensions component.

Component: General → Extensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: