Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(720)

Issue 10700117: Replaced static URLRequestFileJob factory with non-static protocol handler for File jobs (Closed)

Created:
8 years, 5 months ago by shalev
Modified:
8 years, 4 months ago
CC:
chromium-reviews, Aaron Boodman, cbentzel+watch_chromium.org, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org
Visibility:
Public.

Description

Replaced static URLRequestFileJob factory with non-static protocol handler for File jobs This is a refactor and shouldn't have any visible behavioral change. BUG=crbug.com/142945 TEST=browser_tests --single_process --gtest_filter=FullscreenControllerTest.FullscreenFileURL Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=152381

Patch Set 1 #

Patch Set 2 : Removed context from file_dir_job, merged with error_job cl #

Total comments: 6

Patch Set 3 : Fixed comments #

Patch Set 4 : Fixed bug in test file #

Patch Set 5 : Simplified changes #

Patch Set 6 : Minor fix #

Patch Set 7 : Fixed merge #

Total comments: 12

Patch Set 8 : Made changes suggested by reviewers #

Total comments: 6

Patch Set 9 : Nits fixed #

Total comments: 4

Patch Set 10 : Fixed nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+181 lines, -37 lines) Patch
M chrome/browser/extensions/extension_protocols.cc View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
M chrome/browser/extensions/extension_resource_protocols.cc View 1 2 3 4 5 6 7 8 2 chunks +5 lines, -2 lines 0 comments Download
M chrome/browser/profiles/off_the_record_profile_io_data.cc View 1 2 3 4 5 6 7 3 chunks +23 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_impl_io_data.cc View 1 2 3 4 5 6 7 3 chunks +31 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_io_data.cc View 1 2 3 4 5 6 7 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/ui/webui/chrome_url_data_manager_backend.h View 1 5 6 7 8 9 2 chunks +3 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/chrome_url_data_manager_backend.cc View 1 2 3 4 5 6 7 8 9 3 chunks +13 lines, -7 lines 0 comments Download
M content/test/net/url_request_mock_http_job.cc View 1 2 3 4 2 chunks +4 lines, -1 line 0 comments Download
M net/net.gyp View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
A net/url_request/file_protocol_handler.h View 1 2 3 4 5 6 7 8 1 chunk +33 lines, -0 lines 0 comments Download
A net/url_request/file_protocol_handler.cc View 1 2 3 4 1 chunk +48 lines, -0 lines 0 comments Download
M net/url_request/url_request_file_job.h View 1 2 3 4 5 6 7 2 chunks +3 lines, -7 lines 0 comments Download
M net/url_request/url_request_file_job.cc View 1 2 3 4 5 6 4 chunks +9 lines, -13 lines 0 comments Download
M webkit/tools/test_shell/test_shell.cc View 1 2 3 4 1 chunk +3 lines, -1 line 0 comments Download

Messages

Total messages: 22 (0 generated)
erikwright (departed)
http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/off_the_record_profile_io_data.cc File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/off_the_record_profile_io_data.cc#newcode319 chrome/browser/profiles/off_the_record_profile_io_data.cc:319: chrome::kFtpScheme, kFileScheme? http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/profile_impl_io_data.cc File chrome/browser/profiles/profile_impl_io_data.cc (right): http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/profile_impl_io_data.cc#newcode555 chrome/browser/profiles/profile_impl_io_data.cc:555: chrome::kFtpScheme, ...
8 years, 5 months ago (2012-07-13 14:50:36 UTC) #1
shalev
http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/off_the_record_profile_io_data.cc File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10700117/diff/2001/chrome/browser/profiles/off_the_record_profile_io_data.cc#newcode319 chrome/browser/profiles/off_the_record_profile_io_data.cc:319: chrome::kFtpScheme, On 2012/07/13 14:50:36, erikwright wrote: > kFileScheme? Done. ...
8 years, 5 months ago (2012-07-17 19:40:16 UTC) #2
shalev
Hi Matt, This cl introduces a protocol handler for file jobs, analogously to the cls ...
8 years, 4 months ago (2012-08-14 19:50:05 UTC) #3
willchan no longer on Chromium
Is there a metabug tracking all this work? Please add a BUG link if so. ...
8 years, 4 months ago (2012-08-14 20:45:47 UTC) #4
mmenke
http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc#newcode259 chrome/browser/profiles/off_the_record_profile_io_data.cc:259: chrome::kFileScheme, new net::FileProtocolHandler(NULL)); Suggest a comment that a FileProtocolHandler ...
8 years, 4 months ago (2012-08-15 14:57:15 UTC) #5
erikwright (departed)
URLRequestJobManager returns an Error job when a scheme is unknown, with ERR_FAILED. The file protocol ...
8 years, 4 months ago (2012-08-15 19:02:02 UTC) #6
mmenke1
http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc#newcode259 chrome/browser/profiles/off_the_record_profile_io_data.cc:259: chrome::kFileScheme, new net::FileProtocolHandler(NULL)); On 2012/08/15 19:02:03, erikwright wrote: > ...
8 years, 4 months ago (2012-08-15 19:13:15 UTC) #7
erikwright (departed)
On 2012/08/15 19:13:15, mmenke (incorrect) wrote: > http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc > File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): > > http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc#newcode259 ...
8 years, 4 months ago (2012-08-15 19:20:25 UTC) #8
mmenke1
On 2012/08/15 19:20:25, erikwright wrote: > On 2012/08/15 19:13:15, mmenke (incorrect) wrote: > > > ...
8 years, 4 months ago (2012-08-15 19:34:22 UTC) #9
erikwright (departed)
OK. Add a TODO: Without a network_delegate this protocol handler will never handle file: requests, ...
8 years, 4 months ago (2012-08-15 19:52:05 UTC) #10
shalev
http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc File chrome/browser/profiles/off_the_record_profile_io_data.cc (right): http://codereview.chromium.org/10700117/diff/31001/chrome/browser/profiles/off_the_record_profile_io_data.cc#newcode259 chrome/browser/profiles/off_the_record_profile_io_data.cc:259: chrome::kFileScheme, new net::FileProtocolHandler(NULL)); On 2012/08/15 19:13:16, mmenke (incorrect) wrote: ...
8 years, 4 months ago (2012-08-15 21:23:41 UTC) #11
mmenke
LGTM with nits addressed https://chromiumcodereview.appspot.com/10700117/diff/23019/chrome/browser/extensions/extension_resource_protocols.cc File chrome/browser/extensions/extension_resource_protocols.cc (right): https://chromiumcodereview.appspot.com/10700117/diff/23019/chrome/browser/extensions/extension_resource_protocols.cc#newcode22 chrome/browser/extensions/extension_resource_protocols.cc:22: request->context()->network_delegate()), While you're here, could ...
8 years, 4 months ago (2012-08-16 14:54:31 UTC) #12
shalev
http://codereview.chromium.org/10700117/diff/23019/chrome/browser/extensions/extension_resource_protocols.cc File chrome/browser/extensions/extension_resource_protocols.cc (right): http://codereview.chromium.org/10700117/diff/23019/chrome/browser/extensions/extension_resource_protocols.cc#newcode22 chrome/browser/extensions/extension_resource_protocols.cc:22: request->context()->network_delegate()), On 2012/08/16 14:54:31, Matt Menke wrote: > While ...
8 years, 4 months ago (2012-08-16 15:07:25 UTC) #13
erikwright (departed)
LGTM.
8 years, 4 months ago (2012-08-16 15:18:32 UTC) #14
mmenke
Oh, and I'm a profile/ owner now, so you can land without Will's signoff, though ...
8 years, 4 months ago (2012-08-16 15:24:34 UTC) #15
Evan Stade
webui lgtm http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/chrome_url_data_manager_backend.cc File chrome/browser/ui/webui/chrome_url_data_manager_backend.cc (right): http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/chrome_url_data_manager_backend.cc#newcode582 chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:582: net::NetworkDelegate* network_delegate_; document ownership http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/chrome_url_data_manager_backend.h File chrome/browser/ui/webui/chrome_url_data_manager_backend.h ...
8 years, 4 months ago (2012-08-16 16:45:56 UTC) #16
not at google - send to devlin
extensions lgtm
8 years, 4 months ago (2012-08-16 22:38:08 UTC) #17
shalev
http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/chrome_url_data_manager_backend.cc File chrome/browser/ui/webui/chrome_url_data_manager_backend.cc (right): http://codereview.chromium.org/10700117/diff/20019/chrome/browser/ui/webui/chrome_url_data_manager_backend.cc#newcode582 chrome/browser/ui/webui/chrome_url_data_manager_backend.cc:582: net::NetworkDelegate* network_delegate_; On 2012/08/16 16:45:56, Evan Stade wrote: > ...
8 years, 4 months ago (2012-08-17 15:55:13 UTC) #18
brettw
webkit lgtm, i only looked at test_shell.cc
8 years, 4 months ago (2012-08-17 17:22:05 UTC) #19
tony
webkit LGTM
8 years, 4 months ago (2012-08-20 17:13:01 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/shalev@chromium.org/10700117/22005
8 years, 4 months ago (2012-08-20 17:48:20 UTC) #21
commit-bot: I haz the power
8 years, 4 months ago (2012-08-20 20:30:48 UTC) #22
Change committed as 152381

Powered by Google App Engine
This is Rietveld 408576698