Fix ResourceLoadTiming resolution lose issue (chromium part)

Fix ResourceLoadTiming resolution lose issue (chromium part)

To Fix ResourceLoadTiming resolution lose issue, "int" deltas in millisecond will be replaced
by "double" monotonicallyIncreasing time in blink. This change remove previous delta calculation
in glue, and pass nature monotonicallyIncreasing time to blink.

Contributed by pan.deng@intel.com
 
BUG=242452
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202765

[Pan, 2013-05-21 09:39:34]

Please review.
This is glue side change with https://codereview.chromium.org/15265004/

thanks!
Pan

[eustas, 2013-05-21 09:47:37]

https://codereview.chromium.org/15552003/diff/1/webkit/glue/weburlloader_impl.cc
File webkit/glue/weburlloader_impl.cc (left):

https://codereview.chromium.org/15552003/diff/1/webkit/glue/weburlloader_impl...
webkit/glue/weburlloader_impl.cc:160: return static_cast<int>((time -
base_time).InMillisecondsRoundedUp());
I don't think that storing time in double is a good idea.

If we do care about precision, we should store offsets as time ticks.

[Pan, 2013-05-21 12:45:18]

On 2013/05/21 09:47:37, eustas.ru wrote:
>
https://codereview.chromium.org/15552003/diff/1/webkit/glue/weburlloader_impl.cc
> File webkit/glue/weburlloader_impl.cc (left):
> 
>
https://codereview.chromium.org/15552003/diff/1/webkit/glue/weburlloader_impl...
> webkit/glue/weburlloader_impl.cc:160: return static_cast<int>((time -
> base_time).InMillisecondsRoundedUp());
> I don't think that storing time in double is a good idea.
> 
> If we do care about precision, we should store offsets as time ticks.

If blink awares base::TimeTicks, I think it is good to store it.
While, double is enough to represent even sub-microsecond, and there are also
double timestamps used in blink such as navigation timing(DocumentLoadTiming),
so I suggested an easy fix here, make sense? :)

thanks
Pan

[Pan, 2013-05-22 09:17:36]

Add macro and default enable.
This CL must be after https://codereview.chromium.org/15265004/.
please review :)

thanks
Pan

[James Simonsen, 2013-05-22 21:41:05]

lgtm

(not an owner and clueless about compile time flags though)

Agreed that a double containing seconds is the unfortunate unit for time in
Blink. We're stuck with it for now.

[Pan, 2013-05-23 03:54:19]

On 2013/05/22 21:41:05, James Simonsen wrote:
> lgtm
> 
> (not an owner and clueless about compile time flags though)
> 
> Agreed that a double containing seconds is the unfortunate unit for time in
> Blink. We're stuck with it for now.

thanks!
@darin, Blink side change https://codereview.chromium.org/15265004/ has been
made, could you please review this :)

Pan

[Pan, 2013-05-23 23:47:59]

Hi, @darin, jamesr, kinuko, michaeln, and tony

This is the 2nd patch we would like to make in chromium part to fix timing
resolution issue.
We have landed 1st
https://codereview.chromium.org/15265004/
and going to land 3rd https://codereview.chromium.org/15863002/ which must
happen after this.

could you please help review this? :)
thanks!
Pan

[michaeln, 2013-05-24 00:46:01]

c++ looks ok (modulo the nit), not entirely sure what i'm looking at with the
gpyi voodoo... but tryjobs rule.

https://codereview.chromium.org/15552003/diff/5001/webkit/glue/weburlloader_i...
File webkit/glue/weburlloader_impl.cc (right):

https://codereview.chromium.org/15552003/diff/5001/webkit/glue/weburlloader_i...
webkit/glue/weburlloader_impl.cc:162: (load_timing.proxy_resolve_start -
TimeTicks()).InSecondsF());
Odd... ticksFromThePast - ticksNow? Doesn't that make for a negative number or
am i somehow crosseyed?

Oh... nevermind... TimeTicks() produces an instance with ticks_ = 0.

Maybe make a local to help clarify, TimeTicks kNullTIcks, and use that local in
these calculations

[Pan, 2013-05-24 01:04:25]

> Maybe make a local to help clarify, TimeTicks kNullTIcks, and use that local
in
> these calculations

right, thanks! :)

Pan

[michaeln, 2013-05-24 19:17:45]

code change l g t m

but i have no idea if the gpy voodoo is correct or if the two-side patch dance
will actually work out w/o busting some build somewhere?

tryjobs would help answer that question, please run tryjobs

I see you've got the webkit api setup to support one or the other style of
interface (delta ints, or abs floats) in a mutually exclusive way. Sometimes we
leave old and new WebKitAPI interfaces in place (and callable) when making a
transition like this, so upstream webkit bots could survive with some skew in
chromium checkouts (and vice versa).

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

https://codereview.chromium.org/15552003/diff/14001/webkit/glue/weburlloader_...
webkit/glue/weburlloader_impl.cc:158: TimeTicks kNullTicks;
nit: const TimeTicks kNullTicks;

[Pan, 2013-05-27 23:28:30]

On 2013/05/24 19:17:45, michaeln wrote:
> code change l g t m
> 
> but i have no idea if the gpy voodoo is correct or if the two-side patch dance
> will actually work out w/o busting some build somewhere?
> 
> tryjobs would help answer that question, please run tryjobs

thanks, tryjobs result is there, although mac_asan is failed, it seems it has
been broken for some time, :) 

> I see you've got the webkit api setup to support one or the other style of
> interface (delta ints, or abs floats) in a mutually exclusive way. Sometimes
we
> leave old and new WebKitAPI interfaces in place (and callable) when making a
> transition like this, so upstream webkit bots could survive with some skew in
> chromium checkouts (and vice versa).
Got it, thanks, I will make transition as your solution next time :)


>
https://codereview.chromium.org/15552003/diff/14001/webkit/glue/weburlloader_...
> File webkit/glue/weburlloader_impl.cc (right):
> 
>
https://codereview.chromium.org/15552003/diff/14001/webkit/glue/weburlloader_...
> webkit/glue/weburlloader_impl.cc:158: TimeTicks kNullTicks;
> nit: const TimeTicks kNullTicks;

thanks, done.

[michaeln, 2013-05-28 19:01:01]

lgtm

[commit-bot: I haz the power, 2013-05-28 22:29:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pan.deng@intel.com/15552003/21001

[commit-bot: I haz the power, 2013-05-28 23:01:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-29 02:30:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pan.deng@intel.com/15552003/21001

[commit-bot: I haz the power, 2013-05-29 03:54:44]

Change committed as 202765