Blink Platform: Remove time functions from Platform.

Blink Platform: Remove time functions from Platform.

Simplification!

To support the use case of mocking timers for tests, CurrentTime.h now
provides a customization function. Currently, it overrides the three
functions at once, but practically that wouldn't be a problem.

Some unit tests try to override timing functions by replacing
Platform. However, CachingCorrectnessTest and TextFinderTest seemed to
fail to do so, as they apparently lacked Platform::initialize()
calls. I'm not sure why these tests have not been failing. Anyways, this
patch "fixes" these issues, too.

BUG=565765
Committed: https://crrev.com/a5466f457a195abd0f03066439c0efae3b5e14b0
Cr-Commit-Position: refs/heads/master@{#375524}

[Yuta Kitamura, 2015-12-25 07:17:58]

The CQ bit was checked by yutak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-25 07:18:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550563002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550563002/1

[commit-bot: I haz the power, 2015-12-25 07:26:43]

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

[commit-bot: I haz the power, 2015-12-25 07:26:44]

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

[Yuta Kitamura, 2015-12-25 09:03:44]

yutak@chromium.org changed reviewers:
+ esprehn@chromium.org, haraken@chromium.org

[Yuta Kitamura, 2015-12-25 09:03:44]

(No rush, I don't expect this to get landed this year.)

[haraken, 2015-12-28 02:24:27]

LGTM

I'm just curious but does this CL speed up performance.now()? The slowness of
performance.now() is caused by a bunch of unnecessary indirection between
Chromium and Blink, and that's one of the reasons we started the Onion Soup.
This CL removes the indirection.

https://codereview.chromium.org/1550563002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1550563002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:62:
WTF::setTimeFunctionsForTesting([]() -> double {

Nit: I've never seen Blink using the []()-> syntax. I'd slightly prefer not
using an advanced feature in C++.

https://codereview.chromium.org/1550563002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1550563002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/testing/RunAllTests.cpp:46:
WTF::setTimeFunctionsForTesting([]() -> double {

Ditto.

[Yuta Kitamura, 2016-01-05 05:31:06]

https://codereview.chromium.org/1550563002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1550563002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:62:
WTF::setTimeFunctionsForTesting([]() -> double {
On 2015/12/28 02:24:27, haraken wrote:
> 
> Nit: I've never seen Blink using the []()-> syntax. I'd slightly prefer not
> using an advanced feature in C++.

I think mine is clearer and simpler, but I'd defer this
to Elliott. This is C++11's lambda syntax, just like
writing "function () { ... }" in JavaScript.

Chromium allows lambdas under certain conditions:
https://chromium-cpp.appspot.com/

[Yuta Kitamura, 2016-01-05 06:49:07]

On 2015/12/28 02:24:27, haraken wrote:
> LGTM
> 
> I'm just curious but does this CL speed up performance.now()? The slowness of
> performance.now() is caused by a bunch of unnecessary indirection between
> Chromium and Blink, and that's one of the reasons we started the Onion Soup.
> This CL removes the indirection.
> 

According to my casual local testing, my patch seems to improve
the performance.now() performance (tested on Linux Official build).

  * ToT: saturated at ~1.36M calls/sec
  * With this patch applied: saturated at ~1.42M calls/sec
  => ~4.4% improvement

Not so drastic, but a nice side effect ;)

[haraken, 2016-01-05 06:49:57]

On 2016/01/05 06:49:07, Yuta Kitamura wrote:
> On 2015/12/28 02:24:27, haraken wrote:
> > LGTM
> > 
> > I'm just curious but does this CL speed up performance.now()? The slowness
of
> > performance.now() is caused by a bunch of unnecessary indirection between
> > Chromium and Blink, and that's one of the reasons we started the Onion Soup.
> > This CL removes the indirection.
> > 
> 
> According to my casual local testing, my patch seems to improve
> the performance.now() performance (tested on Linux Official build).
> 
>   * ToT: saturated at ~1.36M calls/sec
>   * With this patch applied: saturated at ~1.42M calls/sec
>   => ~4.4% improvement
> 
> Not so drastic, but a nice side effect ;)

Great!

[esprehn, 2016-01-05 07:01:09]

https://codereview.chromium.org/1550563002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.cc (left):

https://codereview.chromium.org/1550563002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:289: return
renderer_scheduler_->MonotonicallyIncreasingTimeSeconds();
The scheduler implements virtual time, where is that done now?

[Yuta Kitamura, 2016-01-05 09:17:46]

yutak@chromium.org changed reviewers:
+ alexclarke@chromium.org

[Yuta Kitamura, 2016-01-05 09:17:46]

https://codereview.chromium.org/1550563002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.cc (left):

https://codereview.chromium.org/1550563002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:289: return
renderer_scheduler_->MonotonicallyIncreasingTimeSeconds();
On 2016/01/05 07:01:09, esprehn wrote:
> The scheduler implements virtual time, where is that done now?

Good point. This was my oversight, but after looking at some
scheduler code I started to wonder if the ability to take over
Blink's clock for schedulers was really necessary.

The production implementation of CurrentTimeSeconds() is
base::Time::Now().ToDoubleT() which is same as what my patch
does, thus the sole purpose of this hijacking is for testing.
However, according to my try results, all try bots are green.
So...?

I think I need some wisdom from the author of this code about
the real intent of this... alexclarke, WDYT?

If we really need this ability, we may have to make rather
intrusive API so the modules outside Blink can control Blink's
clock, or otherwise leave the Platform interface as is.

[alex clarke (OOO till 29th), 2016-01-05 10:49:24]

https://codereview.chromium.org/1550563002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.cc (left):

https://codereview.chromium.org/1550563002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:289: return
renderer_scheduler_->MonotonicallyIncreasingTimeSeconds();
On 2016/01/05 09:17:46, Yuta Kitamura wrote:
> On 2016/01/05 07:01:09, esprehn wrote:
> > The scheduler implements virtual time, where is that done now?
> 
> Good point. This was my oversight, but after looking at some
> scheduler code I started to wonder if the ability to take over
> Blink's clock for schedulers was really necessary.

This is for Headless Chrome
(https://docs.google.com/document/d/1dIMHIl1xutUXqXWRXqXrDd3bo9hachIt_ZkPK_Bsh...)

> 
> The production implementation of CurrentTimeSeconds() is
> base::Time::Now().ToDoubleT() which is same as what my patch
> does, thus the sole purpose of this hijacking is for testing.
> However, according to my try results, all try bots are green.
> So...?
We don't have good integration testing for this (yet) sadly.

> 
> I think I need some wisdom from the author of this code about
> the real intent of this... alexclarke, WDYT?
> 
> If we really need this ability, we may have to make rather
> intrusive API so the modules outside Blink can control Blink's
> clock, or otherwise leave the Platform interface as is.

For Headless parts of blink need the real clock still but most of it needs the
virtual clock.  I don't have a strong opinion on how this stuff should be wired
up so if there's something we can do that fits in better with Onion Soup then we
should explore that.

We'd be happy to talk about this over VC.

[Yuta Kitamura, 2016-01-07 07:51:14]

On 2016/01/05 10:49:24, alexclarke1 wrote:
>
https://codereview.chromium.org/1550563002/diff/1/content/renderer/renderer_b...
> File content/renderer/renderer_blink_platform_impl.cc (left):
> 
>
https://codereview.chromium.org/1550563002/diff/1/content/renderer/renderer_b...
> content/renderer/renderer_blink_platform_impl.cc:289: return
> renderer_scheduler_->MonotonicallyIncreasingTimeSeconds();
> On 2016/01/05 09:17:46, Yuta Kitamura wrote:
> > On 2016/01/05 07:01:09, esprehn wrote:
> > > The scheduler implements virtual time, where is that done now?
> > 
> > Good point. This was my oversight, but after looking at some
> > scheduler code I started to wonder if the ability to take over
> > Blink's clock for schedulers was really necessary.
> 
> This is for Headless Chrome
>
(https://docs.google.com/document/d/1dIMHIl1xutUXqXWRXqXrDd3bo9hachIt_ZkPK_Bsh...)
> 
> > 
> > The production implementation of CurrentTimeSeconds() is
> > base::Time::Now().ToDoubleT() which is same as what my patch
> > does, thus the sole purpose of this hijacking is for testing.
> > However, according to my try results, all try bots are green.
> > So...?
> We don't have good integration testing for this (yet) sadly.
> 
> > 
> > I think I need some wisdom from the author of this code about
> > the real intent of this... alexclarke, WDYT?
> > 
> > If we really need this ability, we may have to make rather
> > intrusive API so the modules outside Blink can control Blink's
> > clock, or otherwise leave the Platform interface as is.
> 
> For Headless parts of blink need the real clock still but most of it needs the
> virtual clock.  I don't have a strong opinion on how this stuff should be
wired
> up so if there's something we can do that fits in better with Onion Soup then
we
> should explore that.
> 
> We'd be happy to talk about this over VC.

Thanks for the link. After reading the document, I think I
should give up on landing this patch; the ability to inject
custom timers seems necessary, and we can't get rid of the
time functions from Platform.

<aside>
At first, the changes for adding virtual timers looked strange,
because they basically meant that content/ did depend on
components/scheduler/ for the time source. Generally content/
can't depend on components/, but it turned out that this case
was OK, because components/scheduler/ did not depend on content/.

However, I still think the dependency story around schedulers
is confusing; I feel that scheduler is the part of basic browser
functionality, so I think it'd be much clearer if the scheduler
stuff was put somewhere in content/. At least virtual clock should
live in content/ IMO.
</aside>

[esprehn, 2016-01-07 08:57:14]

I don't think this is okay, ghost rider should not be imposing a 5% penalty
on performance.now() for all web content.

Given that there's no automated test coverage, this feature is not
shipping, and it's a performance improvement for web content I think we
should land this and ghost rider should look into how to do this without
imposing this overhead. They also need to get tests, its not okay to have
something like this (especially new code) without coverage.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[esprehn, 2016-01-07 09:03:25]

I don't think this is okay, ghost rider should not be imposing a 5% penalty
on performance.now() for all web content.

Given that there's no automated test coverage, this feature is not
shipping, and it's a performance improvement for web content I think we
should land this and ghost rider should look into how to do this without
imposing this overhead. They also need to get tests, its not okay to have
something like this (especially new code) without coverage.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[alex clarke (OOO till 29th), 2016-01-07 10:42:25]

I don't want ghost rider to block this patch, I suggest you land it and we can
figure out how to properly plumb in the virtual time later.

[dtapuska, 2016-02-05 18:17:50]

On 2016/01/07 10:42:25, alexclarke1 wrote:
> I don't want ghost rider to block this patch, I suggest you land it and we can
> figure out how to properly plumb in the virtual time later.

What is the status of this change?

Personally I would have changed CurrentTime.h to take a base::Clock object 
and the renderer's scheduler could just set its clock object as the override.

Get rid of the function ptr overrides; and then if someone wants to override it;
they provide the base::Clock object.

Right now to implement the scheduler override; you need to check for a virtual
function override; call it; and then call a virtual on the base::Clock object.
You can probably skip the virtual function call I'm proposing.

[dtapuska, 2016-02-05 18:18:16]

Description was changed from

==========
Blink Platform: Remove time functions from Platform.

Simplification!

To support the use case of mocking timers for tests, CurrentTime.h now
provides a customization function. Currently, it overrides the three
functions at once, but practically that wouldn't be a problem.

Some unit tests try to override timing functions by replacing
Platform. However, CachingCorrectnessTest and TextFinderTest seemed to
fail to do so, as they apparently lacked Platform::initialize()
calls. I'm not sure why these tests have not been failing. Anyways, this
patch "fixes" these issues, too.

BUG=565765
==========

to

==========
Blink Platform: Remove time functions from Platform.

Simplification!

To support the use case of mocking timers for tests, CurrentTime.h now
provides a customization function. Currently, it overrides the three
functions at once, but practically that wouldn't be a problem.

Some unit tests try to override timing functions by replacing
Platform. However, CachingCorrectnessTest and TextFinderTest seemed to
fail to do so, as they apparently lacked Platform::initialize()
calls. I'm not sure why these tests have not been failing. Anyways, this
patch "fixes" these issues, too.

BUG=565765
==========

[esprehn, 2016-02-06 02:49:12]

On 2016/02/05 at 18:17:50, dtapuska wrote:
> On 2016/01/07 10:42:25, alexclarke1 wrote:
> > I don't want ghost rider to block this patch, I suggest you land it and we
can
> > figure out how to properly plumb in the virtual time later.
> 
> What is the status of this change?
> 
> Personally I would have changed CurrentTime.h to take a base::Clock object 
> and the renderer's scheduler could just set its clock object as the override.
> 
> Get rid of the function ptr overrides; and then if someone wants to override
it; they provide the base::Clock object.
> 
> Right now to implement the scheduler override; you need to check for a virtual
function override; call it; and then call a virtual on the base::Clock object.
You can probably skip the virtual function call I'm proposing.

That sounds great to me, we need to land either this patch , or the one you talk
about. yutak@ Would you land this now? :)

[Yuta Kitamura, 2016-02-08 09:56:53]

On 2016/02/06 02:49:12, esprehn wrote:
> On 2016/02/05 at 18:17:50, dtapuska wrote:
> > On 2016/01/07 10:42:25, alexclarke1 wrote:
> > > I don't want ghost rider to block this patch, I suggest you land it and we
> can
> > > figure out how to properly plumb in the virtual time later.
> > 
> > What is the status of this change?
> > 
> > Personally I would have changed CurrentTime.h to take a base::Clock object 
> > and the renderer's scheduler could just set its clock object as the
override.
> > 
> > Get rid of the function ptr overrides; and then if someone wants to override
> it; they provide the base::Clock object.
> > 
> > Right now to implement the scheduler override; you need to check for a
virtual
> function override; call it; and then call a virtual on the base::Clock object.
> You can probably skip the virtual function call I'm proposing.
> 
> That sounds great to me, we need to land either this patch , or the one you
talk
> about. yutak@ Would you land this now? :)

I couldn't follow the reasoning, but if it's fine to have
the timer hook removed temporarily, I'm happy to land this.

Lemme try to resurrect the patch...

[Yuta Kitamura, 2016-02-08 10:34:03]

The CQ bit was checked by yutak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-08 10:35:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550563002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550563002/20001

[Yuta Kitamura, 2016-02-08 10:46:53]

yutak@chromium.org changed reviewers:
+ jam@chromium.org, tkent@chromium.org

[Yuta Kitamura, 2016-02-08 10:46:54]

OK I think PS2 is up-to-date, PTAL:

haraken: overall, for sanity check between PS1 and 2
tkent: as WebKit/public/ owner
jam: as components/html_viewer/ and content/ owner

Thanks!

[alex clarke (OOO till 29th), 2016-02-08 10:53:44]

On 2016/02/05 18:17:50, dtapuska wrote:
> On 2016/01/07 10:42:25, alexclarke1 wrote:
> > I don't want ghost rider to block this patch, I suggest you land it and we
can
> > figure out how to properly plumb in the virtual time later.
> 
> What is the status of this change?
> 
> Personally I would have changed CurrentTime.h to take a base::Clock object 
> and the renderer's scheduler could just set its clock object as the override.
That sounds like a good change for test-ability.  As far as the scheduler and
virtual time goes we now think virtual time should be a property of the
WebTaskRunner rather than of the platform.  So hopefully this patch can go
ahead.


> 
> Get rid of the function ptr overrides; and then if someone wants to override
it;
> they provide the base::Clock object.
> 
> Right now to implement the scheduler override; you need to check for a virtual
> function override; call it; and then call a virtual on the base::Clock object.
> You can probably skip the virtual function call I'm proposing.

[commit-bot: I haz the power, 2016-02-08 11:37:12]

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

[commit-bot: I haz the power, 2016-02-08 11:37:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2016-02-08 17:21:11]

lgtm

[Yuta Kitamura, 2016-02-09 08:15:29]

The CQ bit was checked by yutak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-09 08:15:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550563002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550563002/40001

[commit-bot: I haz the power, 2016-02-09 08:19:23]

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

[commit-bot: I haz the power, 2016-02-09 08:19:24]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuta Kitamura, 2016-02-09 08:35:32]

The CQ bit was checked by yutak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-09 08:36:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550563002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550563002/60001

[Yuta Kitamura, 2016-02-09 09:07:22]

haraken, tkent: PTAL? PS4 seems fine.

haraken: overall, for sanity check between PS1 and 4
tkent: as WebKit/public/ owner

Thanks!

[Yuta Kitamura, 2016-02-09 09:07:23]

haraken, tkent: PTAL? PS4 seems fine.

haraken: overall, for sanity check between PS1 and 4
tkent: as WebKit/public/ owner

Thanks!

[haraken, 2016-02-09 09:13:59]

Sorry, I missed this one...

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:57:
WTF::setTimeFunctionsForTesting([]() -> double {

It looks like that a lambda function in this style is not allowed?

http://chromium-cpp.appspot.com/

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/CurrentTime.cpp (right):

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/CurrentTime.cpp:39: double currentTime()

Given that this method is performance-sensitive, I'd inline it.

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/CurrentTime.cpp:41: if
(mockTimeFunctionForTesting)

I'd add UNLIKELY.

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/CurrentTime.cpp:46: double
monotonicallyIncreasingTime()

Ditto.

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/testing/RunAllTests.cpp:42:
WTF::setTimeFunctionsForTesting([]() -> double {

It looks like that a lambda function in this style is not allowed?

http://chromium-cpp.appspot.com/

[commit-bot: I haz the power, 2016-02-09 09:37:01]

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

[commit-bot: I haz the power, 2016-02-09 09:37:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuta Kitamura, 2016-02-09 09:54:57]

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:57:
WTF::setTimeFunctionsForTesting([]() -> double {
On 2016/02/09 09:13:58, haraken wrote:
> 
> It looks like that a lambda function in this style is not allowed?
> 
> http://chromium-cpp.appspot.com/

It's in the "allowed features" section (with notes).

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/CurrentTime.cpp (right):

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/CurrentTime.cpp:39: double currentTime()
On 2016/02/09 09:13:58, haraken wrote:
> 
> Given that this method is performance-sensitive, I'd inline it.

Hm, that would make mockTimeFunctionForTesting public.
Is that fine for you?

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/CurrentTime.cpp:41: if
(mockTimeFunctionForTesting)
On 2016/02/09 09:13:58, haraken wrote:
> 
> I'd add UNLIKELY.

I'm almost certain that PGO (profile-guided optimization)
can do that for us; generally speaking, I think (UN)LIKELY
macros should be added only after the profilers determine
the bottlenecks...

[esprehn, 2016-02-09 10:20:48]

Are we actually building with PGO enabled anywhere? We do something like that on
windows, but I don't think we do on other platforms.

[tkent, 2016-02-10 00:04:39]

third_party/WebKit lgtm

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:57:
WTF::setTimeFunctionsForTesting([]() -> double {
On 2016/02/09 at 09:54:56, Yuta Kitamura wrote:
> On 2016/02/09 09:13:58, haraken wrote:
> > 
> > It looks like that a lambda function in this style is not allowed?
> > 
> > http://chromium-cpp.appspot.com/
> 
> It's in the "allowed features" section (with notes).

Lambda is ok, but you don't need to convert existing function to lambda in this
CL.  It would be noisy for reviewers.

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/CurrentTime.cpp (right):

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/CurrentTime.cpp:41: if
(mockTimeFunctionForTesting)
On 2016/02/09 at 09:54:57, Yuta Kitamura wrote:
> On 2016/02/09 09:13:58, haraken wrote:
> > 
> > I'd add UNLIKELY.
> 
> I'm almost certain that PGO (profile-guided optimization)
> can do that for us; generally speaking, I think (UN)LIKELY
> macros should be added only after the profilers determine
> the bottlenecks...

I agree with yutak.

[Yuta Kitamura, 2016-02-15 07:26:39]

haraken: PTAL?

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/testing/RunAllTests.cpp (right):

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/testing/RunAllTests.cpp:57:
WTF::setTimeFunctionsForTesting([]() -> double {
On 2016/02/10 00:04:39, tkent wrote:
> On 2016/02/09 at 09:54:56, Yuta Kitamura wrote:
> > On 2016/02/09 09:13:58, haraken wrote:
> > > 
> > > It looks like that a lambda function in this style is not allowed?
> > > 
> > > http://chromium-cpp.appspot.com/
> > 
> > It's in the "allowed features" section (with notes).
> 
> Lambda is ok, but you don't need to convert existing function to lambda in
this
> CL.  It would be noisy for reviewers.

Reverted.

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/wtf/CurrentTime.cpp (right):

https://codereview.chromium.org/1550563002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/wtf/CurrentTime.cpp:39: double currentTime()
On 2016/02/09 09:13:58, haraken wrote:
> 
> Given that this method is performance-sensitive, I'd inline it.

According to my local testing, inlining had a negative impact
on perf.now() throughput, so I left these out-of-line.

[haraken, 2016-02-15 07:34:06]

LGTM

[Yuta Kitamura, 2016-02-15 10:27:19]

The CQ bit was checked by yutak@chromium.org

[Yuta Kitamura, 2016-02-15 10:27:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/1550563002/#ps80001
(title: "Revert lambdas.")

[commit-bot: I haz the power, 2016-02-15 10:27:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550563002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550563002/80001

[commit-bot: I haz the power, 2016-02-15 10:30:29]

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

[commit-bot: I haz the power, 2016-02-15 10:30:30]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuta Kitamura, 2016-02-16 05:48:11]

The CQ bit was checked by yutak@chromium.org

[Yuta Kitamura, 2016-02-16 05:48:11]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
haraken@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1550563002/#ps100001
(title: "Merrrge agaaain.")

[commit-bot: I haz the power, 2016-02-16 05:48:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1550563002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1550563002/100001

[commit-bot: I haz the power, 2016-02-16 06:58:57]

Description was changed from

==========
Blink Platform: Remove time functions from Platform.

Simplification!

To support the use case of mocking timers for tests, CurrentTime.h now
provides a customization function. Currently, it overrides the three
functions at once, but practically that wouldn't be a problem.

Some unit tests try to override timing functions by replacing
Platform. However, CachingCorrectnessTest and TextFinderTest seemed to
fail to do so, as they apparently lacked Platform::initialize()
calls. I'm not sure why these tests have not been failing. Anyways, this
patch "fixes" these issues, too.

BUG=565765
==========

to

==========
Blink Platform: Remove time functions from Platform.

Simplification!

To support the use case of mocking timers for tests, CurrentTime.h now
provides a customization function. Currently, it overrides the three
functions at once, but practically that wouldn't be a problem.

Some unit tests try to override timing functions by replacing
Platform. However, CachingCorrectnessTest and TextFinderTest seemed to
fail to do so, as they apparently lacked Platform::initialize()
calls. I'm not sure why these tests have not been failing. Anyways, this
patch "fixes" these issues, too.

BUG=565765
==========

[commit-bot: I haz the power, 2016-02-16 06:58:58]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-02-16 22:51:35]

Description was changed from

==========
Blink Platform: Remove time functions from Platform.

Simplification!

To support the use case of mocking timers for tests, CurrentTime.h now
provides a customization function. Currently, it overrides the three
functions at once, but practically that wouldn't be a problem.

Some unit tests try to override timing functions by replacing
Platform. However, CachingCorrectnessTest and TextFinderTest seemed to
fail to do so, as they apparently lacked Platform::initialize()
calls. I'm not sure why these tests have not been failing. Anyways, this
patch "fixes" these issues, too.

BUG=565765
==========

to

==========
Blink Platform: Remove time functions from Platform.

Simplification!

To support the use case of mocking timers for tests, CurrentTime.h now
provides a customization function. Currently, it overrides the three
functions at once, but practically that wouldn't be a problem.

Some unit tests try to override timing functions by replacing
Platform. However, CachingCorrectnessTest and TextFinderTest seemed to
fail to do so, as they apparently lacked Platform::initialize()
calls. I'm not sure why these tests have not been failing. Anyways, this
patch "fixes" these issues, too.

BUG=565765

Committed: https://crrev.com/a5466f457a195abd0f03066439c0efae3b5e14b0
Cr-Commit-Position: refs/heads/master@{#375524}
==========

[commit-bot: I haz the power, 2016-02-16 22:51:36]

Patchset 6 (id:??) landed as
https://crrev.com/a5466f457a195abd0f03066439c0efae3b5e14b0
Cr-Commit-Position: refs/heads/master@{#375524}