Windows: Add an "Open in Adobe Reader" menu item for PDF files in the download shelf.

Windows: Add an "Open in Adobe Reader" menu item for PDF files in the download shelf.

BUG=370746
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281172

[Lei Zhang, 2014-06-09 09:04:46]

Not 100% ready, but PTAL and let me know if you have any concerns.

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

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:153:
prefs->EnableAutoOpenBasedOnExtension(path);
I still need to implement some special logic to implement requirement (3),
probably with a separate pref to keep track of whether this is for .pdf + secure
reader, and resetting the checkbox if reader gets uninstalled.

[jam, 2014-06-09 19:49:10]

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

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:52: #if
defined(ENABLE_PLUGIN_INSTALLATION)
nit: OS_WIN implies ENABLE_PLUGIN_INSTALLATION...

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:56:
GetAdobeReaderPluginInfo(NULL, &plugin_info);
this won't work after we remove NPAPI, which is what's motivating this feature.
right now the classification of whether Reader plugin is out of date or not
comes from a json file that is part of chrome and is updated by component
updater. for this one-off use case, i think it's fine to hard code the versions
of Reader that are out-of-date in a cc file. if we really need to update it
immediately, we can do a push then.

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:85: #endif  //
defined(ENABLE_PLUGIN_INSTALLATION)
nit: move the anonymous namespace inside the ifdef and then you can combine both
of these ifdefs?

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
File chrome/browser/ui/pdf/adobe_reader_info_win.h (right):

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.h:38: bool
GetAdobeReaderPluginInfo(content::WebContents* web_contents,
nit: above and here, i think it would be clearer if this took a Profile* or
BrowserContext* instead of a WebContents. That way it's clear that the enabled
flag comes from the profile, and isn't a per-tab thing

[asanka, 2014-06-09 21:22:23]

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

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:368: int
DownloadShelfContextMenu::GetAlwaysOpenCommandId() const {
Nit: This returns a string ID, not a command ID.

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:73: }
What about is_default_plugin_for_pdf ? My cursory reading is that if more than
one plugin handles application/pdf, then the plugin returned by
PluginService::GetPluginInfo() may not be the Adobe Reader plugin. Thus the
plugin used to open application/pdf in-browser may not be Adobe Reader.

If the default plugin is PDFium Chrome would decide to open .pdf files
in-browser using PDFium, contradicting the menu option being introduced.

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:119: app_cmd.find(kReaderKey) !=
base::string16::npos);
Shouldn't this also check HKCR\.pdf ? .pdf might be getting mapped to something
other than AcroExch.Document.

[Lei Zhang, 2014-06-12 05:04:26]

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

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:52: #if
defined(ENABLE_PLUGIN_INSTALLATION)
On 2014/06/09 19:49:09, jam wrote:
> nit: OS_WIN implies ENABLE_PLUGIN_INSTALLATION...

The PDF UI code we started with has defined(OS_WIN) &&
defined(ENABLE_PLUGIN_INSTALLATION), but I'm happy to simply both, as I doubt
anyone builds with OS_WIN but not ENABLE_PLUGIN_INSTALLATION.

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:56:
GetAdobeReaderPluginInfo(NULL, &plugin_info);
On 2014/06/09 19:49:09, jam wrote:
> this won't work after we remove NPAPI, which is what's motivating this
feature.
> right now the classification of whether Reader plugin is out of date or not
> comes from a json file that is part of chrome and is updated by component
> updater. for this one-off use case, i think it's fine to hard code the
versions
> of Reader that are out-of-date in a cc file. if we really need to update it
> immediately, we can do a push then.

Ok, I changed this to check the version of the reader EXE file on disk. I didn't
find a good way to get the version number out of the registry. :\

Since we have to get it from disk and we are on the UI thread here, I made
DownloadPref do a check once at initialization time and cache the result
globally.

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:368: int
DownloadShelfContextMenu::GetAlwaysOpenCommandId() const {
On 2014/06/09 21:22:22, asanka wrote:
> Nit: This returns a string ID, not a command ID.

Done.

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:73: }
On 2014/06/09 21:22:22, asanka wrote:
> What about is_default_plugin_for_pdf ? My cursory reading is that if more than
> one plugin handles application/pdf, then the plugin returned by
> PluginService::GetPluginInfo() may not be the Adobe Reader plugin. Thus the
> plugin used to open application/pdf in-browser may not be Adobe Reader.
> 
> If the default plugin is PDFium Chrome would decide to open .pdf files
> in-browser using PDFium, contradicting the menu option being introduced.

We check |kAdobeReaderIdentifier| above. If it never matches, then is_installed
will be false.

This brings up another question: does the "always open in reader" checkbox
affect downloads only or does it also affect PDFs being viewed in the browser?
For patch set 3, I'm assuming "always open in reader" means always open in the
external viewer. Thus I'm disabling both the built-in and Acrobat PDF plugins.
Perhaps we want to disable all plugins that handle application/pdf?

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:85: #endif  //
defined(ENABLE_PLUGIN_INSTALLATION)
On 2014/06/09 19:49:09, jam wrote:
> nit: move the anonymous namespace inside the ifdef and then you can combine
both
> of these ifdefs?

I removed the ENABLE_PLUGIN_INSTALLATION) #ifdefs per your other comment.

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:119: app_cmd.find(kReaderKey) !=
base::string16::npos);
On 2014/06/09 21:22:22, asanka wrote:
> Shouldn't this also check HKCR\.pdf ? .pdf might be getting mapped to
something
> other than AcroExch.Document.

Sure. I improved the detection here.

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
File chrome/browser/ui/pdf/adobe_reader_info_win.h (right):

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.h:38: bool
GetAdobeReaderPluginInfo(content::WebContents* web_contents,
On 2014/06/09 19:49:09, jam wrote:
> nit: above and here, i think it would be clearer if this took a Profile* or
> BrowserContext* instead of a WebContents. That way it's clear that the enabled
> flag comes from the profile, and isn't a per-tab thing

Done.

[asanka, 2014-06-12 16:50:55]

For background, the current download open behavior is roughly:

1. When determining the filename for a download, we check if the file can be
opened safely in the browser. The check is implemented in
DownloadTargetDeterminer::DoDetermineIfHandledSafely(). A file is considered to
be handled safely in the browser if visiting the file:// URL results in the
content being rendered by the renderer or using a sandboxed Pepper plugin.

2. Opening of downloads is handled by
ChromeDownloadManagerDelegate::OpenDownload(). It will open the download in the
browser (by visiting the file:// URL in a new tab) if the download is considered
to be handled safely AND the file type is in a whitelist (see
IsOpenInBrowserPreferreredForFile()). As a result PDFs open in browser if the
PDFium plugin is enabled.

3. If the user wishes to open it using the OS, then DownloadShelfContextMenu
invokes ChromeDownloadManagerDelegate::OpenDownloadUsingPlatformHandler().

That said:

It sounds like the desired "Open in Adobe Reader" behavior is (assuming this is
to deal with the depreciation of NPAPI):
- Force all application/pdf resources to be downloaded (requires disabling all
plugins that handle application/pdf or blacklisting application/pdf MIME  type
to minimize collateral).
- Open downloaded .pdf files in external Adobe Reader application. (from the
downloads side, we get this behavior for free if browser plugins are disabled
and Adobe Reader is the default application for handling .pdf files).
- Conditionalize the above behavior on a) Adobe Reader being up-to-date AND b)
it being the default handler for .pdf files.

I'm not sure about disabling plugins in response to a download preference since
it affects how all resources with associated MIME types are handled, not just
downloads. If that's the intended behavior then perhaps that pref should live
someplace else where its scope is more apparent. Maybe in PluginPrefs?

One option is for the plugin discovery phase to detect a) and b) above. Then
based on a preference to use Adobe Reader, block browser plugins from handling
application/pdf in ChromePluginServiceFilter. WDYT?

I'm still a bit iffy on the UX for this since the UI for controlling this is
currently planned to be downloads specific even though the scope of the behavior
isn't limited to downloads. Also the handling of PDFs can change at arbitrary
points in time without explanation due to Adobe Reader going out of date.

Apologies if I'm distracting this CL, but I want to make sure we have a good
design.

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

https://codereview.chromium.org/324593004/diff/60001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:385: return
path.Extension() == FILE_PATH_LITERAL(".pdf");
Nit: path.MatchesExtension(...)

https://codereview.chromium.org/324593004/diff/60001/chrome/browser/ui/pdf/ad...
File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):

https://codereview.chromium.org/324593004/diff/60001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:101: return false;
Have you looked at using AssocQueryString() to get at the executable name for
the 'open' verb for .pdfs? I think we are already pulling in Shlwapi.dll, so
might as well use it.

[jam, 2014-06-12 18:18:18]

On 2014/06/12 16:50:55, asanka wrote:
> For background, the current download open behavior is roughly:
> 
> 1. When determining the filename for a download, we check if the file can be
> opened safely in the browser. The check is implemented in
> DownloadTargetDeterminer::DoDetermineIfHandledSafely(). A file is considered
to
> be handled safely in the browser if visiting the file:// URL results in the
> content being rendered by the renderer or using a sandboxed Pepper plugin.
> 
> 2. Opening of downloads is handled by
> ChromeDownloadManagerDelegate::OpenDownload(). It will open the download in
the
> browser (by visiting the file:// URL in a new tab) if the download is
considered
> to be handled safely AND the file type is in a whitelist (see
> IsOpenInBrowserPreferreredForFile()). As a result PDFs open in browser if the
> PDFium plugin is enabled.
> 
> 3. If the user wishes to open it using the OS, then DownloadShelfContextMenu
> invokes ChromeDownloadManagerDelegate::OpenDownloadUsingPlatformHandler().
> 
> That said:
> 
> It sounds like the desired "Open in Adobe Reader" behavior is (assuming this
is
> to deal with the depreciation of NPAPI):
> - Force all application/pdf resources to be downloaded (requires disabling all
> plugins that handle application/pdf or blacklisting application/pdf MIME  type
> to minimize collateral).

The goal is that when the user downloads a pdf, they can click "open" and it
opens up Adobe Reader.

The goal is not that all viewed pdfs should open up in Adobe Reader.

> - Open downloaded .pdf files in external Adobe Reader application. (from the
> downloads side, we get this behavior for free if browser plugins are disabled
> and Adobe Reader is the default application for handling .pdf files).
> - Conditionalize the above behavior on a) Adobe Reader being up-to-date AND b)
> it being the default handler for .pdf files.
> 
> I'm not sure about disabling plugins in response to a download preference
since
> it affects how all resources with associated MIME types are handled, not just
> downloads. If that's the intended behavior then perhaps that pref should live
> someplace else where its scope is more apparent. Maybe in PluginPrefs?
> 
> One option is for the plugin discovery phase to detect a) and b) above. Then
> based on a preference to use Adobe Reader, block browser plugins from handling
> application/pdf in ChromePluginServiceFilter. WDYT?
> 
> I'm still a bit iffy on the UX for this since the UI for controlling this is
> currently planned to be downloads specific even though the scope of the
behavior
> isn't limited to downloads. Also the handling of PDFs can change at arbitrary
> points in time without explanation due to Adobe Reader going out of date.
> 
> Apologies if I'm distracting this CL, but I want to make sure we have a good
> design.
> 
>
https://codereview.chromium.org/324593004/diff/60001/chrome/browser/download/...
> File chrome/browser/download/download_shelf_context_menu.cc (right):
> 
>
https://codereview.chromium.org/324593004/diff/60001/chrome/browser/download/...
> chrome/browser/download/download_shelf_context_menu.cc:385: return
> path.Extension() == FILE_PATH_LITERAL(".pdf");
> Nit: path.MatchesExtension(...)
> 
>
https://codereview.chromium.org/324593004/diff/60001/chrome/browser/ui/pdf/ad...
> File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):
> 
>
https://codereview.chromium.org/324593004/diff/60001/chrome/browser/ui/pdf/ad...
> chrome/browser/ui/pdf/adobe_reader_info_win.cc:101: return false;
> Have you looked at using AssocQueryString() to get at the executable name for
> the 'open' verb for .pdfs? I think we are already pulling in Shlwapi.dll, so
> might as well use it.

[jam, 2014-06-12 18:19:01]

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):

https://codereview.chromium.org/324593004/diff/20001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:73: }
On 2014/06/12 05:04:26, Lei Zhang wrote:
> On 2014/06/09 21:22:22, asanka wrote:
> > What about is_default_plugin_for_pdf ? My cursory reading is that if more
than
> > one plugin handles application/pdf, then the plugin returned by
> > PluginService::GetPluginInfo() may not be the Adobe Reader plugin. Thus the
> > plugin used to open application/pdf in-browser may not be Adobe Reader.
> > 
> > If the default plugin is PDFium Chrome would decide to open .pdf files
> > in-browser using PDFium, contradicting the menu option being introduced.
> 
> We check |kAdobeReaderIdentifier| above. If it never matches, then
is_installed
> will be false.
> 
> This brings up another question: does the "always open in reader" checkbox
> affect downloads only or does it also affect PDFs being viewed in the browser?
> For patch set 3, I'm assuming "always open in reader" means always open in the
> external viewer. Thus I'm disabling both the built-in and Acrobat PDF plugins.
> Perhaps we want to disable all plugins that handle application/pdf?

It should only affect downloads.

[asanka, 2014-06-12 21:46:14]

On 2014/06/12 18:18:18, jam wrote:
> On 2014/06/12 16:50:55, asanka wrote:
> > For background, the current download open behavior is roughly:
> > 
> > 1. When determining the filename for a download, we check if the file can be
> > opened safely in the browser. The check is implemented in
> > DownloadTargetDeterminer::DoDetermineIfHandledSafely(). A file is considered
> to
> > be handled safely in the browser if visiting the file:// URL results in the
> > content being rendered by the renderer or using a sandboxed Pepper plugin.
> > 
> > 2. Opening of downloads is handled by
> > ChromeDownloadManagerDelegate::OpenDownload(). It will open the download in
> the
> > browser (by visiting the file:// URL in a new tab) if the download is
> considered
> > to be handled safely AND the file type is in a whitelist (see
> > IsOpenInBrowserPreferreredForFile()). As a result PDFs open in browser if
the
> > PDFium plugin is enabled.
> > 
> > 3. If the user wishes to open it using the OS, then DownloadShelfContextMenu
> > invokes ChromeDownloadManagerDelegate::OpenDownloadUsingPlatformHandler().
> > 
> > That said:
> > 
> > It sounds like the desired "Open in Adobe Reader" behavior is (assuming this
> is
> > to deal with the depreciation of NPAPI):
> > - Force all application/pdf resources to be downloaded (requires disabling
all
> > plugins that handle application/pdf or blacklisting application/pdf MIME 
type
> > to minimize collateral).
> 
> The goal is that when the user downloads a pdf, they can click "open" and it
> opens up Adobe Reader.
> 
> The goal is not that all viewed pdfs should open up in Adobe Reader.

Ah! Thanks for the clarification.

In that case, I'd suggest looking at IsOpenInBrowserPreferreredForFile() in
chrome_download_manager_delegate. That controls whether we ask the platform the
open the download (which in our case would open it in Adobe Reader) or whether
we open it in a new tab (PDFium). This way the correct thing will happen
regardless of how we invoke 'Open' on the download (the download shelf, download
page, an extension, or automatically due to the 'Always open files of this type'
preference).

> > - Open downloaded .pdf files in external Adobe Reader application. (from the
> > downloads side, we get this behavior for free if browser plugins are
disabled
> > and Adobe Reader is the default application for handling .pdf files).
> > - Conditionalize the above behavior on a) Adobe Reader being up-to-date AND
b)
> > it being the default handler for .pdf files.
> > 
> > I'm not sure about disabling plugins in response to a download preference
> since
> > it affects how all resources with associated MIME types are handled, not
just
> > downloads. If that's the intended behavior then perhaps that pref should
live
> > someplace else where its scope is more apparent. Maybe in PluginPrefs?
> > 
> > One option is for the plugin discovery phase to detect a) and b) above. Then
> > based on a preference to use Adobe Reader, block browser plugins from
handling
> > application/pdf in ChromePluginServiceFilter. WDYT?
> > 
> > I'm still a bit iffy on the UX for this since the UI for controlling this is
> > currently planned to be downloads specific even though the scope of the
> behavior
> > isn't limited to downloads. Also the handling of PDFs can change at
arbitrary
> > points in time without explanation due to Adobe Reader going out of date.
> > 
> > Apologies if I'm distracting this CL, but I want to make sure we have a good
> > design.
> > 
> >
>
https://codereview.chromium.org/324593004/diff/60001/chrome/browser/download/...
> > File chrome/browser/download/download_shelf_context_menu.cc (right):
> > 
> >
>
https://codereview.chromium.org/324593004/diff/60001/chrome/browser/download/...
> > chrome/browser/download/download_shelf_context_menu.cc:385: return
> > path.Extension() == FILE_PATH_LITERAL(".pdf");
> > Nit: path.MatchesExtension(...)
> > 
> >
>
https://codereview.chromium.org/324593004/diff/60001/chrome/browser/ui/pdf/ad...
> > File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):
> > 
> >
>
https://codereview.chromium.org/324593004/diff/60001/chrome/browser/ui/pdf/ad...
> > chrome/browser/ui/pdf/adobe_reader_info_win.cc:101: return false;
> > Have you looked at using AssocQueryString() to get at the executable name
for
> > the 'open' verb for .pdfs? I think we are already pulling in Shlwapi.dll, so
> > might as well use it.

[Lei Zhang, 2014-06-18 13:48:29]

PTAL. This follows the requirement clarification and follows the advice to
modify IsOpenInBrowserPreferreredForFile().

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

https://codereview.chromium.org/324593004/diff/60001/chrome/browser/download/...
chrome/browser/download/download_shelf_context_menu.cc:385: return
path.Extension() == FILE_PATH_LITERAL(".pdf");
On 2014/06/12 16:50:55, asanka wrote:
> Nit: path.MatchesExtension(...)

Done.

https://codereview.chromium.org/324593004/diff/60001/chrome/browser/ui/pdf/ad...
File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):

https://codereview.chromium.org/324593004/diff/60001/chrome/browser/ui/pdf/ad...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:101: return false;
On 2014/06/12 16:50:55, asanka wrote:
> Have you looked at using AssocQueryString() to get at the executable name for
> the 'open' verb for .pdfs? I think we are already pulling in Shlwapi.dll, so
> might as well use it.

Done.

[jam, 2014-06-19 16:37:07]

sorry for the delay, just saw this.

I defer to asanka for all the download integration since I'm not familiar with
that code

https://codereview.chromium.org/324593004/diff/180001/chrome/browser/ui/pdf/a...
File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):

https://codereview.chromium.org/324593004/diff/180001/chrome/browser/ui/pdf/a...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:162: return
file_version.IsValid() && !file_version.IsOlderThan(kSecureVersion);
what if IsAdobeReaderDefaultPDFViewerInternal returns the path to Adobe Acrobat,
I'm guessing that'll have a different secure version?

https://codereview.chromium.org/324593004/diff/180001/chrome/installer/DEPS
File chrome/installer/DEPS (right):

https://codereview.chromium.org/324593004/diff/180001/chrome/installer/DEPS#n...
chrome/installer/DEPS:10: "+content/public/common/result_codes.h",
nit: not needed anymore?

https://codereview.chromium.org/324593004/diff/180001/content/public/common/r...
File content/public/common/registry_utils_win.h (right):

https://codereview.chromium.org/324593004/diff/180001/content/public/common/r...
content/public/common/registry_utils_win.h:21: // Windows registry path
constants and utility functions.
since this is generic-windows code with no content dependencies and it's used by
content & chrome, it should go into base/win/

[Lei Zhang, 2014-06-19 18:12:30]

https://codereview.chromium.org/324593004/diff/180001/chrome/browser/ui/pdf/a...
File chrome/browser/ui/pdf/adobe_reader_info_win.cc (right):

https://codereview.chromium.org/324593004/diff/180001/chrome/browser/ui/pdf/a...
chrome/browser/ui/pdf/adobe_reader_info_win.cc:162: return
file_version.IsValid() && !file_version.IsOlderThan(kSecureVersion);
On 2014/06/19 16:37:06, jam wrote:
> what if IsAdobeReaderDefaultPDFViewerInternal returns the path to Adobe
Acrobat,
> I'm guessing that'll have a different secure version?

http://www.adobe.com/support/downloads/product.jsp?product=1&platform=Windows
seems to indicate the latest is 11.0.7, which is the same as Reader. I don't
have a copy to verify the revision number is 79+ though.

https://codereview.chromium.org/324593004/diff/180001/chrome/installer/DEPS
File chrome/installer/DEPS (right):

https://codereview.chromium.org/324593004/diff/180001/chrome/installer/DEPS#n...
chrome/installer/DEPS:10: "+content/public/common/result_codes.h",
On 2014/06/19 16:37:07, jam wrote:
> nit: not needed anymore?

I can put this in a separate CL.

https://codereview.chromium.org/324593004/diff/180001/content/public/common/r...
File content/public/common/registry_utils_win.h (right):

https://codereview.chromium.org/324593004/diff/180001/content/public/common/r...
content/public/common/registry_utils_win.h:21: // Windows registry path
constants and utility functions.
On 2014/06/19 16:37:07, jam wrote:
> since this is generic-windows code with no content dependencies and it's used
by
> content & chrome, it should go into base/win/

|kRegistryAcrobat| and |kRegistryAcrobatReader| don't belong in base. Would you
prefer I put them in content/public/common or just hard code them since they're
only used in 2 places?

[Lei Zhang, 2014-06-19 18:19:44]

https://codereview.chromium.org/324593004/diff/180001/content/public/common/r...
File content/public/common/registry_utils_win.h (right):

https://codereview.chromium.org/324593004/diff/180001/content/public/common/r...
content/public/common/registry_utils_win.h:21: // Windows registry path
constants and utility functions.
On 2014/06/19 16:37:07, jam wrote:
> since this is generic-windows code with no content dependencies and it's used
by
> content & chrome, it should go into base/win/

base/win/win_util.h seems to discourage stuffing more functions and there. Do
you have a place in mind? Should we consider just duplicating GetInstalledPath()
as well if we want to avoid exposing it via content/public? The content/ usage
will likely go away after NPAPI deprecation completes.

[Lei Zhang, 2014-06-20 06:22:51]

https://codereview.chromium.org/324593004/diff/180001/content/public/common/r...
File content/public/common/registry_utils_win.h (right):

https://codereview.chromium.org/324593004/diff/180001/content/public/common/r...
content/public/common/registry_utils_win.h:21: // Windows registry path
constants and utility functions.
On 2014/06/19 18:19:44, Lei Zhang wrote:
> On 2014/06/19 16:37:07, jam wrote:
> > since this is generic-windows code with no content dependencies and it's
used
> by
> > content & chrome, it should go into base/win/
> 
> base/win/win_util.h seems to discourage stuffing more functions and there. Do
> you have a place in mind? Should we consider just duplicating
GetInstalledPath()
> as well if we want to avoid exposing it via content/public? The content/ usage
> will likely go away after NPAPI deprecation completes.

I decided to just leave content/ alone.

[Lei Zhang, 2014-06-20 22:30:06]

I pulled out adding the new string in https://codereview.chromium.org/342323009/
and will hopefully get that in today. I'll likely merge this CL to M37 after
landing it on trunk.

[asanka, 2014-06-25 03:55:08]

Shall we hash out the expected UX for the context menu? I'm not sure we are
getting it right. Also the bug mentions the "Always open in system handler"
option which I don't see in the CL. Is that for a follow-up?

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

https://codereview.chromium.org/324593004/diff/200001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:709: DownloadPrefs*
prefs = DownloadPrefs::FromBrowserContext(profile_);
Now that this is a method of CDMD, you can just use the download_prefs_ member.

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

https://codereview.chromium.org/324593004/diff/200001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:136:
BrowserThread::PostBlockingPoolTask(
Shall we leave this on the FILE thread?

The sequencing of the creation on FILE thread is, I believe, intentional. The
download related set of objects are instantiated when the first time something
touches the download system (e.g. because the user started a new download or
because the user opened the chrome://downloads page). Sequencing on the FILE
thread ensures that the tasks that will be queued later will be executed after
this one (e.g. opening the downloads folder) and thus see the default download
folder as existing. Posting on the blocking pool doesn't give us that sequence
restriction. Thus it is easier to reason about things if we stick to the FILE
thread for now.

This predates the blocking pool, and there were plans to consider moving to
sequenced worker pool, but there's no cycles to do the transition.

That said, I'm not sure why this is done for Chrome OS. Maybe the Files app
misbehaves if there's no download folder? It's certainly not a requirement on
other OSs.

https://codereview.chromium.org/324593004/diff/200001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:145: BrowserThread::GetBlockingPool(),
This also makes the extremely likely but still non-deterministic assumption that
the value would be available by the time we hit
CDMD::IsOpenInBrowserPreferredForFile().

Have you looked at DownloadTargetDeterminer? It is intentionally designed to
take these thread jumps into consideration and put all the cross-thread data
gathering into a predictable sequence. For example, we have a check in there for
looking up the plugin that would handle the target file if it was opened inside
the browser (DownloadTargetDeterminer::DoDetermineIfHandledSafely()). We can
move this logic there as well so that this check is performed deterministically
before its result is used.

https://codereview.chromium.org/324593004/diff/200001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:328: #endif
What should happen here if the Adobe Reader is not up-to-date and .pdf is listed
as an auto open file type?

[Lei Zhang, 2014-06-26 08:01:59]

On 2014/06/25 03:55:08, asanka wrote:
> Shall we hash out the expected UX for the context menu? I'm not sure we are
> getting it right. Also the bug mentions the "Always open in system handler"
> option which I don't see in the CL. Is that for a follow-up?

IDS_DOWNLOAD_MENU_ALWAYS_OPEN_TYPE (Always open files of this type) that already
exists does just that, so I didn't see the need to add a new string for opening
PDFs with the default download handler that's different from always open for
other file types.

[Lei Zhang, 2014-06-26 08:06:12]

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

https://codereview.chromium.org/324593004/diff/200001/chrome/browser/download...
chrome/browser/download/chrome_download_manager_delegate.cc:709: DownloadPrefs*
prefs = DownloadPrefs::FromBrowserContext(profile_);
On 2014/06/25 03:55:07, asanka wrote:
> Now that this is a method of CDMD, you can just use the download_prefs_
member.

Done.

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

https://codereview.chromium.org/324593004/diff/200001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:136:
BrowserThread::PostBlockingPoolTask(
On 2014/06/25 03:55:08, asanka wrote:
> Shall we leave this on the FILE thread?
> 
> The sequencing of the creation on FILE thread is, I believe, intentional. The
> download related set of objects are instantiated when the first time something
> touches the download system (e.g. because the user started a new download or
> because the user opened the chrome://downloads page). Sequencing on the FILE
> thread ensures that the tasks that will be queued later will be executed after
> this one (e.g. opening the downloads folder) and thus see the default download
> folder as existing. Posting on the blocking pool doesn't give us that sequence
> restriction. Thus it is easier to reason about things if we stick to the FILE
> thread for now.
> 
> This predates the blocking pool, and there were plans to consider moving to
> sequenced worker pool, but there's no cycles to do the transition.
> 
> That said, I'm not sure why this is done for Chrome OS. Maybe the Files app
> misbehaves if there's no download folder? It's certainly not a requirement on
> other OSs.

Reverted. I generally like to do cleanups along the way, one of which is to
ever-so-slowly get rid of the FILE thread.

https://codereview.chromium.org/324593004/diff/200001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:145: BrowserThread::GetBlockingPool(),
On 2014/06/25 03:55:08, asanka wrote:
> This also makes the extremely likely but still non-deterministic assumption
that
> the value would be available by the time we hit
> CDMD::IsOpenInBrowserPreferredForFile().
> 
> Have you looked at DownloadTargetDeterminer? It is intentionally designed to
> take these thread jumps into consideration and put all the cross-thread data
> gathering into a predictable sequence. For example, we have a check in there
for
> looking up the plugin that would handle the target file if it was opened
inside
> the browser (DownloadTargetDeterminer::DoDetermineIfHandledSafely()). We can
> move this logic there as well so that this check is performed
deterministically
> before its result is used.

Thanks for the suggestion, done.

Yes, I was very sad that I couldn't find the full version number using only the
registry, so I was hoping to do this early on and have the info ready by the
time the user hits the context menu 99% of the time.

https://codereview.chromium.org/324593004/diff/200001/chrome/browser/download...
chrome/browser/download/download_prefs.cc:328: #endif
On 2014/06/25 03:55:08, asanka wrote:
> What should happen here if the Adobe Reader is not up-to-date and .pdf is
listed
> as an auto open file type?

We currently ignore it because IsOpenInBrowserPreferreredForFile() will return
true for PDFs, right? I imagine we'll continue to do so if Adobe Reader is not
up-to-date.

[asanka, 2014-07-01 19:55:15]

https://codereview.chromium.org/324593004/diff/260001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/324593004/diff/260001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:518: if
(local_path_.Extension() == FILE_PATH_LITERAL(".pdf"))
Why skip the check if we are looking at a .pdf?

https://codereview.chromium.org/324593004/diff/260001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:518: if
(local_path_.Extension() == FILE_PATH_LITERAL(".pdf"))
Also, local_path_.MatchesExtension(...) rather than comparing the extension
directly since it deals with case sensitivity.

[asanka, 2014-07-01 19:56:18]

On 2014/07/01 19:55:15, asanka wrote:
>
https://codereview.chromium.org/324593004/diff/260001/chrome/browser/download...
> File chrome/browser/download/download_target_determiner.cc (right):
> 
>
https://codereview.chromium.org/324593004/diff/260001/chrome/browser/download...
> chrome/browser/download/download_target_determiner.cc:518: if
> (local_path_.Extension() == FILE_PATH_LITERAL(".pdf"))
> Why skip the check if we are looking at a .pdf?
> 
>
https://codereview.chromium.org/324593004/diff/260001/chrome/browser/download...
> chrome/browser/download/download_target_determiner.cc:518: if
> (local_path_.Extension() == FILE_PATH_LITERAL(".pdf"))
> Also, local_path_.MatchesExtension(...) rather than comparing the extension
> directly since it deals with case sensitivity.

And LGTM with the comments addressed.

[Lei Zhang, 2014-07-01 20:47:57]

https://codereview.chromium.org/324593004/diff/260001/chrome/browser/download...
File chrome/browser/download/download_target_determiner.cc (right):

https://codereview.chromium.org/324593004/diff/260001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:518: if
(local_path_.Extension() == FILE_PATH_LITERAL(".pdf"))
On 2014/07/01 19:55:14, asanka wrote:
> Why skip the check if we are looking at a .pdf?

Whoops, meant to only check for PDFs.

https://codereview.chromium.org/324593004/diff/260001/chrome/browser/download...
chrome/browser/download/download_target_determiner.cc:518: if
(local_path_.Extension() == FILE_PATH_LITERAL(".pdf"))
On 2014/07/01 19:55:14, asanka wrote:
> Also, local_path_.MatchesExtension(...) rather than comparing the extension
> directly since it deals with case sensitivity.

Done.

[Lei Zhang, 2014-07-02 18:52:20]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-07-02 18:52:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/324593004/320001

[commit-bot: I haz the power, 2014-07-02 19:08:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-02 19:36:06]

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

[commit-bot: I haz the power, 2014-07-02 19:36:06]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Lei Zhang, 2014-07-02 23:15:49]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-07-02 23:17:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/324593004/320001

[commit-bot: I haz the power, 2014-07-03 05:49:09]

Change committed as 281172