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: emit devtools-open-url event for DevTools link selection #36774

Merged
merged 2 commits into from
Jan 26, 2023

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jan 2, 2023

Description of Change

Closes #36715.

This PR adds a new event devtools-open-url corresponding to a link in DevTools being either clicked or selected for opening in a new tab via context menu. There is precedent for event emission corresponding to DevToolsEmbedderMessageDispatcher::Delegate methods - this was previously a no-op and so the primary change here is adding an implementation to surface the action to developers.

Alternatively, we could potentially have implemented the new window behavior ourselves, but this is (imo) a large security hole and as such I believe we should ibstead give developers more fine-grained control over handling and optionally opening new links there.

Tested by adding:

mainWindow.loadURL('https://www.baidu.com/')

mainWindow.webContents.openDevTools()

mainWindow.webContents.on('devtools-open-url', (event, url) => {
  console.log(url)
})

to the createWindow method in a default Fiddle and confirming that url is correctly logged.

Checklist

Release Notes

Notes: Added a new devtools-open-url event to webContents to allow developers to open new windows with them.

@codebytere codebytere added the semver/minor backwards-compatible functionality label Jan 2, 2023
@electron-cation electron-cation bot added new-pr 🌱 PR opened in the last 24 hours api-review/requested 🗳 labels Jan 2, 2023
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

Chrome directs this through Browser::OpenURL: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/devtools/devtools_ui_bindings.cc;l=196-201;drc=fd7b4c1844e6431a5ba4475eb515257a3a238a31

We have an open-url event on app already, but it seems to be only used on macOS, for when a user opens an app-registered URL. Oh well.

Let's call this devtools-open-url instead of devtools-open-in-new-tab.

@deepak1556
Copy link
Member

Can we not just open the platform specific application to handle the URL

// Open the given external protocol URL in the desktop's default manner.
// (For example, mailto: URLs in the default mail user agent.)
void OpenExternal(const GURL& url,
const OpenExternalOptions& options,
OpenCallback callback);
? Is it necessary for applications to handle this event ?

@codebytere codebytere changed the title feat: emit devtools-open-in-new-tab event for DevTools link selection feat: emit devtools-open-url event for DevTools link selection Jan 4, 2023
@codebytere
Copy link
Member Author

codebytere commented Jan 4, 2023

@deepak1556 i considered that, but i think that allowing developers to decide for themselves is comparatively little effort and allows them to make a wider range of choices. I think it's reasonable for example to assume that a developer might want to open a URL in a new BrowserWindow within their own app, instead of shelling out to their external browser.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 9, 2023
Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

API LGTM

Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

API LGTM

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

API LGTM

Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

API LGTM

@codebytere codebytere merged commit 7d46d3e into main Jan 26, 2023
@codebytere codebytere deleted the open-in-new-tab-event branch January 26, 2023 08:54
@release-clerk
Copy link

release-clerk bot commented Jan 26, 2023

Release Notes Persisted

Added a new devtools-open-url event to webContents to allow developers to open new windows with them.

khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
…ctron#36774)

* feat: emit event for DevTools link selection

* chore: devtools-open-in-new-tab -> devtools-open-url
gecko19 pushed a commit to brightsign/electron that referenced this pull request Feb 28, 2023
…ctron#36774)

* feat: emit event for DevTools link selection

* chore: devtools-open-in-new-tab -> devtools-open-url
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Developer tools click link does not create a new page
6 participants