Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: session.setExtensionAPIHandlers for handling chrome.tabs.get and default tab requests #27917

Draft
wants to merge 53 commits into
base: main
Choose a base branch
from

Conversation

sentialx
Copy link
Contributor

@sentialx sentialx commented Feb 25, 2021

Description of Change

This PR adds an API session.setExtensionAPIHandlers so that users can specify the behavior of the chrome.tabs.get API and the default tab behavior (for example when an extension calls chrome.tabs.getZoom without tabId it fallbacks to the active tab). The same handlers are being used for Port.sender.Tab and also for ElectronExtensionsBrowserClient::GetTabAndWindowIdForWebContents which is responsible for chrome.webRequest request details tabId property). Also adds support for chrome.tabs.insertCSS and removeCSS

This PR does not introduce any breaking changes to the API. If the handlers are not registered, chrome.tabs.get will fallback to the previous behavior and warn the user.

For example:

  ses.setExtensionAPIHandlers({
    getTab: (webContents) => {
      return {
        id: webContents.id,
        active: true,
        windowId: 1,
        highlighted: true,
        index: 1,
        groupId: 1,
        openerTabId: 0,
        discarded: false,
        autoDiscardable: true,
        pinned: true,
        mutedReason: 'user',
	    mutedExtensionId: 'sdfdsasdafadwerewrwxcvxcver',
      };
    },
    getActiveTab: (sender) => {
      // We have access to the sender WebContents, 
      // so users can for example get an active tab within the window 
      // from which the request originated.
      return webContents.fromId(selectedTabId);
    },
  });

cc @nornagon @samuelmaddock

Could this be backported to 11-x-y and 12-x-y?

Checklist

Release Notes

Notes:

  • Added session.setExtensionAPIHandlers so that users can specify the behavior for chrome.tabs.get and the default tab.
  • Added more properties to the Port.sender.tab.
  • Added support for chrome.tabs.insertCSS and chrome.tabs.removeCSS.
  • Fixed an issue where chrome.webRequest would not specify the tabId property in the request details.
  • Added webContents.getExtensionTabDetails to get the Chrome API Tab representation of webContents.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 25, 2021
Copy link

@CyberFlameGO CyberFlameGO left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.h Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.h Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/extensions/api/tabs/tabs_api.cc Outdated Show resolved Hide resolved
shell/browser/extensions/api/tabs/tabs_api.cc Outdated Show resolved Hide resolved
shell/browser/extensions/extension_tab_util.cc Outdated Show resolved Hide resolved
shell/browser/extensions/extension_tab_util.h Outdated Show resolved Hide resolved
Co-authored-by: Jeremy Rose <nornagon@nornagon.net>
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Mar 4, 2021
@sentialx sentialx requested a review from nornagon March 10, 2021 19:03
shell/browser/api/electron_api_web_contents.cc Outdated Show resolved Hide resolved
shell/browser/extensions/api/tabs/tabs_constants.cc Outdated Show resolved Hide resolved
docs/api/structures/tab-details.md Outdated Show resolved Hide resolved
docs/api/session.md Show resolved Hide resolved
docs/api/session.md Outdated Show resolved Hide resolved
docs/api/web-contents.md Outdated Show resolved Hide resolved
docs/api/web-contents.md Outdated Show resolved Hide resolved
shell/browser/api/electron_api_session.cc Outdated Show resolved Hide resolved
shell/browser/extensions/api/tabs/tabs_api.cc Outdated Show resolved Hide resolved
shell/browser/extensions/extension_tab_util.cc Outdated Show resolved Hide resolved
sentialx and others added 2 commits March 10, 2021 22:19
Co-authored-by: Jeremy Rose <nornagon@nornagon.net>
@sentialx sentialx marked this pull request as ready for review March 15, 2021 13:00
@sentialx sentialx requested a review from nornagon March 15, 2021 13:00
@sentialx sentialx changed the title feat: session.setChromeAPIHandlers for handling chrome.tabs.get and default tab requests feat: session.setExtensionAPIHandlers for handling chrome.tabs.get and default tab requests Mar 15, 2021
@nornagon
Copy link
Member

Re: gin::CreateHandle, it looks like our helpers weren't properly compatible. I've updated them and pushed to your branch, hope that's OK!

I also converted the callbacks to be direct values, instead of unique_ptr, since there's no particular need for the indirection.

@sentialx
Copy link
Contributor Author

Thanks!

@nornagon
Copy link
Member

@sentialx @samuelmaddock how are we feeling about merging this? I know it's been a while!

@sentialx
Copy link
Contributor Author

I think everything was done here, so I guess it's okay to merge.

shell/browser/extensions/extension_tab_util.cc Outdated Show resolved Hide resolved
"description": "The tab's loading status."
},
{
"id": "MutedInfoReason",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before we merge this, I'd like to sync parts of this file from upstream to ensure the schema is up to date.

@nornagon
Copy link
Member

nornagon commented Mar 3, 2022

API LGTM

Co-authored-by: Samuel Maddock <samuel.maddock@gmail.com>
@nornagon
Copy link
Member

Marking as wip pending some attention from @samuelmaddock and/or @sentialx

@nornagon
Copy link
Member

converted to draft to reflect WIP status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants