Make URLRequest's constructor private, and make URLRequestContext a friend class.

Make URLRequest's constructor private, and make URLRequestContext a friend class.

This will hopefully make it easier to refactor URLRequest construction
behind the scenes.

Also get rid of TestURLRequest, which serves no purpose.

TBR=waffles@chromium.org
BUG=407306
Committed: https://crrev.com/19378d2ee458f4ef60cf80cbe1cb81db22bdabde
Cr-Commit-Position: refs/heads/master@{#293847}

[mmenke, 2014-08-27 21:33:48]

Patchset #5 (id:80001) has been deleted

[mmenke, 2014-08-27 21:33:56]

Patchset #4 (id:60001) has been deleted

[mmenke, 2014-08-27 21:34:03]

Patchset #2 (id:20001) has been deleted

[mmenke, 2014-08-28 14:46:08]

Patchset #6 (id:160001) has been deleted

[mmenke, 2014-08-28 14:46:46]

mmenke@chromium.org changed reviewers:
+ pauljensen@chromium.org

[mmenke, 2014-08-28 14:46:46]

pauljensen:  PTAL

[pauljensen, 2014-08-28 15:48:39]

Unifying all URLRequest construction is nice.  Adding a friend class is fine in
this case.

I'm normally not a fan of transitioning from things constructed on the stack to
scope_ptr's as it adds heap allocation and deallocation, but in this case it
will be so infrequent it won't be noticable.  This is just my low-level compiler
optimization ideals talking.

Anyhow, this all looks good, just remove that one "be".

https://codereview.chromium.org/501163002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/user_script_listener_unittest.cc (right):

https://codereview.chromium.org/501163002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/user_script_listener_unittest.cc:164:
scoped_ptr<net::URLRequest> StartTestRequest(
I like the change to use a scoped_ptr instead of a pointer.

https://codereview.chromium.org/501163002/diff/220001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/501163002/diff/220001/net/url_request/url_req...
net/url_request/url_request.h:708: // URLRequests are always be created by
calling
remove "be"

https://codereview.chromium.org/501163002/diff/220001/net/url_request/url_req...
File net/url_request/url_request_file_job_unittest.cc (left):

https://codereview.chromium.org/501163002/diff/220001/net/url_request/url_req...
net/url_request/url_request_file_job_unittest.cc:233: }  // namespace
nit: I liked this up here as I find it a nice divider between file-scoped and
globally-scoped/accessed-elsewhere items.  I guess the TEST_F() things aren't
really scoped but they are called from outside this file.  This is pretty much a
personal preference.  The style guide doesn't say much about this.  I'm fine
with the way you have it now.

[mmenke, 2014-08-28 16:46:00]

Thanks!

On 2014/08/28 15:48:39, pauljensen wrote:
> Unifying all URLRequest construction is nice.  Adding a friend class is fine
in
> this case.

I'm rarely a fan of friends, but yea, seemed appropriate in this case.

> I'm normally not a fan of transitioning from things constructed on the stack
to
> scope_ptr's as it adds heap allocation and deallocation, but in this case it
> will be so infrequent it won't be noticable.  This is just my low-level
compiler
> optimization ideals talking.
> 
> Anyhow, this all looks good, just remove that one "be".

https://codereview.chromium.org/501163002/diff/220001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/501163002/diff/220001/net/url_request/url_req...
net/url_request/url_request.h:708: // URLRequests are always be created by
calling
On 2014/08/28 15:48:39, pauljensen wrote:
> remove "be"

Done.

https://codereview.chromium.org/501163002/diff/220001/net/url_request/url_req...
File net/url_request/url_request_file_job_unittest.cc (left):

https://codereview.chromium.org/501163002/diff/220001/net/url_request/url_req...
net/url_request/url_request_file_job_unittest.cc:233: }  // namespace
On 2014/08/28 15:48:39, pauljensen wrote:
> nit: I liked this up here as I find it a nice divider between file-scoped and
> globally-scoped/accessed-elsewhere items.  I guess the TEST_F() things aren't
> really scoped but they are called from outside this file.  This is pretty much
a
> personal preference.  The style guide doesn't say much about this.  I'm fine
> with the way you have it now.

Each TEST_F does technically create a new class, which is globally scoped, and
could theoretically have name conflicts, I believe.

I generally go with the whole-file-in-abstract-namespace approach, because it
theoretically keeps stuff out of the global namespace, and it's been suggested
to me a couple times in previous codereviews.  But I really don't feel strongly
about this - I treat it like using braces in single line ifs - I may mention it
on codereviews, or change it, but often ignore it as well.

[pauljensen, 2014-08-28 16:53:18]

LGTM

[mmenke, 2014-08-28 17:01:07]

mmenke@chromium.org changed reviewers:
+ atwilson@chromium.org, battre@chromium.org, bengr@chromium.org,
mkosiba@chromium.org, rdevlin.cronin@chromium.org

[mmenke, 2014-08-28 17:01:07]

This is just a refactoring to get rid of TestURLRequest (Which basically adds
nothing to the URLRequest it inherits from), and to make everything create
URLRequests through the URLRequestContext's function for the purpose.

mkosiba:  Please review android_webview
battre:  Please review browser/extensions/api
rdevlin.cronin:  Please review
browser/extensions/user_script_listener_unittest.cc
bengr:  Please review components/data_reduciton_proxy
atwilson:  Please review components/policy.

[Devlin, 2014-08-28 23:50:33]

On 2014/08/28 17:01:07, mmenke wrote:
> This is just a refactoring to get rid of TestURLRequest (Which basically adds
> nothing to the URLRequest it inherits from), and to make everything create
> URLRequests through the URLRequestContext's function for the purpose.
> 
> mkosiba:  Please review android_webview
> battre:  Please review browser/extensions/api
> rdevlin.cronin:  Please review
> browser/extensions/user_script_listener_unittest.cc
> bengr:  Please review components/data_reduciton_proxy
> atwilson:  Please review components/policy.

browser/extensions/user_script_listener_unittest.cc lgtm

[battre, 2014-08-29 07:43:02]

browser/extensions/api LGTM

[Andrew T Wilson (Slow), 2014-08-29 08:02:21]

policy lgtm

[mkosiba (inactive), 2014-09-01 08:58:15]

android_webview LGTM

[mmenke, 2014-09-03 15:31:19]

bengr:  Ping!

[bengr, 2014-09-05 20:31:52]

lgtm

[mmenke, 2014-09-05 20:34:15]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-09-05 20:46:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/501163002/240001

[commit-bot: I haz the power, 2014-09-05 21:10:52]

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

[commit-bot: I haz the power, 2014-09-05 21:10:57]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/52361)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mmenke, 2014-09-08 20:01:19]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-09-08 20:02:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/501163002/260001

[commit-bot: I haz the power, 2014-09-08 20:12:07]

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

[commit-bot: I haz the power, 2014-09-08 20:12:08]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2014-09-09 00:31:25]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-09-09 00:34:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/501163002/260001

[commit-bot: I haz the power, 2014-09-09 00:49:35]

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

[commit-bot: I haz the power, 2014-09-09 00:49:36]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[mmenke, 2014-09-09 00:58:17]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-09-09 01:09:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/501163002/280001

[commit-bot: I haz the power, 2014-09-09 01:20:31]

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

[commit-bot: I haz the power, 2014-09-09 01:20:32]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/53116)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/58106)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mmenke, 2014-09-09 02:39:46]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2014-09-09 02:55:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mmenke@chromium.org/501163002/300001

[commit-bot: I haz the power, 2014-09-09 04:29:18]

Committed patchset #12 (id:300001) as 513ff525455cb771a1bfb289a163e7a096acfd2b

[commit-bot: I haz the power, 2014-09-10 03:50:19]

Patchset 12 (id:??) landed as
https://crrev.com/19378d2ee458f4ef60cf80cbe1cb81db22bdabde
Cr-Commit-Position: refs/heads/master@{#293847}