Adds a pref and a policy to decide if PDFs should always be opened externally.

Adds a pref and a policy to decide if PDFs should always be opened externally.

If set this pref disables the PDF viewer and instead download the file and
automatically opens the external PDF viewer.

UI for controlling the pref will be added in subsequent CL.

BUG=628014
TEST=unit_tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/d5e6f66e96a2a91700a169cc1afa66c0da46185d
Cr-Commit-Position: refs/heads/master@{#423189}

[pastarmovj, 2016-09-27 15:21:13]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 15:21:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 15:46:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 15:46:17]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[pastarmovj, 2016-09-28 07:26:35]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 07:26:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 07:48:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 07:48:27]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[pastarmovj, 2016-09-28 09:57:04]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 09:57:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 10:10:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 10:10:16]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[pastarmovj, 2016-09-28 10:15:03]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 10:15:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 10:30:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 10:30:15]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[pastarmovj, 2016-09-28 11:11:30]

Patchset #6 (id:100001) has been deleted

[pastarmovj, 2016-09-28 11:11:48]

The CQ bit was checked by pastarmovj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 11:11:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 11:22:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 11:22:58]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[pastarmovj, 2016-09-28 12:09:34]

Patchset #6 (id:50013) has been deleted

[pastarmovj, 2016-09-28 13:20:12]

pastarmovj@chromium.org changed reviewers:
+ asanka@chromium.org, bauerb@chromium.org

[pastarmovj, 2016-09-28 13:20:13]

asanka@chromium.org: Please review changes in 

downloads

bauerb@chromium.org: Please review changes in 

plugins

Thanks!
Julian

[pastarmovj, 2016-09-28 13:21:21]

pastarmovj@chromium.org changed reviewers:
+ mkwst@chromium.org

[pastarmovj, 2016-09-28 13:21:21]

mkwst@chromium.org: Please review changes in 

histograms.xml

[Mike West, 2016-09-28 13:24:01]

Hi! Sorry, I'm only in the metrics OWNERS file for Blink-side UseCounter changes
to histograms. You'll need a real owner. :(

[pastarmovj, 2016-09-28 13:29:50]

pastarmovj@chromium.org changed reviewers:
+ isherman@chromium.org
- mkwst@chromium.org

[pastarmovj, 2016-09-28 13:29:51]

Hi isherman, can you please review the change to histograms.xml.

Thanks,
Julian

[asanka, 2016-09-28 17:27:54]

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
chrome/browser/download/download_prefs.cc:353: if
(!override_adobe_version_check_for_tests_ &&
What should be the non-test behavior if |always_open_pdfs_externally| is set to
true and the Adobe reader is not up-to-date?

[Ilya Sherman, 2016-09-28 20:21:30]

histograms.xml lgtm

[pastarmovj, 2016-09-29 08:40:44]

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
chrome/browser/download/download_prefs.cc:353: if
(!override_adobe_version_check_for_tests_ &&
On 2016/09/28 17:27:53, asanka wrote:
> What should be the non-test behavior if |always_open_pdfs_externally| is set
to
> true and the Adobe reader is not up-to-date?

It will cause a regular file download and let the user open it manually when
finished. This only prevents Chrome from automatically running Adobe Acrobat
Reader upon download completion.

P.S. The windows try bots actually trip over this check so they might be at risk
;)

[Bernhard Bauer, 2016-09-29 08:44:35]

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:99: // Used by tests to switch off
version checks for Adobe.
Rename the method so it states that the checks are disabled?

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:123:
FRIEND_TEST_ALL_PREFIXES(DownloadPrefsTest, AlwaysOpenPdfExternally);
Friend declarations go first in the private section. Alternatively, just make
the method public? It has a ...ForTests suffix anyway, which I think should be
good enough to deter people from using it in production code.

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.cc:236: return POLICY_DISABLED;
Nit: I think it's more common to leave out braces for one-line bodies in this
file.

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.cc:241: } else if
(IsStringMatchedInSet(name, policy_disabled_plugin_patterns_) &&
Remove these else's?

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_prefs.h (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.h:121: void SetAlwaysOpenPdfExternally(bool
always_open_pdf_externally);
If this is for tests, add ...ForTesting.

https://codereview.chromium.org/2369353002/diff/130001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/common/pref_nam...
chrome/common/pref_names.cc:983: const char kAlwaysOpenPdfExternally[] =
"plugins.always_open_pdf_externally";
Can you name the constant in a way that reflects its value? I.e.
kPluginsAlwaysOpenPdfExternally.

[pastarmovj, 2016-09-29 12:14:00]

Description was changed from

==========
Adds a pref and a policy to decide if PDFs should always be opened externally.

If set this pref disables the PDF viewer and instead download the file and
automatically opens the external PDF viewer.

UI for controlling the pref will be added in subsequent CL.

BUG=628014
TEST=unit_tests
==========

to

==========
Adds a pref and a policy to decide if PDFs should always be opened externally.

If set this pref disables the PDF viewer and instead download the file and
automatically opens the external PDF viewer.

UI for controlling the pref will be added in subsequent CL.

BUG=628014
TEST=unit_tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[pastarmovj, 2016-09-29 12:14:41]

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:99: // Used by tests to switch off
version checks for Adobe.
On 2016/09/29 08:44:35, Bernhard Bauer wrote:
> Rename the method so it states that the checks are disabled?

Done.

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:123:
FRIEND_TEST_ALL_PREFIXES(DownloadPrefsTest, AlwaysOpenPdfExternally);
On 2016/09/29 08:44:35, Bernhard Bauer wrote:
> Friend declarations go first in the private section. Alternatively, just make
> the method public? It has a ...ForTests suffix anyway, which I think should be
> good enough to deter people from using it in production code.

Done.

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.cc:236: return POLICY_DISABLED;
On 2016/09/29 08:44:35, Bernhard Bauer wrote:
> Nit: I think it's more common to leave out braces for one-line bodies in this
> file.

Yup. Copy-paste relict.

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.cc:241: } else if
(IsStringMatchedInSet(name, policy_disabled_plugin_patterns_) &&
On 2016/09/29 08:44:35, Bernhard Bauer wrote:
> Remove these else's?

Done.

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_prefs.h (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.h:121: void SetAlwaysOpenPdfExternally(bool
always_open_pdf_externally);
On 2016/09/29 08:44:35, Bernhard Bauer wrote:
> If this is for tests, add ...ForTesting.

I kept it consistent with SetPolicyEnforcedPluginPatterns but since both are
only used in tests now I think this is fair change.

https://codereview.chromium.org/2369353002/diff/130001/chrome/common/pref_nam...
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/2369353002/diff/130001/chrome/common/pref_nam...
chrome/common/pref_names.cc:983: const char kAlwaysOpenPdfExternally[] =
"plugins.always_open_pdf_externally";
On 2016/09/29 08:44:35, Bernhard Bauer wrote:
> Can you name the constant in a way that reflects its value? I.e.
> kPluginsAlwaysOpenPdfExternally.

Done.

[Bernhard Bauer, 2016-09-30 09:03:28]

lgtm

[asanka, 2016-09-30 14:34:53]

Disregard the comment about download_prefs if download changes are deemed
unnecessary.

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:120: bool
disable_adobe_version_check_for_tests_;
Rather than disable the version check, can we force the test to succeed? I.e.
Make adobe_reader_info_win.h: IsAdobeReaderUptodate() and
IsAdobeReaderDefaultPDFViewer() overridable for testing?

This way the effects of the Adobe reader version check will be uniform rather
than just being overridden here.

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.cc:236: return POLICY_DISABLED;
This should propagate up to PluginService::GetPluginInfo(), correct? I.e. if
always_open_pdf_externally_ is true, then the PDF plugin would appear to all
callers of GetPluginInfo() as disabled?

If so, as far as the downloads code is concerned, the behavior should be
equivalent to disabling the PDF plugin. Is this not the case?

[pastarmovj, 2016-10-04 10:06:34]

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/downloa...
File chrome/browser/download/download_prefs.h (right):

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/downloa...
chrome/browser/download/download_prefs.h:120: bool
disable_adobe_version_check_for_tests_;
On 2016/09/30 14:34:53, asanka wrote:
> Rather than disable the version check, can we force the test to succeed? I.e.
> Make adobe_reader_info_win.h: IsAdobeReaderUptodate() and
> IsAdobeReaderDefaultPDFViewer() overridable for testing?
> 
> This way the effects of the Adobe reader version check will be uniform rather
> than just being overridden here. 

I agree with your comment but for the sake of expediting this CL into the
current version and due to lack of other direct use cases of this I would like
to make this work a separate issue and filed crbug/652624 are you ok with that?

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.cc:236: return POLICY_DISABLED;
On 2016/09/30 14:34:53, asanka wrote:
> This should propagate up to PluginService::GetPluginInfo(), correct? I.e. if
> always_open_pdf_externally_ is true, then the PDF plugin would appear to all
> callers of GetPluginInfo() as disabled?
> 
> If so, as far as the downloads code is concerned, the behavior should be
> equivalent to disabling the PDF plugin. Is this not the case?

I am not sure I understand the question but the general idea is that if the
internal PDF viewer is disabled Chrome should thread PDF files as regular
downloads but also fast-track opening them externally if possible thus reducing
the friction of using an external PDF program.  The two parts of this CL work
together to achieve that - we first make sure the internal plugin is disabled
which then makes the pdf mime type slip into the regular download code and
eventually enable the automatic opening of the download if the policy has been
enabled.

[pastarmovj, 2016-10-05 11:01:20]

Asanka, gentle ping?

[asanka, 2016-10-05 15:18:27]

lgtm

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.cc:236: return POLICY_DISABLED;
On 2016/10/04 at 10:06:34, pastarmovj wrote:
> On 2016/09/30 14:34:53, asanka wrote:
> > This should propagate up to PluginService::GetPluginInfo(), correct? I.e. if
> > always_open_pdf_externally_ is true, then the PDF plugin would appear to all
> > callers of GetPluginInfo() as disabled?
> > 
> > If so, as far as the downloads code is concerned, the behavior should be
> > equivalent to disabling the PDF plugin. Is this not the case?
> 
> I am not sure I understand the question but the general idea is that if the
internal PDF viewer is disabled Chrome should thread PDF files as regular
downloads but also fast-track opening them externally if possible thus reducing
the friction of using an external PDF program.  The two parts of this CL work
together to achieve that - we first make sure the internal plugin is disabled
which then makes the pdf mime type slip into the regular download code and
eventually enable the automatic opening of the download if the policy has been
enabled.

Once the PDF plugin is disabled, the downloads code should automatically treat
it as a regular download. The determination of whether a PDF should be opened in
the browser takes into consideration whether there is a sandboxed plugin
available to open the MIME type
(https://cs.chromium.org/chromium/src/chrome/browser/download/download_target_...).

Is this not sufficient? Are there issues with how PDFs are handled when the
plugin is disabled via 'aways open PDF externally' in addition to not triggering
the auto-open logic?

[pastarmovj, 2016-10-05 15:32:25]

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/plugins...
File chrome/browser/plugins/plugin_prefs.cc (right):

https://codereview.chromium.org/2369353002/diff/150001/chrome/browser/plugins...
chrome/browser/plugins/plugin_prefs.cc:236: return POLICY_DISABLED;
On 2016/10/05 15:18:27, asanka wrote:
> On 2016/10/04 at 10:06:34, pastarmovj wrote:
> > On 2016/09/30 14:34:53, asanka wrote:
> > > This should propagate up to PluginService::GetPluginInfo(), correct? I.e.
if
> > > always_open_pdf_externally_ is true, then the PDF plugin would appear to
all
> > > callers of GetPluginInfo() as disabled?
> > > 
> > > If so, as far as the downloads code is concerned, the behavior should be
> > > equivalent to disabling the PDF plugin. Is this not the case?
> > 
> > I am not sure I understand the question but the general idea is that if the
> internal PDF viewer is disabled Chrome should thread PDF files as regular
> downloads but also fast-track opening them externally if possible thus
reducing
> the friction of using an external PDF program.  The two parts of this CL work
> together to achieve that - we first make sure the internal plugin is disabled
> which then makes the pdf mime type slip into the regular download code and
> eventually enable the automatic opening of the download if the policy has been
> enabled.
> 
> Once the PDF plugin is disabled, the downloads code should automatically treat
> it as a regular download. The determination of whether a PDF should be opened
in
> the browser takes into consideration whether there is a sandboxed plugin
> available to open the MIME type
>
(https://cs.chromium.org/chromium/src/chrome/browser/download/download_target_...).
> 
> Is this not sufficient? Are there issues with how PDFs are handled when the
> plugin is disabled via 'aways open PDF externally' in addition to not
triggering
> the auto-open logic?

Followed up as discussed on a new bug
https://bugs.chromium.org/p/chromium/issues/detail?id=653120

[pastarmovj, 2016-10-05 15:32:35]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2016-10-05 15:32:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2369353002/#ps150001
(title: "Address comments.")

[commit-bot: I haz the power, 2016-10-05 15:32:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 15:44:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-05 15:44:33]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pastarmovj, 2016-10-05 15:46:09]

The CQ bit was checked by pastarmovj@chromium.org

[pastarmovj, 2016-10-05 15:46:09]

The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org,
bauerb@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2369353002/#ps170001
(title: "Remove changes leaked in from the dependent branch.")

[commit-bot: I haz the power, 2016-10-05 15:46:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 16:52:37]

Committed patchset #8 (id:170001)

[commit-bot: I haz the power, 2016-10-05 16:56:56]

Description was changed from

==========
Adds a pref and a policy to decide if PDFs should always be opened externally.

If set this pref disables the PDF viewer and instead download the file and
automatically opens the external PDF viewer.

UI for controlling the pref will be added in subsequent CL.

BUG=628014
TEST=unit_tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Adds a pref and a policy to decide if PDFs should always be opened externally.

If set this pref disables the PDF viewer and instead download the file and
automatically opens the external PDF viewer.

UI for controlling the pref will be added in subsequent CL.

BUG=628014
TEST=unit_tests
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/d5e6f66e96a2a91700a169cc1afa66c0da46185d
Cr-Commit-Position: refs/heads/master@{#423189}
==========

[commit-bot: I haz the power, 2016-10-05 16:56:58]

Patchset 8 (id:??) landed as
https://crrev.com/d5e6f66e96a2a91700a169cc1afa66c0da46185d
Cr-Commit-Position: refs/heads/master@{#423189}