[Net] Add a priority parameter to URLRequest's constructor

[Net] Add a priority parameter to URLRequest's constructor

This is so that it is clearer what the intended initial priority of
a URLRequest is.

It is also needed so that we can later enforce that if a URLRequest
is set to ignore limits, it has MAXIMUM_PRIORITY; otherwise, we'd have
to mandate that SetPriority() is called before set_load_flags(), which
is fiddly.

Also standardize on a single URLRequest constructor.

BUG=166689
R=jam@chromium.org, jamesr@chromium.org, joth@chromium.org, mmenke@chromium.org, scottbyer@chromium.org, sky@chromium.org, tommi@chromium.org

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

[akalin, 2013-10-30 01:44:27]

(Note that I auto-formatted with 'git cl format')

+mmenke for overall review and net/ OWNERS
+jam for chrome/ OWNERS
+scottbyer for cloud_print/ OWNERS
+joth for components/navigation_interception OWNERS
+sky for content/browser OWNERS
+jamesr for webkit/browser OWNERS

[jamesr, 2013-10-30 01:46:05]

Could you please pick a primary reviewer for the API change and then once
they've approved the patch add all of the associated OWNERS as fyis?

[akalin, 2013-10-30 01:48:55]

On 2013/10/30 01:46:05, jamesr wrote:
> Could you please pick a primary reviewer for the API change and then once
> they've approved the patch add all of the associated OWNERS as fyis?

Okay. mmenke is the primary reviewer. All other people don't have to look at
this until he approves, I guess.

[akalin, 2013-10-30 03:34:08]

+tommi for chrome_frame/ OWNERS

[tommi (sloooow) - chröme, 2013-10-30 10:27:37]

On 2013/10/30 03:34:08, akalin wrote:
> +tommi for chrome_frame/ OWNERS

lgtm

[sky, 2013-10-30 15:09:31]

LGTM

[mmenke, 2013-10-30 15:27:10]

LGTM, just nits.

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/extension...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/extension...
chrome/browser/extensions/extension_protocols_unittest.cc:177: NULL);
I don't think we should depend on the resource context's request context having
a NULL delegate.  Just use the request context's CreateRequest function.

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/policy/ur...
File chrome/browser/policy/url_blacklist_manager_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/policy/ur...
chrome/browser/policy/url_blacklist_manager_unittest.cc:501:
GURL("http://google.com"), net::DEFAULT_PRIORITY, NULL, &context, NULL);
Again, prefer not to add dependencies on context's from external files having
NULL network delegates.

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/prerender...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/prerender...
chrome/browser/prerender/prerender_tracker_unittest.cc:535: NULL);
Again, don't want to add assumptions here.

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/search/if...
File chrome/browser/search/iframe_source_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/search/if...
chrome/browser/search/iframe_source_unittest.cc:104: NULL);
Again, don't want to add assumptions here.

https://codereview.chromium.org/51953002/diff/220004/components/navigation_in...
File
components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/51953002/diff/220004/components/navigation_in...
components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc:126:
NULL) {
This depends on the resource_context_ having a NULL NetworkDelegate.  Maybe
switch to using resource_context_.GetRequestContext()->CreateRequest, and
sotring the request in a scoped_ptr?

https://codereview.chromium.org/51953002/diff/220004/content/browser/loader/r...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/content/browser/loader/r...
content/browser/loader/resource_loader_unittest.cc:220: NULL));
Again, seems like we shouldn't be depending on resource_context_'s request
context having a NULL delegate.

https://codereview.chromium.org/51953002/diff/220004/net/url_request/url_requ...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/51953002/diff/220004/net/url_request/url_requ...
net/url_request/url_request_context.cc:74: const {
nit:  Const should go on same line as last argument.  Simplest just to bring all
arguments down a line and indent by 4.

https://codereview.chromium.org/51953002/diff/220004/net/url_request/url_requ...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:688:
"hORIBHx+ufPj/gLB/zGI/G4Bk0AAAAASUVORK5CYII="),
I think this was much better before.  Assume you used auto formatting, but this
now looks pretty hideous.

[Scott Byer, 2013-10-30 16:03:12]

cloud print LGTM

[jam, 2013-10-30 18:12:22]

lgtm

[akalin, 2013-10-30 21:44:36]

Addressed all comments, added more usage of scoped_ptr

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/extension...
File chrome/browser/extensions/extension_protocols_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/extension...
chrome/browser/extensions/extension_protocols_unittest.cc:177: NULL);
On 2013/10/30 15:27:11, mmenke wrote:
> I don't think we should depend on the resource context's request context
having
> a NULL delegate.  Just use the request context's CreateRequest function.

From our chat, making the 'deprecated' constructor the only one, removing
network_delegate args everywhere.

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/policy/ur...
File chrome/browser/policy/url_blacklist_manager_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/policy/ur...
chrome/browser/policy/url_blacklist_manager_unittest.cc:501:
GURL("http://google.com"), net::DEFAULT_PRIORITY, NULL, &context, NULL);
On 2013/10/30 15:27:11, mmenke wrote:
> Again, prefer not to add dependencies on context's from external files having
> NULL network delegates.

removed arg.

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/prerender...
File chrome/browser/prerender/prerender_tracker_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/prerender...
chrome/browser/prerender/prerender_tracker_unittest.cc:535: NULL);
On 2013/10/30 15:27:11, mmenke wrote:
> Again, don't want to add assumptions here.

removed arg.

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/search/if...
File chrome/browser/search/iframe_source_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/chrome/browser/search/if...
chrome/browser/search/iframe_source_unittest.cc:104: NULL);
On 2013/10/30 15:27:11, mmenke wrote:
> Again, don't want to add assumptions here.

removed arg.

https://codereview.chromium.org/51953002/diff/220004/components/navigation_in...
File
components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc
(right):

https://codereview.chromium.org/51953002/diff/220004/components/navigation_in...
components/navigation_interception/intercept_navigation_resource_throttle_unittest.cc:126:
NULL) {
On 2013/10/30 15:27:11, mmenke wrote:
> This depends on the resource_context_ having a NULL NetworkDelegate.  Maybe
> switch to using resource_context_.GetRequestContext()->CreateRequest, and
> sotring the request in a scoped_ptr?

removed arg.

https://codereview.chromium.org/51953002/diff/220004/content/browser/loader/r...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/content/browser/loader/r...
content/browser/loader/resource_loader_unittest.cc:220: NULL));
On 2013/10/30 15:27:11, mmenke wrote:
> Again, seems like we shouldn't be depending on resource_context_'s request
> context having a NULL delegate.

removed arg.

https://codereview.chromium.org/51953002/diff/220004/net/url_request/url_requ...
File net/url_request/url_request_context.cc (right):

https://codereview.chromium.org/51953002/diff/220004/net/url_request/url_requ...
net/url_request/url_request_context.cc:74: const {
On 2013/10/30 15:27:11, mmenke wrote:
> nit:  Const should go on same line as last argument.  Simplest just to bring
all
> arguments down a line and indent by 4.

Done.

https://codereview.chromium.org/51953002/diff/220004/net/url_request/url_requ...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/220004/net/url_request/url_requ...
net/url_request/url_request_unittest.cc:688:
"hORIBHx+ufPj/gLB/zGI/G4Bk0AAAAASUVORK5CYII="),
On 2013/10/30 15:27:11, mmenke wrote:
> I think this was much better before.  Assume you used auto formatting, but
this
> now looks pretty hideous.

Done.

[joth, 2013-10-30 21:48:48]

a_w/ lgtm

https://codereview.chromium.org/51953002/diff/90059/android_webview/browser/n...
File
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/51953002/diff/90059/android_webview/browser/n...
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc:22:
using net::DEFAULT_PRIORITY;
nit: this seems excessive given it's used in one place and doesn't save any
vertical space by being able to omit the net:: where used.

(But hey it's only test code; I'm not strongly opinionated on this)

[akalin, 2013-10-30 23:25:58]

https://codereview.chromium.org/51953002/diff/90059/android_webview/browser/n...
File
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/51953002/diff/90059/android_webview/browser/n...
android_webview/browser/net/android_stream_reader_url_request_job_unittest.cc:22:
using net::DEFAULT_PRIORITY;
On 2013/10/30 21:48:49, joth wrote:
> nit: this seems excessive given it's used in one place and doesn't save any
> vertical space by being able to omit the net:: where used.
> 
> (But hey it's only test code; I'm not strongly opinionated on this)

Done.

[mmenke, 2013-10-31 15:23:42]

LGTM

https://codereview.chromium.org/51953002/diff/1040001/net/proxy/proxy_script_...
File net/proxy/proxy_script_fetcher_impl.cc (right):

https://codereview.chromium.org/51953002/diff/1040001/net/proxy/proxy_script_...
net/proxy/proxy_script_fetcher_impl.cc:137:
url_request_context_->CreateRequest(url, DEFAULT_PRIORITY, this);
include request_priority.h?

https://codereview.chromium.org/51953002/diff/1040001/net/url_request/url_req...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/1040001/net/url_request/url_req...
net/url_request/url_request_job_unittest.cc:33: "", TEST_MODE_NORMAL,
&GZipServer, 0, OK};
Probably best to keep the old format here and below - easier to count lines than
arguments.

[akalin, 2013-10-31 20:02:01]

Comments addressed, committing soon

https://codereview.chromium.org/51953002/diff/1040001/net/proxy/proxy_script_...
File net/proxy/proxy_script_fetcher_impl.cc (right):

https://codereview.chromium.org/51953002/diff/1040001/net/proxy/proxy_script_...
net/proxy/proxy_script_fetcher_impl.cc:137:
url_request_context_->CreateRequest(url, DEFAULT_PRIORITY, this);
On 2013/10/31 15:23:43, mmenke wrote:
> include request_priority.h?

Done. Fixed another file, too.

https://codereview.chromium.org/51953002/diff/1040001/net/url_request/url_req...
File net/url_request/url_request_job_unittest.cc (right):

https://codereview.chromium.org/51953002/diff/1040001/net/url_request/url_req...
net/url_request/url_request_job_unittest.cc:33: "", TEST_MODE_NORMAL,
&GZipServer, 0, OK};
On 2013/10/31 15:23:43, mmenke wrote:
> Probably best to keep the old format here and below - easier to count lines
than
> arguments.

Done.

[commit-bot: I haz the power, 2013-10-31 20:21:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/51953002/1180001

[commit-bot: I haz the power, 2013-10-31 21:09:28]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jamesr, 2013-10-31 22:00:44]

webkit/ lgtm

[akalin, 2013-10-31 22:05:29]

Committed patchset #13 manually as r232219 (presubmit successful).