WebExtension `tabId` should return a unique ID for each tab
Categories
(GeckoView :: Extensions, enhancement, P1)
Tracking
(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)
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details | Review |
Right now it returns always 10001
.
See e.g. https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/get
Reporter | ||
Updated•5 years ago
|
Comment 1•5 years ago
|
||
Agi says this should be easy to fix with a map of tabIds. Not needed for Fenix MVP.
Reporter | ||
Comment 2•5 years ago
|
||
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
Comment 3•5 years ago
|
||
csadilek says he work around this and we can fix later, but we should try to fix for Fenix MVP or soon after.
Assignee | ||
Comment 4•5 years ago
|
||
Please review
Assignee | ||
Comment 5•5 years ago
|
||
Assignee | ||
Comment 6•5 years ago
|
||
Comment 7•5 years ago
|
||
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 | ||
Comment 8•5 years ago
|
||
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?
Comment 9•5 years ago
|
||
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.
Comment 10•5 years ago
|
||
(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.
Comment 11•5 years ago
|
||
Pushed by jwillcox@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d47b2d152a7d GeckoView unique tab id r=geckoview-reviewers,snorp
Comment 12•5 years ago
|
||
bugherder |
Comment 13•5 years ago
|
||
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
Comment 14•5 years ago
|
||
Comment on attachment 9068016 [details]
Bug 1551377 - GeckoView unique tab id
small geckoview fix for 68.0b7
Comment 15•5 years ago
|
||
bugherder uplift |
Comment 16•5 years ago
|
||
[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
Comment 17•2 years ago
|
||
Moving some WebExtension bugs to the GeckoView::Extensions component.
Description
•