Add option to open PDFs in system viewer to OS X and Linux.

Add option to open PDFs in system viewer to OS X and Linux.

Not just Windows.

BUG=370746
TEST=With a fresh profile, download a PDF. In the Download Shelf, select
Always Open With System Viewer. Check that subsequent downloads of PDFs
launch the system PDF viewer. (On Corp Macs, that is defined to be
Chrome; change it to Preview.app.)

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

[palmer, 2014-10-21 00:38:40]

palmer@chromium.org changed reviewers:
+ asanka@chromium.org, rdsmith@chromium.org, thestig@chromium.org

[palmer, 2014-10-21 00:38:41]

asanka and rdsmith: OWNERS for chrome/browser/download/OWNERS.

thestig: To make sure you agree this matches your intent on Windows, close
enough.

[Lei Zhang, 2014-10-21 11:07:06]

Looks like this matches my intent on Windows.

https://codereview.chromium.org/665163005/diff/20001/chrome/browser/download/...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/665163005/diff/20001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:722: #if
defined(OS_MACOSX) || defined(OS_LINUX)
I'm not 100% sure what pieces of code Chrome for iOS uses, but just to note,
OS_MACOSX is true for iOS.

[Randy Smith (Not in Mondays), 2014-10-21 15:02:07]

Do we need to make this system specific?  I'm slightly allergic to platform
ifdefs, and was feeling a bit sniffly reading the code.

[asanka, 2014-10-21 16:18:43]

On 2014/10/21 15:02:07, rdsmith wrote:
> Do we need to make this system specific?  I'm slightly allergic to platform
> ifdefs, and was feeling a bit sniffly reading the code.

+1. Yeah. I'm also thinking that the ShouldOpenPDFsInSystemViewer setting should
be cross platform. Having both "Open in Adobe Reader" and "Open in System
viewer" preferences can be a bit confusing for a first time reader. Also what
about non-Adobe PDF viewers on Windows (though probably not a big use case).

How about something like the following?:

Configuration                             | Action when Open in System Viewer is
true
------------------------------------------+-----------------------------------------
Windows (Adobe Reader)                    | Open in Adobe Reader
Windows (A.R. out-of-date)                | Open in Chrome
Windows (not Adobe Reader)                | Open in system viewer
Mac                                       | Open in system viewer
Linux                                     | Open in system viewer

When Open In System Viewer is false, we just open in Chrome. No ambiguity there.

I.e.:
* If we can tell that the reader is Adobe:
  - The preference is called 'Open in Adobe Reader'
  - It will only be honored if Reader is up-to-date.

* Else:
  - The preference is called 'Open in system viewer'
  - It will be honored regardless of whether the viewer is up-to-date or not
since we can't tell the difference.

On Windows we are able to tell if the system viewer is Adobe and it's
up-to-date. So we were able to ask a more specific question ("Always open in
Adobe Reader"). But since that's what we've been asking, would there be concerns
with making the preference apply generally? This would only make a difference if
the user makes another application be the default PDF viewer.

WDYT?

[palmer, 2014-10-21 19:23:31]

Yes, that all sounds good and I'll work on it now.

[palmer, 2014-10-21 20:31:49]

https://codereview.chromium.org/665163005/diff/20001/chrome/browser/download/...
File chrome/browser/download/chrome_download_manager_delegate.cc (right):

https://codereview.chromium.org/665163005/diff/20001/chrome/browser/download/...
chrome/browser/download/chrome_download_manager_delegate.cc:722: #if
defined(OS_MACOSX) || defined(OS_LINUX)
On 2014/10/21 11:07:06, Lei Zhang wrote:
> I'm not 100% sure what pieces of code Chrome for iOS uses, but just to note,
> OS_MACOSX is true for iOS.

OK, checking for defined(OS_MACOSX) && !defined(OS_IOS).

[palmer, 2014-10-21 20:52:50]

OK, see what you think now. There's less #ifdef'ing. Thanks!

[asanka, 2014-10-23 07:38:04]

This seems to not handle non-'Adobe Reader' PDF viewers on Windows. Is that
intentional?

https://codereview.chromium.org/665163005/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/665163005/diff/40001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:408: bool
DownloadShelfContextMenu::CanOpenPdfInReader() const {
CanOpenPdfInSystemViewer?

https://codereview.chromium.org/665163005/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/665163005/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:1469: "download.open_pdf_in_adobe_reader";
This change has the effect of resetting the "Open in <...>" preference for PDFs.
I recall having to add migration code when this cropped up before so that users
who have explicitly set their preference won't notice any change in behavior.

[palmer, 2014-10-23 22:17:08]

https://codereview.chromium.org/665163005/diff/40001/chrome/browser/download/...
File chrome/browser/download/download_shelf_context_menu.cc (right):

https://codereview.chromium.org/665163005/diff/40001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:408: bool
DownloadShelfContextMenu::CanOpenPdfInReader() const {
> CanOpenPdfInSystemViewer?

OK, changed the name.

https://codereview.chromium.org/665163005/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/665163005/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:1469: "download.open_pdf_in_adobe_reader";
> This change has the effect of resetting the "Open in <...>" preference for
PDFs.
> I recall having to add migration code when this cropped up before so that
users
> who have explicitly set their preference won't notice any change in behavior.

Should leave the name the same for Windows? Or have both names, and make them
synonyms?

[palmer, 2014-10-23 22:37:53]

> This seems to not handle non-'Adobe Reader' PDF viewers on Windows. Is that
> intentional?

Nope, not intentional. I think I handled this in the most recent patchset. Let
me know if you see another area that needs fixing.

[asanka, 2014-10-28 21:32:51]

https://codereview.chromium.org/665163005/diff/80001/chrome/browser/download/...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/665163005/diff/80001/chrome/browser/download/...
chrome/browser/download/download_prefs.cc:348: if
(!DownloadTargetDeterminer::IsAdobeReaderUpToDate())
if (<is adobe the default reader> && IsAdobeReaderUpToDate())

For the case where Adobe reader is not the default, DTD::IsAdobeReaderUpToDate()
is always false.

[palmer, 2014-10-28 22:38:39]

https://codereview.chromium.org/665163005/diff/80001/chrome/browser/download/...
File chrome/browser/download/download_prefs.cc (right):

https://codereview.chromium.org/665163005/diff/80001/chrome/browser/download/...
chrome/browser/download/download_prefs.cc:348: if
(!DownloadTargetDeterminer::IsAdobeReaderUpToDate())
On 2014/10/28 21:32:50, asanka wrote:
> if (<is adobe the default reader> && IsAdobeReaderUpToDate())
> 
> For the case where Adobe reader is not the default,
DTD::IsAdobeReaderUpToDate()
> is always false.

Done.

[asanka, 2014-10-31 18:07:45]

lgtm

I'd suggest manual testing and adding a few TEST= notes for QA folks where
appropriate for the cases I've outlined earlier.

[palmer, 2014-11-07 20:33:14]

The CQ bit was checked by palmer@chromium.org

[commit-bot: I haz the power, 2014-11-07 20:34:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/665163005/120001

[commit-bot: I haz the power, 2014-11-07 21:38:18]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-11-07 21:39:45]

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