Print Preview: Always enable the internal PDF plugin for print preview.

Print Preview: Always enable the internal PDF plugin for print preview.

BUG=76442
TEST=Adobe PDF plugin is enabled, and print preview still works.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96918

[Lei Zhang, 2011-08-15 21:26:33]

+ the right account

[jam, 2011-08-15 22:03:04]

http://codereview.chromium.org/7649026/diff/1/content/browser/plugin_service.cc
File content/browser/plugin_service.cc (right):

http://codereview.chromium.org/7649026/diff/1/content/browser/plugin_service....
content/browser/plugin_service.cc:365: url.SchemeIs(chrome::kChromeUIScheme))) {
I believe you're doing this because the url for the pdf changes depending on the
print settings. but this doesn't match the concept of overriding a plugin for a
url. it looks like you're just reusing that code, but changing the behavior
(since now any overriden plugin applies for any chrome ui url)

perhaps you want to change the documentation for OverriddenPlugin so that if
passed in url is empty, it applies to all urls in that tab?

[Lei Zhang, 2011-08-16 00:16:41]

http://codereview.chromium.org/7649026/diff/1/content/browser/plugin_service.cc
File content/browser/plugin_service.cc (right):

http://codereview.chromium.org/7649026/diff/1/content/browser/plugin_service....
content/browser/plugin_service.cc:365: url.SchemeIs(chrome::kChromeUIScheme))) {
On 2011/08/15 22:03:04, John Abd-El-Malek wrote:
> I believe you're doing this because the url for the pdf changes depending on
the
> print settings. but this doesn't match the concept of overriding a plugin for
a
> url. it looks like you're just reusing that code, but changing the behavior
> (since now any overriden plugin applies for any chrome ui url)
> 
> perhaps you want to change the documentation for OverriddenPlugin so that if
> passed in url is empty, it applies to all urls in that tab?

Done

[jam, 2011-08-16 00:45:53]

lgtm with nit

http://codereview.chromium.org/7649026/diff/7002/content/browser/plugin_servi...
File content/browser/plugin_service.h (right):

http://codereview.chromium.org/7649026/diff/7002/content/browser/plugin_servi...
content/browser/plugin_service.h:104: // Safe to be called from any thread. If
|plugin| has an empty |url| field,
nit: this comment belongs above OverriddenPlugin::url. Also, to be precise it
should say "render view" instead of "tab", since if the user navigates in that
tab to a pdf, we'll still use reader.

[Lei Zhang, 2011-08-16 01:12:02]

On 2011/08/16 00:45:53, John Abd-El-Malek wrote:
> lgtm with nit
> 
>
http://codereview.chromium.org/7649026/diff/7002/content/browser/plugin_servi...
> File content/browser/plugin_service.h (right):
> 
>
http://codereview.chromium.org/7649026/diff/7002/content/browser/plugin_servi...
> content/browser/plugin_service.h:104: // Safe to be called from any thread. If
> |plugin| has an empty |url| field,
> nit: this comment belongs above OverriddenPlugin::url. Also, to be precise it
> should say "render view" instead of "tab", since if the user navigates in that
> tab to a pdf, we'll still use reader.

Done.

[commit-bot: I haz the power, 2011-08-16 11:22:59]

Change committed as 96918