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: partially support chrome.tabs.update #30069

Merged
merged 4 commits into from
Jul 27, 2021
Merged

feat: partially support chrome.tabs.update #30069

merged 4 commits into from
Jul 27, 2021

Conversation

nornagon
Copy link
Member

@nornagon nornagon commented Jul 9, 2021

Description of Change

Adds partial support for the chrome.tabs.update extensions API, to support
link clicks in PDFs.

Closes #29091.

Checklist

Release Notes

Notes: Clicking a hyperlink in a PDF now does what you'd expect.

@nornagon nornagon added the semver/patch backwards-compatible bug fixes label Jul 9, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 9, 2021
@nornagon nornagon marked this pull request as draft July 9, 2021 22:58
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 10, 2021
@nornagon nornagon marked this pull request as ready for review July 26, 2021 18:50
@nornagon nornagon requested a review from VerteDinde July 26, 2021 23:42
Copy link
Member

@VerteDinde VerteDinde left a comment

Choose a reason for hiding this comment

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

LGTM! Tested the original issue, which is now fixed, and code looks great. Had one question about an error message, not blocking 👍

// Don't let the extension crash the browser or renderers.
if (IsKillURL(url)) {
const char kNoCrashBrowserError[] =
"I'm sorry. I'm afraid I can't do that.";
Copy link
Member

Choose a reason for hiding this comment

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

😂 I actually like this message, but would it be possible for us to give the user more of a hint about what went wrong? Looking at kill_hosts[] above, would a user be trying to intentionally kill their tab/app by passing one of these URLs in?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually copy-pasted directly from Chromium: https://source.chromium.org/chromium/chromium/src/+/main:chromecast/browser/extensions/api/tabs/tabs_constants.cc;l=84;drc=479cc17585a64910853e9949b053499ecbeca9a5

This only happens if an extension tries to do something like chrome.tabs.update(tabId, { url: 'chrome://crash' }).

@nornagon
Copy link
Member Author

CI failures unrelated; merging.

@nornagon nornagon merged commit ceebae1 into main Jul 27, 2021
@nornagon nornagon deleted the tabs-update branch July 27, 2021 20:36
@release-clerk
Copy link

release-clerk bot commented Jul 27, 2021

Release Notes Persisted

Clicking a hyperlink in a PDF now does what you'd expect.

BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
@richpixel
Copy link

richpixel commented Dec 21, 2021

Release Notes Persisted

Clicking a hyperlink in a PDF now does what you'd expect.

What is the expected behavior? I'm on electron 14.2.1 and when links are clicked nothing happens. Hovering over the link shows the pointer (as expected). I'm rendering the PDF with an embed tag in a Renderer process. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: PDF containing hyperlinks don't work when clicking on them
3 participants