Implement tabs.remove extension API in GeckoView
Categories
(GeckoView :: Extensions, enhancement, P3)
Tracking
(firefox-esr68 wontfix, firefox69 wontfix, firefox70 fixed)
People
(Reporter: robwu, Assigned: chrmod)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
The closeTab
method of the BrowserApp
shim is currently just a no-op stub, added in bug 1499067.
It is used in a few places in extension code:
tabs.remove
implementation: https://searchfox.org/mozilla-central/rev/883bfd385efc917d5e787ba66fb51824f6f0e3d6/mobile/android/components/extensions/ext-tabs.js#359- Closing an "extension popup" (=tab that opens when browserAction/pageAction button is clicked) after switching away from it: https://searchfox.org/mozilla-central/rev/883bfd385efc917d5e787ba66fb51824f6f0e3d6/mobile/android/components/extensions/ext-utils.js#396
- Closing extension tabs when an extension is unloaded: https://searchfox.org/mozilla-central/rev/883bfd385efc917d5e787ba66fb51824f6f0e3d6/mobile/android/components/extensions/ext-android.js#47
Extension popups are not implemented because browserAction/pageAction are not implemented (1530402). Even if they are implemented, we will hopefully not open them as a "tab", which has always been a hack to work around the limitations on Fennec.
Extension tabs do not need to be closed yet, because they are currently not supported (bug 1534640).
This leaves us with tabs.remove
as the only user of BrowserApp.removeTab
. I suggest that we remove the removeTab
shim (since it was only added for tabs.remove
in bug 1499067, and nothing else should rely on it), and add a new async method that is called by the implementation of tabs.remove
in ext-tabs.js
. It should probably call into WebExtensionController.java
, which has the responsibility of closing the "tab"/window/GeckoSession.
When tabs.remove
is implemented, we should remove the various calls to window.close()
that were introduced in https://phabricator.services.mozilla.com/D36575 to help tests pass.
Assignee | ||
Comment 1•5 years ago
|
||
Implementation would go like follows:
async closeTab(aTab, options) {
if (options.extensionId) {
throw new Error('options.extensionId is required');
}
const window = aTab.browser.ownerGlobal;
await window.WindowEventDispatcher.sendRequestForResult({
type: "GeckoView:WebExtension:CloseTab",
extensionId: options.extensionId,
});
}
- new delegate in
WebExtensionConroller.TabCloseDelegate
https://searchfox.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/geckoview/WebExtensionController.java#12
public class WebExtensionController {
public interface TabCloseDelegate {
@UiThread
@Nullable
default void onTabClose(@Nullable WebExtension source, @NotNull GeckoSession geckosession) throws Exception {
throw new Exception("WebExtension cannot close tabs");
}
}
- WebExtensionConroller listens for new message "GeckoView:WebExtension:CloseTab"
public class WebExtensionController {
private class TabCloseEventListener implements BundleEventListener {
@Override
public void handleMessage(final String event, final GeckoBundle message,
final EventCallback callback, final GeckoSession session) {
if ("GeckoView:WebExtension:CloseTab".equals(event)) {
closeTab(message, session, callback);
return;
}
}
}
@UiThread
public void setTabCloseDelegate(final @Nullable TabCloseDelegate delegate) {
mTabCloseDelegate = delegate;
// implemented similarly to setTabDelegate
}
private void closeTab(final GeckoBundle message, final GeckoSession session, final EventCallback callback) {
if (mTabCloseDelegate == null) {
callback.sendError();
return;
}
WebExtension extension = mDispatcher.getWebExtension(message.getString("extensionId"));
try {
mTabCloseDelegate.onTabClose(extension, session);
callback.sendSuccess();
} catch(Exception e) {
callback.sendError(e.message);
}
}
}
Does it make sense?
Comment 2•5 years ago
|
||
Mostly looks good! One thing though:
We don't need a TabCloseDelegate
, just put the onTabClose
in the TabDelegate
. Similar, we can just reuse EventListener
instead of having a new TabCloseEventListener
.
Also nit, I would rename onTabClose -> onCloseTabRequest
similar to the ContentDelegate::onCloseRequest
In general I think we should use TabDelegate
for every action that we need coming off of the tabs
API.
As usual, thank you for working on this! Really appreciated.
Assignee | ||
Comment 3•5 years ago
|
||
Was thinking about it and having a separate delegate has one benefit. We don't have to listen to messages that we are not interested in. This is more gradual control over what is being handled. But if you're fine with just one tab delegate I'm also for it.
Reporter | ||
Comment 4•5 years ago
|
||
I agree with agi; it's preferred to have only one TabsDelegate that handles every tab-specific method.
If an app is interested in any tabs API, it's reasonable to require them to have a concrete implementation of the delegate (even if it's just delegated to the default implementation).
Comment 5•5 years ago
|
||
All interfaces need to have default
impls in GeckoView (this is enforced by apilint
) so the app has full control on what it needs to implement. Having the default
impl negate the request by default is what we do everywhere else and I think it's fine here too.
Assignee | ||
Comment 6•5 years ago
|
||
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Comment 7•5 years ago
|
||
Failed to Land
On Fri, July 26, 2019, 10:05 PM GMT+3, by archaeopteryx@coole-files.de.
Revisions: D38216 diff 134830
Details: We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. applying /tmp/tmpn2dnMt mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md Hunk #2 FAILED at 355. 1 out of 2 hunks FAILED -- saving rejects to file mobile/android/geckoview/src/main/java/org/mozilla/geckoview/doc-files/CHANGELOG.md.rej abort: patch command failed: exited with status 256
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
Rebased with cental and updated changelog hash.
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fefe727dcf3c
Implement browser.tabs.remove for GeckoView webextensions APIs r=agi,robwu,rpl,geckoview-reviewers,snorp
Updated•5 years ago
|
Comment 10•5 years ago
|
||
bugherder |
Comment 11•5 years ago
|
||
Do we need to uplift the browser.tabs.remove extension API to GV 69 Beta? Does Fenix need this API right away?
Comment 12•5 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #11)
Do we need to uplift the browser.tabs.remove extension API to GV 69 Beta? Does Fenix need this API right away?
No. firefox69=wontfix because we can let this API ride with 70.
Comment 13•2 years ago
|
||
Moving some extension bugs to the GeckoView::Extensions component.
Description
•