-
Notifications
You must be signed in to change notification settings - Fork 868
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
Use pdfium #2342
Use pdfium #2342
Conversation
* License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
* You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
||
#include "base/path_service.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we planning to re-introduce navigator.plugins
? I think updating the test makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jumde I deleted existing tests because this PR deleted all related codes and went back to chromium's original state. If we want to control exposed plugin/mimetype list strictly, I think we need to re-implement instead of previous BravePluginRegistryImpl
way. (See #2128 (comment)). WDYT?
Did some quick tests - looks great! 😄 Will create a test plan and update original post here Besides creating a security review (which I can help with, when ready) one of the big TODO's (several called out on brave/brave-browser#3846) would be removing the existing PDF.js install for users (in favor of PDFium) |
Verifications:
|
Pushed commit for fixing brave/brave-browser#3694. forgot to add. :) |
Hmm, brave/brave-browser#1264 is still reproduced and I checked that our current stable(0.63.48) have same issue. |
@simonhong weird- it works for me on Nightly. But perhaps that hints to why. Does brave/brave-browser#1264 still happen with your latest fix? |
Yes, still happening. |
@bsclifton I'm testing on Mac and Nightly and Stable on my local have same issue... strange.. |
@bsclifton http://www.fina.org/content/swimming-world-ranking seems shields issue. When shields off, it displays well. Third party cookie block causes this issue. |
Yes, you're right. This PR should include logic for deleting PDF.js. I'll take a look that, too now. |
browser/net/brave_common_static_redirect_network_delegate_helper.cc
Outdated
Show resolved
Hide resolved
Note to self: will look into the removal of PDF.js (and making sure it can be installed) |
b7bc646
to
c600e87
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created a test plan a while back - added some more and ran through it. Things look great!
Waiting until go-update
change is deployed and security review happens before approving
please remember to update |
Update to https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode will be needed post merge for plugins and mime types. https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove) - Need to be updated to remove any references to PDF.js |
So far, brave only exposes flash when it is available. Brave will use pdfium as a pdf viewer instead of pdf.js. That causes plugins and mimeTypes will include pdf things. That means we expose all that brave have. So, we don't need to blacklist except flash plugins.
Some google sites check pdf plugin name and do a different behavior whether browser has chrome pdf plugin or not. Ex, google docs print menu displays print preview when chrome pdf plugin is available. Otherwise, just tries to download that documents as a pdf document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested out, works great 😄 Filled in test plan and re-tested / tagged all bugs this closes
Updating the docs now... |
Updated https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove) @jumde I didn't see what to edit on https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode - can you help me update this? |
@@ -19,9 +19,9 @@ const char ChromeContentClient::kPDFExtensionPluginName[] = "Chrome PDF Viewer"; | ||
const char ChromeContentClient::kPDFInternalPluginName[] = "Chrome PDF Plugin"; | ||
#else | ||
const char ChromeContentClient::kPDFExtensionPluginName[] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be done with a chromium_src override
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes brave/brave-browser#3846
Fixes brave/brave-browser#3694
Fixes brave/brave-browser#884
Fixes brave/brave-browser#3119
Fixes brave/brave-browser#4424
To use pdfium,
PluginRegistry::GetPlugins()
must include pdfium plugins.Built-in pdf plugins and flash are all that we can expose via
navigator.plugins
andmimeTypes
.So
BravePluginRegistryImpl
is deleted.Submitter Checklist:
npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security
) onnpm run lint
)git rebase master
(if needed).git rebase -i
to squash commits (if needed).Test Plan:
Test fix for brave/brave-browser#3694
Test fix for brave/brave-browser#884
Test fix for brave/brave-browser#4424
Embed test (possible fix for brave/brave-browser#3119)
This browser supports inline PDFs
should be shown (green color)Test that PDF.js is still installable
Test that PDF.js does not show
--show-component-extension-options
Reviewer Checklist:
After-merge Checklist:
changes has landed on.