1565782 - Implement tabs.remove extension API in GeckoView

Status: RESOLVED FIXED

(GeckoView :: Extensions, enhancement, P3)

Description

[Rob Wu [:robwu]]

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.

Comment 1

[Krzysztof Jan Modras [:chrmod]]

Implementation would go like follows:

1. https://searchfox.org/mozilla-central/source/mobile/android/modules/geckoview/GeckoViewTab.jsm#45

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,
  });
}

2. 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");
     } 
}

3. 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

[[ex-Mozilla] Agi Sferro | :agi]

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.

Comment 3

[Krzysztof Jan Modras [:chrmod]]

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.

Comment 4

[Rob Wu [:robwu]]

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

[[ex-Mozilla] Agi Sferro | :agi]

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.

Comment 6

[Krzysztof Jan Modras [:chrmod]]

Attachment: Bug 1565782 - Implement browser.tabs.remove for GeckoView webextensions APIs

Comment 7

[Andreea Pavel [:apavel]]

Failed to Land
On Fri, July 26, 2019, 10:05 PM GMT+3, by [e-mail].
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

Comment 8

[Krzysztof Jan Modras [:chrmod]]

Rebased with cental and updated changelog hash.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fefe727dcf3c
Implement browser.tabs.remove for GeckoView webextensions APIs r=agi,robwu,rpl,geckoview-reviewers,snorp

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/fefe727dcf3c

Comment 11

[Chris Peterson [:cpeterson]]

Do we need to uplift the browser.tabs.remove extension API to GV 69 Beta? Does Fenix need this API right away?

Comment 12

[Chris Peterson [:cpeterson]]

(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

[Chris Peterson [:cpeterson]]

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