Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 1924473003: [Downloads] Use the initiating StoragePartition for resumption. (Closed)

Created:
3 years, 10 months ago by asanka
Modified:
3 years, 9 months ago
CC:
chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, extensions-reviews_chromium.org, loading-reviews_chromium.org, ncarter (slow)
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Downloads] Use the initiating StoragePartition for resumption. The URLRequest created for downloads resumption should be created using the same StoragePartition as the one used to initiate the download. Downloads used to use the request context returned via ResourceContext::GetURLRequestContext(), which can yield the wrong URLRequestContext (and the wrong StoragePartition) when multiple StoragePartitions are in use. Currently non-default StoragePartitions are used for implementing WebView tag in platform apps. Hence a download that's initiated in a WebView, if resumed, could result in a network request being issued that's backed by the wrong StoragePartition without this fix. This change: * Adds the site instance URL to the metadata that's persisted for each download. * Updates history database schema to accommodate site URL. * Updates DownloadURLParameters to take a URLRequestContextGetter instead of a ResourceContext. The latter can be uniquely determined by the DownloadManager. * Updates DownloadItemImpl to use the site instance URL to determine the correct StoragePartition to use during resumption. The rest is assorted tests and plumbing. R=creis@chromium.org,sky@chromium.org,asargent@chromium.org,jam@chromium.org,svaldez@chromium.org BUG=606627 Committed: https://crrev.com/3b4be1260ebe931143425302a867b7e7407d002b Cr-Commit-Position: refs/heads/master@{#392849}

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 4

Patch Set 5 : Fix ordering of site_instance_url/SiteInstanceURL fields, and add history test for WebViewTest. #

Total comments: 15

Patch Set 6 : Update WebViewTest to use one in-memory storage partition. #

Total comments: 2

Patch Set 7 : Fix typo in comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+855 lines, -298 lines) Patch
M chrome/browser/apps/guest_view/web_view_browsertest.cc View 1 2 3 4 5 6 8 chunks +364 lines, -2 lines 0 comments Download
M chrome/browser/download/download_history.cc View 1 2 3 4 5 3 chunks +7 lines, -6 lines 0 comments Download
M chrome/browser/download/download_history_unittest.cc View 1 2 3 4 5 5 chunks +9 lines, -5 lines 0 comments Download
M chrome/browser/download/download_ui_controller_unittest.cc View 1 2 3 4 5 2 chunks +6 lines, -7 lines 0 comments Download
M chrome/browser/extensions/api/downloads/downloads_api.cc View 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/extensions/api/downloads/downloads_api_browsertest.cc View 1 2 3 4 5 4 chunks +50 lines, -8 lines 0 comments Download
M chrome/browser/extensions/webstore_installer.cc View 1 2 3 4 5 2 chunks +8 lines, -3 lines 0 comments Download
M chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc View 1 2 chunks +5 lines, -4 lines 0 comments Download
A + chrome/test/data/extensions/platform_apps/web_view/download_cookie_isolation/background.js View 1 1 chunk +1 line, -1 line 0 comments Download
A + chrome/test/data/extensions/platform_apps/web_view/download_cookie_isolation/guest.html View 1 1 chunk +8 lines, -11 lines 0 comments Download
A chrome/test/data/extensions/platform_apps/web_view/download_cookie_isolation/guest.js View 1 2 3 4 5 1 chunk +31 lines, -0 lines 0 comments Download
A + chrome/test/data/extensions/platform_apps/web_view/download_cookie_isolation/manifest.json View 1 0 chunks +-1 lines, --1 lines 0 comments Download
A + chrome/test/data/extensions/platform_apps/web_view/download_cookie_isolation/window.html View 1 1 chunk +4 lines, -4 lines 0 comments Download
A chrome/test/data/extensions/platform_apps/web_view/download_cookie_isolation/window.js View 1 2 3 4 5 1 chunk +77 lines, -0 lines 0 comments Download
M components/history/core/browser/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/history/core/browser/download_database.h View 1 2 3 4 5 1 chunk +4 lines, -0 lines 0 comments Download
M components/history/core/browser/download_database.cc View 1 2 3 4 5 6 chunks +14 lines, -6 lines 0 comments Download
M components/history/core/browser/download_row.h View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments Download
M components/history/core/browser/download_row.cc View 1 2 3 4 5 3 chunks +4 lines, -1 line 0 comments Download
M components/history/core/browser/history_backend_db_unittest.cc View 1 2 3 4 5 6 chunks +65 lines, -9 lines 0 comments Download
M components/history/core/browser/history_database.cc View 2 chunks +10 lines, -1 line 0 comments Download
M components/history/core/test/history_backend_db_base_test.cc View 1 chunk +2 lines, -1 line 0 comments Download
A + components/test/data/history/history.31.sql View 1 chunk +2 lines, -2 lines 0 comments Download
M content/browser/download/download_browsertest.cc View 1 7 chunks +42 lines, -138 lines 0 comments Download
M content/browser/download/download_create_info.h View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/download/download_item_factory.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/download/download_item_impl.h View 1 2 3 4 5 4 chunks +5 lines, -2 lines 0 comments Download
M content/browser/download/download_item_impl.cc View 1 2 3 4 5 7 chunks +26 lines, -18 lines 0 comments Download
M content/browser/download/download_item_impl_unittest.cc View 1 2 2 chunks +1 line, -3 lines 0 comments Download
M content/browser/download/download_manager_impl.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/download/download_manager_impl.cc View 1 2 3 4 5 8 chunks +15 lines, -9 lines 0 comments Download
M content/browser/download/download_manager_impl_unittest.cc View 1 2 3 4 5 4 chunks +10 lines, -21 lines 0 comments Download
M content/browser/download/download_request_core.cc View 2 chunks +4 lines, -3 lines 0 comments Download
M content/browser/download/download_resource_handler.cc View 1 2 3 4 5 4 chunks +12 lines, -2 lines 0 comments Download
M content/browser/loader/resource_dispatcher_host_impl.cc View 1 chunk +1 line, -1 line 0 comments Download
M content/browser/renderer_host/render_message_filter.cc View 1 2 3 4 5 6 1 chunk +1 line, -1 line 0 comments Download
M content/public/browser/download_item.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/download_manager.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/download_url_parameters.h View 5 chunks +26 lines, -15 lines 0 comments Download
M content/public/browser/download_url_parameters.cc View 1 4 chunks +10 lines, -7 lines 0 comments Download
M content/public/test/mock_download_item.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/public/test/mock_download_manager.h View 1 2 3 4 5 3 chunks +3 lines, -0 lines 0 comments Download
M content/public/test/mock_download_manager.cc View 1 2 3 4 5 6 chunks +10 lines, -5 lines 0 comments Download

Messages

Total messages: 30 (13 generated)
asanka
svaldez: chrome/browser/download/* content/browser/download/* sky: components/history/* components/test/data/history/history.31.sql chrome/browser/safe_browsing/incident_reporting/last_download_finder_unittest.cc asargent: chrome/browser/apps/guest_view/web_view_browsertest.cc chrome/browser/extensions/api/downloads/* chrome/browser/extensions/webstore_installer.cc chrome/test/data/extensions/platform_apps/web_view/download_cookie_isolation/* jam: content/public/browser/* content/public/test/* ...
3 years, 9 months ago (2016-05-04 19:55:22 UTC) #5
sky
Said files LGTM
3 years, 9 months ago (2016-05-04 20:49:27 UTC) #6
asargent_no_longer_on_chrome
extensions/apps stuff lgtm
3 years, 9 months ago (2016-05-04 22:30:47 UTC) #7
svaldez
lgtm https://codereview.chromium.org/1924473003/diff/60001/chrome/browser/download/download_ui_controller_unittest.cc File chrome/browser/download/download_ui_controller_unittest.cc (right): https://codereview.chromium.org/1924473003/diff/60001/chrome/browser/download/download_ui_controller_unittest.cc#newcode228 chrome/browser/download/download_ui_controller_unittest.cc:228: EXPECT_CALL(*item, GetSiteInstanceUrl()) nit: Same order as elsewhere (Referrer->SiteInstance->Tab->TabReferrer). ...
3 years, 9 months ago (2016-05-05 14:23:27 UTC) #8
jam
The cl description says "site isolation", although the difference for storage partitions is not site ...
3 years, 9 months ago (2016-05-05 15:49:41 UTC) #9
asanka
jam: I updated the CL description and added a short blurb to the bug to ...
3 years, 9 months ago (2016-05-05 17:38:05 UTC) #12
asanka
asargent: I added a cross-session test for WebViewTest. This is to test that we correctly ...
3 years, 9 months ago (2016-05-05 17:40:49 UTC) #13
Charlie Reis
[CC nick@ for use of GetSiteURL] Thanks for fixing this, and sorry for the delay! ...
3 years, 9 months ago (2016-05-05 21:43:23 UTC) #14
ncarter (slow)
https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/download_item.h File content/public/browser/download_item.h (right): https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/download_item.h#newcode195 content/public/browser/download_item.h:195: virtual const GURL& GetSiteInstanceUrl() const = 0; On 2016/05/05 ...
3 years, 9 months ago (2016-05-05 23:29:07 UTC) #16
asargent_no_longer_on_chrome
On 2016/05/05 17:40:49, asanka wrote: > asargent: I added a cross-session test for WebViewTest. This ...
3 years, 9 months ago (2016-05-06 00:02:15 UTC) #19
asanka
Thanks everyone! > Also, does this mean that we won't attempt to resume downloads started ...
3 years, 9 months ago (2016-05-07 00:24:45 UTC) #20
asanka
On 2016/05/07 at 00:24:45, asanka wrote: > Thanks everyone! > > > Also, does this ...
3 years, 9 months ago (2016-05-10 20:31:35 UTC) #21
Charlie Reis
On 2016/05/07 00:24:45, asanka wrote: > Thanks everyone! > > > Also, does this mean ...
3 years, 9 months ago (2016-05-10 22:50:03 UTC) #22
asanka
https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/download_url_parameters.cc File content/public/browser/download_url_parameters.cc (right): https://codereview.chromium.org/1924473003/diff/80001/content/public/browser/download_url_parameters.cc#newcode40 content/public/browser/download_url_parameters.cc:40: std::unique_ptr<DownloadUrlParameters> DownloadUrlParameters::FromWebContents( On 2016/05/10 at 22:50:02, Charlie Reis wrote: ...
3 years, 9 months ago (2016-05-11 03:14:53 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1924473003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1924473003/120001
3 years, 9 months ago (2016-05-11 03:16:51 UTC) #26
commit-bot: I haz the power
Committed patchset #7 (id:120001)
3 years, 9 months ago (2016-05-11 04:28:09 UTC) #28
commit-bot: I haz the power
3 years, 9 months ago (2016-05-11 04:29:31 UTC) #30
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/3b4be1260ebe931143425302a867b7e7407d002b
Cr-Commit-Position: refs/heads/master@{#392849}

Powered by Google App Engine
This is Rietveld 408576698