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: enable pdf viewer #21794

Merged
merged 50 commits into from Feb 13, 2020
Merged

feat: enable pdf viewer #21794

merged 50 commits into from Feb 13, 2020

Conversation

@nornagon
Copy link
Member

nornagon commented Jan 16, 2020

Description of Change

This builds on top of the native extensions support work that's been ongoing (see #19447) to implement the pdfium-based PDF viewer, the same one that Chrome uses.

Fixes #12337.

Checklist

Release Notes

Notes: Restored support for pdfium-based PDF viewer.

@nornagon nornagon requested a review from electron/wg-upgrades as a code owner Jan 16, 2020
BUILD.gn Outdated Show resolved Hide resolved
shell/app/atom_content_client.cc Outdated Show resolved Hide resolved
shell/browser/atom_browser_client.cc Outdated Show resolved Hide resolved
shell/browser/atom_browser_client.cc Outdated Show resolved Hide resolved
shell/browser/plugins/plugin_utils.cc Show resolved Hide resolved
nornagon added 3 commits Jan 16, 2020
@nornagon nornagon changed the title [wip] feat: pdf viewer feat: pdf viewer Jan 16, 2020
@nornagon nornagon changed the title feat: pdf viewer feat: pdf viewer [wip] Jan 16, 2020
@electron-cation electron-cation bot removed the new-pr 🌱 label Jan 17, 2020
nornagon added 10 commits Feb 7, 2020
@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Feb 11, 2020

@nornagon has manually backported this PR to "9-x-y", please check out #22131

@trop trop bot added in-flight/9-x-y and removed target/9-x-y labels Feb 11, 2020
Copy link
Member

deepak1556 left a comment

LGTM

I am good with the merge, can make changes in a follow up PR as the diff is quite big.

BUILD.gn Show resolved Hide resolved
// loaded. However, in Electron, we load the PDF extension unconditionally
// when it is enabled in the build, so we're OK to load the plugin eagerly
// here.
content::WebPluginInfo info;

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Feb 11, 2020

Member

why do we want to load the plugin eagerly ? What happens if we let it to load with the default flow when the pdf type is encountered by the renderer.

Its weird to see the redeclaration of the plugin info just for eager loading.

This comment has been minimized.

Copy link
@nornagon

nornagon Feb 12, 2020

Author Member

Yeah, I agree it's a little weird to see. The reason I did it this way was to reduce the amount of machinery we need to pull in. Currently we have nothing that listens for extension loads, and it was easy enough to put it here. If there's a good reason to make it lazy later on, we can do so. I think this is fairly light in terms of the amount of work it does though so I think it's OK for it to be eager.


// There is nothing to do if no ElectronExtensionWebContentsObserver is
// attached to the |web_contents|.
if (!web_observer)

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Feb 11, 2020

Member

The lifetime of the observer based on WebContentsUserData should be based on the lifetime of the WebContents , so the above check for if (!web_contents) would be good, this is redundant.

This comment has been minimized.

Copy link
@nornagon

nornagon Feb 12, 2020

Author Member

I'm not 100% sure that we never create a WebContents without an ElectronExtensionWebContentsObserver, so I think this check should stay...

(fwiw, I'm pretty sure we sometimes create content::WebContents without an electron::api::WebContents wrapper, and in that case we might not have created the observer I think?)

shell/browser/electron_browser_client.cc Outdated Show resolved Hide resolved
extensions::ProcessManager::Get(web_contents->GetBrowserContext())
->GetBackgroundHostForExtension(extension->id());
if (host) {
factories->emplace(url::kFileScheme, std::make_unique<FileURLLoaderFactory>(

This comment has been minimized.

Copy link
@deepak1556

This comment has been minimized.

Copy link
@nornagon

nornagon Feb 12, 2020

Author Member

I'm not sure... I copied this logic from Chrome so maybe? How would we tell?

shell/browser/plugins/plugin_utils.h Show resolved Hide resolved
info.name = base::UTF8ToUTF16("Chromium PDF Viewer");
info.path = base::FilePath::FromUTF8Unsafe(extension_misc::kPdfExtensionId);
info.background_color = content::WebPluginInfo::kDefaultBackgroundColor;
info.mime_types.emplace_back("application/pdf", "pdf",

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Feb 11, 2020

Member

would be good to keep the same constants between the utility class plugin declaration and here.

This comment has been minimized.

Copy link
@nornagon

nornagon Feb 12, 2020

Author Member

agree, I think Chrome does some other stuff to deduplicate this. I'll clean up in a followup.

w.loadURL(pdfSource)
const [, contents] = await emittedOnce(app, 'web-contents-created')
expect(contents.getURL()).to.equal('chrome-extension://mhjfbmdgcfjbbpaeojofohoefgiehjai/index.html')
await emittedOnce(contents, 'did-finish-load')

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Feb 11, 2020

Member

Is there a better signal or chrome api we can run to see if the pdf is actually loaded , maybe https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/resources/pdf/pdf_viewer.js;bpv=0;bpt=0;l=1521 ? did-finish-load on the webcontents wouldn't exactly denote that the extension rendered the pdf.

This comment has been minimized.

Copy link
@nornagon

nornagon Feb 11, 2020

Author Member

i tried a lot of things to try to get a better "loaded" signal but nothing i tried worked. the core issue is that the MimeHandlerGuestView is similar to a <webview>, and it doesn't forward any kind of messages out of the <embed>. the best idea i have is to make preloads work in extension pages and use that to add some hooks, but we don't have that capability yet.

This comment has been minimized.

Copy link
@ericshung

ericshung Feb 11, 2020

I'm currently having some issue loading larger PDF file. Is this somehow related?

This comment has been minimized.

Copy link
@nornagon

nornagon Feb 12, 2020

Author Member

@ericshung no, I don't think that's related, but I'm seeing some other issues with that PDF (including discovering a bug in Chromium itself: https://crbug.com/1051548). Thanks for the report.

@nornagon

This comment has been minimized.

Copy link
Member Author

nornagon commented Feb 13, 2020

There's still some work to do on this, but I think this is a good first pass and is ready to merge. I'll open issues for:

@nornagon nornagon merged commit d590f2e into master Feb 13, 2020
14 of 15 checks passed
14 of 15 checks passed
electron-woa-testing
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-woa-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
build-mac Workflow: build-mac
Details
electron-arm-testing Build #20200212.38 succeeded
Details
electron-arm64-testing Build #20200212.37 succeeded
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Feb 13, 2020

Release Notes Persisted

Restored support for pdfium-based PDF viewer.

@nornagon nornagon deleted the extensions-pdf branch Feb 13, 2020
nornagon added a commit that referenced this pull request Feb 13, 2020
@trop trop bot added merged/9-x-y and removed in-flight/9-x-y labels Feb 13, 2020
@sofianguy sofianguy added this to Fixed in 9.0.0-beta.2 in 9-x-y Feb 19, 2020
@tomsontom

This comment has been minimized.

Copy link

tomsontom commented Feb 19, 2020

I tried the beta.2 support but my PDFs are rendering in there! If I use the 10.0.0-nightly they do! I don't see any error message, ... so I'm not sure how I could debug why things work in the nightly but fail in 9.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
9-x-y
Fixed in 9.0.0-beta.2
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.