Getting rid of net::LOAD_ENABLE_LOAD_TIMING

Getting rid of net::LOAD_ENABLE_LOAD_CONTENT.

There are too many load flags. This one is only used by loader code.

BUG=

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

[Lukasz Jagielski, 2014-08-07 11:35:56]

Hi,
Please take a look at my CL proposition.
Best Regards,
Łukasz

[mmenke, 2014-08-07 14:40:55]

https://codereview.chromium.org/451623002/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/451623002/diff/1/content/browser/loader/resou...
content/browser/loader/resource_request_info_impl.h:181: bool
enable_load_timing_;
Did you do any thinking about whether we even need this field, or can just send
load timing information to the other process unconditionally?  It used to be
that load timing information was collected by monitoring NetLog, so devtool
would have to watch for events and then collect events for each request, and
assemble them to construct load timing information, so there was extra overhead
to gathering the information.

The network stack now gathers that information for all requests, so there's
basically no extra overhead if we always send that information to the renderer.

The two questions here are:
1)  Does this open up any timing attacks / security / privacy issues we care
about.
2)  Does providing that information to request that don't expect it expose any
bugs.

The answer for now may just be "Add a TODO to think about this".

[Lukasz Jagielski, 2014-08-07 15:09:11]

On 2014/08/07 14:40:55, mmenke wrote:
>
https://codereview.chromium.org/451623002/diff/1/content/browser/loader/resou...
> File content/browser/loader/resource_request_info_impl.h (right):
> 
>
https://codereview.chromium.org/451623002/diff/1/content/browser/loader/resou...
> content/browser/loader/resource_request_info_impl.h:181: bool
> enable_load_timing_;
> Did you do any thinking about whether we even need this field, or can just
send
> load timing information to the other process unconditionally?  It used to be
> that load timing information was collected by monitoring NetLog, so devtool
> would have to watch for events and then collect events for each request, and
> assemble them to construct load timing information, so there was extra
overhead
> to gathering the information.
> 
> The network stack now gathers that information for all requests, so there's
> basically no extra overhead if we always send that information to the
renderer.
> 
> The two questions here are:
> 1)  Does this open up any timing attacks / security / privacy issues we care
> about.
> 2)  Does providing that information to request that don't expect it expose any
> bugs.
> 
> The answer for now may just be "Add a TODO to think about this".

Blink gathers timing data for the sake of this api
http://www.w3.org/TR/navigation-timing/,
I don't know if there's a chance that some superfluous data appears there thanks
to passing load
timing unconditionally and if this can break something. Sure, I can add a TODO,
there's a fixup
coming up for some broken tests.

[mmenke, 2014-08-07 15:19:44]

On 2014/08/07 15:09:11, Lukasz Jagielski wrote:
> On 2014/08/07 14:40:55, mmenke wrote:
> >
>
https://codereview.chromium.org/451623002/diff/1/content/browser/loader/resou...
> > File content/browser/loader/resource_request_info_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/451623002/diff/1/content/browser/loader/resou...
> > content/browser/loader/resource_request_info_impl.h:181: bool
> > enable_load_timing_;
> > Did you do any thinking about whether we even need this field, or can just
> send
> > load timing information to the other process unconditionally?  It used to be
> > that load timing information was collected by monitoring NetLog, so devtool
> > would have to watch for events and then collect events for each request, and
> > assemble them to construct load timing information, so there was extra
> overhead
> > to gathering the information.
> > 
> > The network stack now gathers that information for all requests, so there's
> > basically no extra overhead if we always send that information to the
> renderer.
> > 
> > The two questions here are:
> > 1)  Does this open up any timing attacks / security / privacy issues we care
> > about.
> > 2)  Does providing that information to request that don't expect it expose
any
> > bugs.
> > 
> > The answer for now may just be "Add a TODO to think about this".
> 
> Blink gathers timing data for the sake of this api
> http://www.w3.org/TR/navigation-timing/,
> I don't know if there's a chance that some superfluous data appears there
thanks
> to passing load
> timing unconditionally and if this can break something. Sure, I can add a
TODO,
> there's a fixup
> coming up for some broken tests.

Great, just a TODO works for me.  Should probably be in RequestInfo, since the
issue of side effects are more renderer-side than browser-side questions.

[mmenke, 2014-08-08 15:09:54]

LGTM, modulo one nit.

https://codereview.chromium.org/451623002/diff/20001/chrome/test/data/webui/n...
File chrome/test/data/webui/net_internals/log_view_painter.js (right):

https://codereview.chromium.org/451623002/diff/20001/chrome/test/data/webui/n...
chrome/test/data/webui/net_internals/log_view_painter.js:933: 'load_flags':
68222976,
If you run into issues with these tests again, you can (optionally) replace
these with LoadTiming.LOAD_FLAG_NAME (Or something like that), to avoid having
to do this again.

https://codereview.chromium.org/451623002/diff/20001/content/child/request_in...
File content/child/request_info.h (right):

https://codereview.chromium.org/451623002/diff/20001/content/child/request_in...
content/child/request_info.h:78: // TODO Investigate if enable_load_timing is
safe to remove.
nit:  Format is TODO(name): ...

Feel free to use TODO(mmenke).

[mmenke, 2014-08-08 15:19:49]

And still LGTM.

[Lukasz Jagielski, 2014-08-08 15:21:31]

On 2014/08/08 15:19:49, mmenke wrote:
> And still LGTM.

Do I still need LGTM from other reviewers?

[mmenke, 2014-08-08 15:50:24]

On 2014/08/08 15:21:31, Lukasz Jagielski wrote:
> On 2014/08/08 15:19:49, mmenke wrote:
> > And still LGTM.
> 
> Do I still need LGTM from other reviewers?

Yes, you do.  I'm an owner for net/ and content/browser/loader, and the js test
change doesn't need an owner, so that leaves content/child, content/test, and
content/comment/.*_messages.h.

Looking at content/common/OWNERS, the _messages files have their own set of
security reviewers, so you should choose any one from the per-file list for
those files.  Picking one randomly, how about kenrb?

content/child has no OWNERS file, so looking at the next level up, I think jam
would be a reasonable reviewer for the content/child and content/test changes.

[Lukasz Jagielski, 2014-08-11 15:10:01]

Hi,
Could you please take a look at my CL proposition?
Regards,
Łukasz

[mmenke, 2014-08-11 15:23:44]

Just to be clear, that was addressed to jam for content/child and content/test,
and kenrb for content/comment/.*messages.  Note that we're just replacing one
network "load flag" sent from the renderer to the browser with a boolean
instead.

On 2014/08/11 15:10:01, Lukasz Jagielski wrote:
> Hi,
> Could you please take a look at my CL proposition?
> Regards,
> Łukasz

[kenrb, 2014-08-11 16:02:02]

ipc lgtm

[jam, 2014-08-11 21:51:32]

lgtm with comment

https://codereview.chromium.org/451623002/diff/40001/content/child/request_in...
File content/child/request_info.h (right):

https://codereview.chromium.org/451623002/diff/40001/content/child/request_in...
content/child/request_info.h:80: bool enable_load_timing;
update the constructor

[Lukasz Jagielski, 2014-08-12 14:50:57]

The CQ bit was checked by ljagielski@opera.com

[commit-bot: I haz the power, 2014-08-12 14:53:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ljagielski@opera.com/451623002/60001

[commit-bot: I haz the power, 2014-08-12 15:42:30]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  linux_chromium_chromeos_rel_swarming 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_...)
  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_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-12 15:51:00]

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

[commit-bot: I haz the power, 2014-08-12 15:51:01]

Try jobs failed on following builders:
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Lukasz Jagielski, 2014-08-13 14:35:57]

The CQ bit was checked by ljagielski@opera.com

[commit-bot: I haz the power, 2014-08-13 14:36:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ljagielski@opera.com/451623002/80001

[commit-bot: I haz the power, 2014-08-13 16:40:13]

Committed patchset #5 (80001) as 289318