Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfeat: enable pdf viewer #21794
feat: enable pdf viewer #21794
Conversation
shell/browser/extensions/electron_component_extension_resource_manager.cc
Show resolved
Hide resolved
shell/browser/plugins/plugin_response_interceptor_url_loader_throttle.cc
Outdated
Show resolved
Hide resolved
|
LGTM I am good with the merge, can make changes in a follow up PR as the diff is quite big. |
| // 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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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?)
| extensions::ProcessManager::Get(web_contents->GetBrowserContext()) | ||
| ->GetBackgroundHostForExtension(extension->id()); | ||
| if (host) { | ||
| factories->emplace(url::kFileScheme, std::make_unique<FileURLLoaderFactory>( |
This comment has been minimized.
This comment has been minimized.
deepak1556
Feb 11, 2020
Member
Will this override the file url loader handled by the network service https://source.chromium.org/chromium/chromium/src/+/master:content/browser/frame_host/render_frame_host_impl.cc;l=5594 ?
This comment has been minimized.
This comment has been minimized.
nornagon
Feb 12, 2020
Author
Member
I'm not sure... I copied this logic from Chrome so maybe? How would we tell?
| 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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
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.
This comment has been minimized.
ericshung
Feb 11, 2020
I'm currently having some issue loading larger PDF file. Is this somehow related?
This comment has been minimized.
This comment has been minimized.
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.
|
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:
|
d590f2e
into
master
release-clerk
bot
commented
Feb 13, 2020
|
Release Notes Persisted
|
tomsontom
commented
Feb 19, 2020
•
|
I tried the beta.2 support but my PDFs are NOT 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 |
exentrich
commented
Feb 24, 2020
|
@tomsontom Same issue |
|
See #22286 |
rgilotra
commented
Mar 26, 2020
•
|
Hi I am posting this comment w.r.t. nightly version 10.x. I see PDF viewer working in development mode. but as soon as it is packaged it reflects same issue "Save As dialog" box as with stable version 8.x. |
nornagon commentedJan 16, 2020
•
edited
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
npm testpassesRelease Notes
Notes: Restored support for pdfium-based PDF viewer.