Closed Bug 1565782 Opened 4 months ago Closed 4 months ago

Implement tabs.remove extension API in GeckoView

Categories

(GeckoView :: General, enhancement, P3)

Unspecified
All
enhancement

Tracking

(firefox-esr68 wontfix, firefox69 wontfix, firefox70 fixed)

RESOLVED FIXED
mozilla70
Tracking Status
firefox-esr68 --- wontfix
firefox69 --- wontfix
firefox70 --- fixed

People

(Reporter: robwu, Assigned: chrmod)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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:

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.

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,
  });
}
  1. 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");
     } 
}
  1. 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?

Flags: needinfo?(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.

Flags: needinfo?(agi)

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.

Flags: needinfo?(agi)

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

Assignee: nobody → krzysztof.modras
Status: NEW → ASSIGNED

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.

Flags: needinfo?(agi)
Priority: -- → P3
Attachment #9078479 - Attachment description: GeckoView Bug 1565782: implement browser.tabs.remove for webextensions → Bug 1565782: implement browser.tabs.remove for GeckoView webextensions APIs
Attachment #9078479 - Attachment description: Bug 1565782: implement browser.tabs.remove for GeckoView webextensions APIs → Bug 1565782 - implement browser.tabs.remove for GeckoView webextensions APIs
Keywords: checkin-needed

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

Flags: needinfo?(krzysztof.modras)
Keywords: checkin-needed
Attachment #9078479 - Attachment description: Bug 1565782 - implement browser.tabs.remove for GeckoView webextensions APIs → GeckoView Bug 1565782: implement browser.tabs.remove for webextensions
Attachment #9078479 - Attachment description: GeckoView Bug 1565782: implement browser.tabs.remove for webextensions → Bug 1565782 - Implement browser.tabs.remove for GeckoView webextensions APIs

Rebased with cental and updated changelog hash.

Flags: needinfo?(krzysztof.modras) → needinfo?(apavel)
Keywords: checkin-needed

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

Keywords: checkin-needed
Flags: needinfo?(apavel)
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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

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

You need to log in before you can comment on or make changes to this bug.