LoadTiming in net part 7: Hooking it all up

LoadTiming in net part 7:  Hooking it all up

Use the new load timing implementation in net/ to replace LoadTimingObserver.

This should fix reported times when using a SPDY proxy, and results
in more heavily tested and (hopefully) less regression-prone code.

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

[mmenke, 2013-02-11 20:21:53]

I'll add a higher level owner once I get the initial signoffs for this.

[mmenke, 2013-02-11 20:23:03]

On 2013/02/11 20:21:53, Matt Menke wrote:
> I'll add a higher level owner once I get the initial signoffs for this.

Also, there's no way I'm landing this before branch, even in the unlikely case I
get all the signoffs by then.

[eroman, 2013-02-13 02:07:20]

lgtm

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
File chrome/browser/net/load_timing_browsertest.cc (right):

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
chrome/browser/net/load_timing_browsertest.cc:89: // and for times retriever
from the renderer process.
retriever ---> retrieved ?

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
chrome/browser/net/load_timing_browsertest.cc:131: // Need to delay starting
until the largest of the times has ellapsed.
nit: ellapsed -> elapsed

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
chrome/browser/net/load_timing_browsertest.cc:150: // Make sure enough time has
ellapsed since start was called.  If this
elapsed

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
chrome/browser/net/load_timing_browsertest.cc:151: // fails, the text fixture
itself is flaky.
"text fixture" --> test fixture?

https://codereview.chromium.org/12094085/diff/68055/content/common/resource_d...
File content/common/resource_dispatcher.cc (right):

https://codereview.chromium.org/12094085/diff/68055/content/common/resource_d...
content/common/resource_dispatcher.cc:632: InterProcessTimeTicksConverter
converter(
Interesting, I didn't even realize this was a problem! Seriously wondering
whether we can't come up with a TimeTicks class that has consistent values
across processes and suspends...

https://codereview.chromium.org/12094085/diff/68055/webkit/glue/weburlloader_...
File webkit/glue/weburlloader_impl.cc (right):

https://codereview.chromium.org/12094085/diff/68055/webkit/glue/weburlloader_...
webkit/glue/weburlloader_impl.cc:158: return -1;
are downstream consumers expecting this -1?

[mmenke, 2013-02-13 02:26:46]

Thanks for the feedback!

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
File chrome/browser/net/load_timing_browsertest.cc (right):

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
chrome/browser/net/load_timing_browsertest.cc:89: // and for times retriever
from the renderer process.
On 2013/02/13 02:07:20, eroman wrote:
> retriever ---> retrieved ?

Done.

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
chrome/browser/net/load_timing_browsertest.cc:131: // Need to delay starting
until the largest of the times has ellapsed.
On 2013/02/13 02:07:20, eroman wrote:
> nit: ellapsed -> elapsed

Done.

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
chrome/browser/net/load_timing_browsertest.cc:150: // Make sure enough time has
ellapsed since start was called.  If this
On 2013/02/13 02:07:20, eroman wrote:
> elapsed

Done.

https://codereview.chromium.org/12094085/diff/68055/chrome/browser/net/load_t...
chrome/browser/net/load_timing_browsertest.cc:151: // fails, the text fixture
itself is flaky.
On 2013/02/13 02:07:20, eroman wrote:
> "text fixture" --> test fixture?

Done (That's one typo I make a lot, for some odd reason).

https://codereview.chromium.org/12094085/diff/68055/content/common/resource_d...
File content/common/resource_dispatcher.cc (right):

https://codereview.chromium.org/12094085/diff/68055/content/common/resource_d...
content/common/resource_dispatcher.cc:632: InterProcessTimeTicksConverter
converter(
On 2013/02/13 02:07:20, eroman wrote:
> Interesting, I didn't even realize this was a problem! Seriously wondering
> whether we can't come up with a TimeTicks class that has consistent values
> across processes and suspends...

That would be handy.

https://codereview.chromium.org/12094085/diff/68055/webkit/glue/weburlloader_...
File webkit/glue/weburlloader_impl.cc (right):

https://codereview.chromium.org/12094085/diff/68055/webkit/glue/weburlloader_...
webkit/glue/weburlloader_impl.cc:158: return -1;
On 2013/02/13 02:07:20, eroman wrote:
> are downstream consumers expecting this -1?

-1 looks to be correct, looking at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Numbers < 0 return the time of the previous phase, which is what the spec
requires.  Except for ssl_start, where negative times result in returning 0,
which is also correct, per spec.

[pfeldman, 2013-02-18 12:34:22]

Do you guys expect any input from the devtools team? Frankly speaking, I am lost
in the changes, so will only be able to assess the end result :)

[mmenke, 2013-02-18 13:38:04]

On 2013/02/18 12:34:22, pfeldman wrote:
> Do you guys expect any input from the devtools team? Frankly speaking, I am
lost
> in the changes, so will only be able to assess the end result :)

Since you landed the original load timing observer code that this replaces
(Admittedly, 2+ years ago) as well as reviewed the CLs that added
inter_process_time_ticks_converter and the new browser test is modelled off of a
devtools browser test, I had thought you'd be a good second reviewer for this,
particularly for the webkit/ and common/ changes.

Eric:  Was your signoff based on the assumption pfeldman would take a closer
look at parts of the CL?

[eroman, 2013-02-19 20:14:30]

I reviewed all the files, you can consider my review authoritative.

If pfeldman wants to take a look too I welcome his input!

[anagy4698, 2013-02-19 20:21:50]

Ejha, az összes fontos üzenetet elolvasta a beérkezett levelek közül!


2013/2/19 <eroman@chromium.org>

> I reviewed all the files, you can consider my review authoritative.
>
> If pfeldman wants to take a look too I welcome his input!
>
>
https://codereview.chromium.**org/12094085/<https://codereview.chromium.org/1...
>



-- 
anagy4698@gmail.com.

[mmenke, 2013-02-19 20:24:49]

[+darin]:  PTAL, thanks!

[mmenke, 2013-02-19 20:28:11]

On 2013/02/19 20:24:49, mmenke wrote:
> [+darin]:  PTAL, thanks!

To elaborate - I need an owner for basically everything but chrome/browser/net. 
You seemed the best fit.

[mmenke, 2013-02-28 15:24:13]

Darin:  Ping.

[darin (slow to review), 2013-02-28 16:57:35]

I just looked at src/content/ and src/webkit/ changes.

https://chromiumcodereview.appspot.com/12094085/diff/99004/content/common/res...
File content/common/resource_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/12094085/diff/99004/content/common/res...
content/common/resource_dispatcher.cc:658: renderer_info->load_timing.field =
converter.ToLocalTimeTicks( \
nit: it seems like you could save some generated code if you had a subroutine
that encapsulated the
converter.ToLocalTimeTicks(FromTimeTicks(...)).ToTimeTicks() business.

https://chromiumcodereview.appspot.com/12094085/diff/99004/webkit/glue/weburl...
File webkit/glue/weburlloader_impl.cc (right):

https://chromiumcodereview.appspot.com/12094085/diff/99004/webkit/glue/weburl...
webkit/glue/weburlloader_impl.cc:202: timing.setRequestTime(
nit: How about breaking this out into its own helper function? 
PopulateURLLoadTiming?

[mmenke, 2013-02-28 17:36:55]

Thanks for the feedback!

https://chromiumcodereview.appspot.com/12094085/diff/99004/content/common/res...
File content/common/resource_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/12094085/diff/99004/content/common/res...
content/common/resource_dispatcher.cc:658: renderer_info->load_timing.field =
converter.ToLocalTimeTicks( \
On 2013/02/28 16:57:35, darin wrote:
> nit: it seems like you could save some generated code if you had a subroutine
> that encapsulated the
> converter.ToLocalTimeTicks(FromTimeTicks(...)).ToTimeTicks() business.

Done.  I assume this ugliness is deliberate, to emphasize the difference between
remote and local times, so I added a function to this file rather than to the
converter itself.

https://chromiumcodereview.appspot.com/12094085/diff/99004/webkit/glue/weburl...
File webkit/glue/weburlloader_impl.cc (right):

https://chromiumcodereview.appspot.com/12094085/diff/99004/webkit/glue/weburl...
webkit/glue/weburlloader_impl.cc:202: timing.setRequestTime(
On 2013/02/28 16:57:35, darin wrote:
> nit: How about breaking this out into its own helper function? 
> PopulateURLLoadTiming?

Done.

[darin (slow to review), 2013-02-28 19:36:42]

LGTM

https://chromiumcodereview.appspot.com/12094085/diff/114001/content/common/re...
File content/common/resource_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/12094085/diff/114001/content/common/re...
content/common/resource_dispatcher.cc:38: // Converts |time| from a remote to
local TimeTicks, overrwriting the original
nit: "overrwriting"

nit: RemoteToLocalTimeTicks would be a slightly clearer name.

https://chromiumcodereview.appspot.com/12094085/diff/114001/content/common/re...
content/common/resource_dispatcher.cc:671: net::LoadTimingInfo* load_timing =
&renderer_info->load_timing;
oh nice, this actually led to being able to kill the macro!

[mmenke, 2013-02-28 19:40:03]

Thanks for the feedback!

https://chromiumcodereview.appspot.com/12094085/diff/114001/content/common/re...
File content/common/resource_dispatcher.cc (right):

https://chromiumcodereview.appspot.com/12094085/diff/114001/content/common/re...
content/common/resource_dispatcher.cc:38: // Converts |time| from a remote to
local TimeTicks, overrwriting the original
On 2013/02/28 19:36:43, darin wrote:
> nit: "overrwriting"
> 
> nit: RemoteToLocalTimeTicks would be a slightly clearer name.

Done and done.  Good name suggestion - clearer and no longer than the old one.

[commit-bot: I haz the power, 2013-02-28 19:44:06]

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

[commit-bot: I haz the power, 2013-02-28 19:44:20]

Presubmit check for 12094085-106005 failed and returned exit status 1.

INFO:root:Found 17 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    content/common/resource_messages.h

Presubmit checks took 4.4s to calculate.

[darin (slow to review), 2013-02-28 19:52:03]

need to get a security review on changes to *_messages.h files.

-darin

On Thu, Feb 28, 2013 at 11:44 AM, <commit-bot@chromium.org> wrote:

> Presubmit check for 12094085-106005 failed and returned exit status 1.
>
> INFO:root:Found 17 file(s).
>
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/**chromium/PRESUBMIT.py
> Running /b/commit-queue/workdir/**chromium/chrome/PRESUBMIT.py
>
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     content/common/resource_**messages.h
>
> Presubmit checks took 4.4s to calculate.
>
>
>
>
https://chromiumcodereview.**appspot.com/12094085/<https://chromiumcodereview...
>

[mmenke, 2013-02-28 19:59:50]

On 2013/02/28 19:52:03, darin wrote:
> need to get a security review on changes to *_messages.h files.
> 
> -darin

Thanks, makes sense.  The OWNERs suggestions have historically so consistently
bad that I've been trained to ignore them.

[mmenke, 2013-02-28 20:15:57]

[+jschuh]:  Could I get a security review for the resource_messages changes? 
Basically just replacing an old structure (webkit_glue::ResourceLoadTimingInfo)
with a new one that comes directly from the network stack (net::LoadTimingInfo).

Thanks!

[jschuh, 2013-02-28 21:14:12]

lgtm. the same effective values are getting passed over ipc so it doesn't appear
to have a security impact.

[commit-bot: I haz the power, 2013-02-28 21:21:27]

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

[mmenke, 2013-03-05 17:23:29]

Just in case anyone's curious why this hasn't landed yet - there's an issue with
InterProcessTimeTicksConverter being disabled, and then webkit DCHECKing when
this results in a requestStart time that's before fetchStart, so I need to fix
that before I can land.

[commit-bot: I haz the power, 2013-03-27 14:27:30]

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

[commit-bot: I haz the power, 2013-03-27 16:45:09]

Change committed as 190955