Chromium Code Reviews
Help | Chromium Project | Sign in
(262)

Issue 55063002: Prefer opening PDF downloads in the browser. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 8 months ago by asanka (OOO until July 6)
Modified:
1 year, 7 months ago
CC:
chromium-reviews, benjhayden+dwatch_chromium.org, felt
Visibility:
Public.

Description

Prefer opening PDF downloads in the browser. PDFs in particular are safer to open in the browser. This patch changes the handling of downloads to open such files in the browser by default instead of the system handler for the file type. A "Open with system handler" menu item will be available so that users can still use the system application if needed. The determination that a file is safer to handle in the browser is done as follows: a) DownloadTargetDeterminer determines whether the MIME type corresponding to the target filename of the download is one which is handled by the renderer or one that is handled by a sandboxed pepper plugin. If so, then the file is considered safely handled by the browser. b) ChromeDownloadManagerDelegate determines whether opening in the browser is preferred for the file type assuming the browser is able to handle it safely. Currently this is true for .pdf files. Opening behavior for a download will default to opening in the browser if both results from a) and b) are true. BUG=148492 TEST=(1) Make sure Chrome PDF Viewer is enabled in chrome://plugins. (2) Download a .pdf file. (3) Download shelf context menu should show "Open" and "Open with system handler" options. (4) "Open" as well as clicking on the shelf icon and clicking on the download in chrome://downloads should all result in opening the file within the browser. (5) "Open with system handler" should open the .pdf with the application that's set up as the default handler for .pdf files on the users' machine. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233460 Reverted : https://src.chromium.org/viewvc/chrome?view=rev&revision=233497 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234031

Patch Set 1 #

Total comments: 2

Patch Set 2 : Unit test + Open in new tab implementation. #

Patch Set 3 : Tests + rebase #

Total comments: 15

Patch Set 4 : Rebase + update browser opening logic to use ScopedTabbedBrowserDisplayer #

Patch Set 5 : Reorganize some code to address comments. #

Total comments: 6

Patch Set 6 : Fix tests #

Total comments: 2

Patch Set 7 : Address comments. Use url as well as MIME type for determining plugins. #

Patch Set 8 : Add UMA. Limit change to PDF files for now. #

Total comments: 2

Patch Set 9 : Fix tests. #

Patch Set 10 : Fix tests on platforms where NPAPI may not be available. #

Patch Set 11 : Merge with 233391 after commit and revert. #

Patch Set 12 : Destroy PluginService once we are done with our plugin tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+927 lines, -130 lines) Patch
M chrome/browser/download/chrome_download_manager_delegate.h View 1 2 4 chunks +14 lines, -0 lines 0 comments Download
M chrome/browser/download/chrome_download_manager_delegate.cc View 1 2 3 4 5 6 7 8 8 chunks +99 lines, -7 lines 0 comments Download
M chrome/browser/download/chrome_download_manager_delegate_unittest.cc View 1 2 3 4 5 6 7 8 9 chunks +23 lines, -36 lines 0 comments Download
M chrome/browser/download/download_item_model.h View 1 chunk +12 lines, -0 lines 0 comments Download
M chrome/browser/download/download_item_model.cc View 1 2 3 4 5 6 7 6 chunks +42 lines, -1 line 0 comments Download
M chrome/browser/download/download_shelf_context_menu.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/download/download_shelf_context_menu.cc View 1 2 3 4 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/download/download_stats.h View 1 2 3 4 5 6 7 2 chunks +20 lines, -0 lines 0 comments Download
M chrome/browser/download/download_stats.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/download/download_target_determiner.h View 1 2 7 chunks +43 lines, -9 lines 0 comments Download
M chrome/browser/download/download_target_determiner.cc View 1 2 3 4 5 6 7 8 9 chunks +159 lines, -16 lines 0 comments Download
M chrome/browser/download/download_target_determiner_delegate.h View 1 2 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/browser/download/download_target_determiner_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 chunks +429 lines, -61 lines 0 comments Download
A chrome/browser/download/download_target_info.h View 1 2 3 4 5 6 1 chunk +47 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -0 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 2 chunks +14 lines, -0 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 43 (0 generated)
asanka (OOO until July 6)
FYI. Working on tests.
1 year, 8 months ago (2013-10-31 17:27:39 UTC) #1
cbentzel
On 2013/10/31 17:27:39, asanka wrote: > FYI. Working on tests. What happens if there is ...
1 year, 8 months ago (2013-10-31 20:19:54 UTC) #2
palmer
https://codereview.chromium.org/55063002/diff/1/chrome/browser/download/chrome_download_manager_delegate.cc File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/55063002/diff/1/chrome/browser/download/chrome_download_manager_delegate.cc#newcode173 chrome/browser/download/chrome_download_manager_delegate.cc:173: if (path.MatchesExtension(FILE_PATH_LITERAL(".pdf"))) Ideally, we should check by the website-declared ...
1 year, 8 months ago (2013-10-31 21:09:14 UTC) #3
asanka (OOO until July 6)
On 2013/10/31 20:19:54, cbentzel wrote: > On 2013/10/31 17:27:39, asanka wrote: > > FYI. Working ...
1 year, 8 months ago (2013-11-01 14:23:55 UTC) #4
asanka (OOO until July 6)
https://codereview.chromium.org/55063002/diff/1/chrome/browser/download/chrome_download_manager_delegate.cc File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/55063002/diff/1/chrome/browser/download/chrome_download_manager_delegate.cc#newcode173 chrome/browser/download/chrome_download_manager_delegate.cc:173: if (path.MatchesExtension(FILE_PATH_LITERAL(".pdf"))) On 2013/10/31 21:09:15, Chromium Palmer wrote: > ...
1 year, 8 months ago (2013-11-01 14:29:36 UTC) #5
palmer
> I'm proposing that we only look for PEPPER_{IN,OUT_OF}_PROCESS plugins. SGTM.
1 year, 8 months ago (2013-11-01 19:04:26 UTC) #6
palmer
> I'm ignoring the website-declared MIME type and the sniffed MIME type here > because ...
1 year, 8 months ago (2013-11-01 19:06:28 UTC) #7
palmer
LGTM, FWIW, BTW. :)
1 year, 8 months ago (2013-11-01 19:11:30 UTC) #8
asanka (OOO until July 6)
Randy: Could you have a look?
1 year, 8 months ago (2013-11-02 00:15:18 UTC) #9
cbentzel
On 2013/11/01 19:06:28, Chromium Palmer wrote: > > I'm ignoring the website-declared MIME type and ...
1 year, 8 months ago (2013-11-04 15:14:38 UTC) #10
asanka (OOO until July 6)
On 2013/11/04 15:14:38, cbentzel wrote: > On 2013/11/01 19:06:28, Chromium Palmer wrote: > > > ...
1 year, 8 months ago (2013-11-04 17:28:30 UTC) #11
Chris Palmer
Yes, it did. On Nov 4, 2013 7:15 AM, <cbentzel@chromium.org> wrote: > On 2013/11/01 19:06:28, ...
1 year, 8 months ago (2013-11-04 17:53:26 UTC) #12
rdsmith
So my basic reaction I think I communicated verbally when we talked, but I'll include ...
1 year, 8 months ago (2013-11-04 19:53:25 UTC) #13
asanka (OOO until July 6)
https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/chrome_download_manager_delegate.cc File chrome/browser/download/chrome_download_manager_delegate.cc (right): https://codereview.chromium.org/55063002/diff/160001/chrome/browser/download/chrome_download_manager_delegate.cc#newcode174 chrome/browser/download/chrome_download_manager_delegate.cc:174: const base::Callback<void(const std::string&)>& callback) { On 2013/11/04 19:53:26, rdsmith ...
1 year, 8 months ago (2013-11-04 21:20:10 UTC) #14
asanka (OOO until July 6)
On 2013/11/04 19:53:25, rdsmith wrote: > So my basic reaction I think I communicated verbally ...
1 year, 8 months ago (2013-11-04 21:30:29 UTC) #15
asanka (OOO until July 6)
On 2013/11/04 21:30:29, asanka wrote: > On 2013/11/04 19:53:25, rdsmith wrote: > > So my ...
1 year, 8 months ago (2013-11-04 21:33:48 UTC) #16
rdsmith
On 2013/11/04 21:30:29, asanka wrote: > On 2013/11/04 19:53:25, rdsmith wrote: > > So my ...
1 year, 8 months ago (2013-11-04 22:29:41 UTC) #17
palmer
> The code complexity derives from how we have to determine whether a file can ...
1 year, 8 months ago (2013-11-04 22:34:56 UTC) #18
rdsmith
My apologies--I didn't manage to get to reviewing the test, and I have to take ...
1 year, 8 months ago (2013-11-04 22:57:46 UTC) #19
rdsmith
On 2013/11/04 22:34:56, Chromium Palmer wrote: > > The code complexity derives from how we ...
1 year, 8 months ago (2013-11-04 23:02:13 UTC) #20
palmer
> > I think it is best to go with the current plan. We should ...
1 year, 8 months ago (2013-11-04 23:48:09 UTC) #21
palmer
> > I think it is best to go with the current plan. We should ...
1 year, 8 months ago (2013-11-04 23:48:09 UTC) #22
palmer
Hmm, when I build and run Chrome with this patch, it doesn't open PDFs in ...
1 year, 8 months ago (2013-11-05 00:19:03 UTC) #23
asanka (OOO until July 6)
On 2013/11/05 00:19:03, Chromium Palmer wrote: > Hmm, when I build and run Chrome with ...
1 year, 8 months ago (2013-11-05 00:28:23 UTC) #24
palmer
> Do you see the Chrome PDF Viewer plugin in chrome://plugins? Ahh, my bad; I ...
1 year, 8 months ago (2013-11-05 00:48:01 UTC) #25
rdsmith
On 2013/11/04 23:48:09, Chromium Palmer wrote: > > > I think it is best to ...
1 year, 8 months ago (2013-11-05 15:31:18 UTC) #26
rdsmith
Asanka: In the context of this argument, could you make sure in this CL that ...
1 year, 8 months ago (2013-11-05 15:32:07 UTC) #27
rdsmith
Oh, I've done the test review, and this looks basically good. Let me know what ...
1 year, 8 months ago (2013-11-05 16:52:39 UTC) #28
asanka (OOO until July 6)
I'll update the CL description. https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/download_target_determiner.cc File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/55063002/diff/310001/chrome/browser/download/download_target_determiner.cc#newcode408 chrome/browser/download/download_target_determiner.cc:408: // handle |mime_type| for ...
1 year, 8 months ago (2013-11-05 21:31:38 UTC) #29
rdsmith
On 2013/11/05 21:31:38, asanka wrote: > The flow for file:// URLs is: > //net: > ...
1 year, 8 months ago (2013-11-05 21:43:12 UTC) #30
asanka (OOO until July 6)
On 2013/11/05 21:43:12, rdsmith wrote: > On 2013/11/05 21:31:38, asanka wrote: > > The flow ...
1 year, 8 months ago (2013-11-05 21:56:59 UTC) #31
Chris Palmer
AFAICT, this CL only increases the chance that the Chrome PDF plugin will be used. ...
1 year, 8 months ago (2013-11-05 22:14:58 UTC) #32
asanka (OOO until July 6)
UMA added. I also limited this change to PDF files for now.
1 year, 8 months ago (2013-11-05 22:40:38 UTC) #33
asanka (OOO until July 6)
+isherman: Could you take a look at histograms.xml?
1 year, 8 months ago (2013-11-05 22:43:55 UTC) #34
rdsmith
LGTM.
1 year, 8 months ago (2013-11-05 22:59:37 UTC) #35
Ilya Sherman
histograms LGTM with a question: https://codereview.chromium.org/55063002/diff/660001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/55063002/diff/660001/tools/metrics/histograms/histograms.xml#newcode21158 tools/metrics/histograms/histograms.xml:21158: + <int value="2" label="Opened ...
1 year, 8 months ago (2013-11-05 23:14:02 UTC) #36
asanka (OOO until July 6)
Thanks everyone! I'll land once the tests are fixed. https://codereview.chromium.org/55063002/diff/660001/tools/metrics/histograms/histograms.xml File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/55063002/diff/660001/tools/metrics/histograms/histograms.xml#newcode21158 tools/metrics/histograms/histograms.xml:21158: ...
1 year, 8 months ago (2013-11-05 23:24:38 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asanka@chromium.org/55063002/1010001
1 year, 7 months ago (2013-11-06 17:41:42 UTC) #38
commit-bot: I haz the power
Retried try job too often on linux_rel for step(s) check_deps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&number=186057
1 year, 7 months ago (2013-11-06 18:50:21 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asanka@chromium.org/55063002/1010001
1 year, 7 months ago (2013-11-06 19:46:36 UTC) #40
commit-bot: I haz the power
Change committed as 233460
1 year, 7 months ago (2013-11-07 01:27:53 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/asanka@chromium.org/55063002/1410001
1 year, 7 months ago (2013-11-08 20:50:02 UTC) #42
commit-bot: I haz the power
1 year, 7 months ago (2013-11-08 23:24:56 UTC) #43
Message was sent while issue was closed.
Change committed as 234031
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1f9106d