cc: Adding BeginFrameTracker object and removing Now() from LTHI.

cc: Adding BeginFrameTracker object and removing Now() from LTHI.

**Sheriffs**: If this patch is making a test flaky / fail,
then the test was already broken and this just makes the
problem visible.

This patch reduces the jitter in LTHI's animation time to
be identical to the vsync jitter rather than being affected
by the system load and OS scheduling delays.

BeginFrameTracker uses DCHECKs and TRACE_EVENT to strictly
track how the BeginFrameArgs are used throughout the LTHI.
Using this information incorrect usage of Now() has been
eliminated. Methods which violate correct usage of
BeginFrameArgs are now clearly marked to prevent
regressions.

BUG=346230
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/0bdb49d47d685409d8f69dd0eb749a4f9e7bca6a
Cr-Commit-Position: refs/heads/master@{#331563}

[mithro-old, 2014-12-12 15:08:58]

mithro@mithis.com changed reviewers:
+ brianderson@chromium.org, skyostil@chromium.org, sunnyps@chromium.org

[mithro-old, 2014-12-12 15:08:59]

Hi everyone,

This is the base CL which I'm trying to land before end of next week and were
the other CLs originated from. Looking for good ideas on how to proceed here.
Would be worth starting an initial of review this patch too.

The primary problem is that the tracker currently fails because we send a
BeginMainFrame message ASAP when doing output surface creation. This means we
don't have a BeginFrameArgs yet to use.

One solution is to make sure we don't send the BeginMainFrame until we have
gotten the first BeginImplFrame. This breaks the following tests in
SchedulerStateMachine (and probably elsewhere). Trying to understand how the
tests pass currently is how I ended up doing the cleanup patches.
[  FAILED  ] SchedulerStateMachineTest.AbortBeginMainFrameAndCancelCommit
[  FAILED  ]
SchedulerStateMachineTest.AbortBeginMainFrameAndCancelCommitWhenInvisible
[  FAILED  ]
SchedulerStateMachineTest.AbortBeginMainFrameAndRequestCommitWhenInvisible
[  FAILED  ]
SchedulerStateMachineTest.AbortBeginMainFrameAndRequestCommitAndBeginImplFrameWhenInvisible
[  FAILED  ]
SchedulerStateMachineTest.TestContextLostWhenIdleAndCommitRequestedWhileRecreating

The second solution was to try and always send a MISSED BeginFrameArgs after we
become visible. This got blocked on  trying to correctly determine the deadline
to give the new MISSED frame. This is were
https://codereview.chromium.org/794293002/ comes from.

Tim 'mithro' Ansell

https://codereview.chromium.org/787763006/diff/1/cc/scheduler/scheduler_state...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/787763006/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:429: //  return true;
This is the problematic bit. We send a request off before we have any
BeginImplFrames.

[brianderson, 2014-12-13 01:31:54]

I'd say go with the first solution if you can and fix the tests without the re
factoring. Most of the tests are probably just overly strict and there isn't
going to be a good way to pick a deadline. The future sync point stuff is going
to require us to get a real BeginFrame before we draw anyway.

I also think we'll need a lot of discussion about the refactoring patch and
wouldn't want to block this patch on that one.

[brianderson, 2014-12-18 02:01:38]

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_tracker.cc (right):

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.cc:55: void BeginFrameTracker::Finish() {
Or is this important for debug / frame timing purposes? Is it important for
anything else? If it's just for debug purposes, is this where it belongs?

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.cc:68: return current_args_;
Would it make sense for this to always return the last finished frame args,
regardless of whether a new one started or not? Seems a bit weird to track
Started/Finished state to determine whether this method can be called.

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_tracker.h (right):

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.h:19: // Microclass to trace and check
properties for correct BeginFrameArgs usage.
Can you explain more about why this class exists? It looks like it helps
centralize some of the uninitialized interval logic. Does it help centralize
anything else?

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.h:21: class CC_EXPORT BeginFrameTracker {
Can you provide descriptions for each of the public methods in this class?

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:352: base::TimeDelta delay =
begin_impl_frame_args_.IsValid()
Why don't you need the check anymore?

https://codereview.chromium.org/787763006/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/787763006/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:3228: current_begin_frame_args_.Finish();
I forget why we need to reset the current args. Can we just keep them valid
until the next WillBeginImplFrame?

https://codereview.chromium.org/787763006/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/787763006/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:397:
layer_tree_host()->Composite(base::TimeTicks::FromInternalValue(2));
Are real timestamps needed to avoid hitting new DCHECKs?

https://codereview.chromium.org/787763006/diff/20001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/787763006/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:596:
layer_tree_host_impl_->ResetCurrentBeginFrameArgsForNextFrame();
Why does this need to move?

[mithro-old, 2014-12-18 17:22:41]

It took me a lot longer to get to this patch today then I had hoped :(

Still trying to chase down what is going on with a bunch of the tests (they are
hitting DCHECK failures).

Tim 'mithro' Ansell

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_tracker.cc (right):

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.cc:55: void BeginFrameTracker::Finish() {
On 2014/12/18 02:01:38, brianderson wrote:
> Or is this important for debug / frame timing purposes? Is it important for
> anything else? If it's just for debug purposes, is this where it belongs?

The start/finish methods help control access to the BFA object which prevent
accidentally using the wrong BFA value. For example, if the LTHI thinks it's
finished with a BFA then nobody else should be trying to access it as it could
be updated by a new BeginFrame message at any time.

It also gives a very strong sanity check that we are only starting / finishing
in one location.

It's modelled on the current Update/Reset system which I actually think was
pretty good until people started working around it.

It also allows us to track the BFA usage. For example, did we use end up using
BFA for 40ms? If so that means we must have missed 2 other BFA objects.

All this information appears in tracing when you enable the traces below too.

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.cc:68: return current_args_;
On 2014/12/18 02:01:38, brianderson wrote:
> Would it make sense for this to always return the last finished frame args,
> regardless of whether a new one started or not? Seems a bit weird to track
> Started/Finished state to determine whether this method can be called.

I want the caller to make a conscious decision about which BFA they want to use.
I don't want people just choose to use Last() because its easier and they are
feeling lazy.

It also provides a nice duality. When Get() is valid to call, Last() DCHECK
fails. When Last() is valid to call, Get() DCHECK fails.

Code which does the following;
 if (tracker.HasFinished())
  return tracker.Last();
 else
  return tracker.Get();

Should be treated with extreme suspicion because it is most certainly wrong in
some way. I want to make the HasFinished() method protected in the future but
can't do that right now.

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_tracker.h (right):

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.h:19: // Microclass to trace and check
properties for correct BeginFrameArgs usage.
On 2014/12/18 02:01:38, brianderson wrote:
> Can you explain more about why this class exists? It looks like it helps
> centralize some of the uninitialized interval logic. Does it help centralize
> anything else?

It centralises a bunch of things;
 * State about the BeginFrameArgs - If they are currently "in use" or node.

 * A large amount of sanity checks that both make sure that the incoming
BeginFrameArgs are sane and that the usage of them is sane. This is how I
discovered not having a valid BeginFrameArgs on the first commit problem.

 * A large amount of debugging code for helping trace the BeginFrameArgs through
the system. When you enable the tracing you get arrows which show how the
BeginFrameArgs flow between tasks. This is pretty awesome to see.

In the future, I can potentially see;

 * UMA tracking and other stats recording about the BeginFrameArgs objects.

 * More helper methods along the same vein as the Interval() one (not quite sure
what they would be yet).

 * Better auditing of BeginFrameArgs usage.

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.h:21: class CC_EXPORT BeginFrameTracker {
On 2014/12/18 02:01:38, brianderson wrote:
> Can you provide descriptions for each of the public methods in this class?

Done.

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (left):

https://codereview.chromium.org/787763006/diff/20001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:352: base::TimeDelta delay =
begin_impl_frame_args_.IsValid()
On 2014/12/18 02:01:38, brianderson wrote:
> Why don't you need the check anymore?

begin_impl_frame_args is now a BeginFrameTracker object which always gives us a
valid Interval() value.

https://codereview.chromium.org/787763006/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/787763006/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:3228: current_begin_frame_args_.Finish();
On 2014/12/18 02:01:38, brianderson wrote:
> I forget why we need to reset the current args. Can we just keep them valid
> until the next WillBeginImplFrame?

See my reasoning else were. I think the fact that even after reset we still
returned BeginFrameArgs has lead to some very bad code which the logic was
trying to prevent from existing in the first place.

https://codereview.chromium.org/787763006/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/787763006/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:397:
layer_tree_host()->Composite(base::TimeTicks::FromInternalValue(2));
On 2014/12/18 02:01:38, brianderson wrote:
> Are real timestamps needed to avoid hitting new DCHECKs?

Yeah, base::TimeTicks() is the "null equivalent" value. It should *never* be
seen by this method and should really already be DCHECKing on it.

https://codereview.chromium.org/787763006/diff/20001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (left):

https://codereview.chromium.org/787763006/diff/20001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:596:
layer_tree_host_impl_->ResetCurrentBeginFrameArgsForNextFrame();
On 2014/12/18 02:01:38, brianderson wrote:
> Why does this need to move?

Because it's currently in the wrong place as code in the STP between this
location and the new location still uses the BFA (as found by tests).

This means currently in production that code is not just getting values affected
by the delay to first BeginFrame, it's getting horrible values which are just
Now() when ever it's called.

[brianderson, 2014-12-18 21:57:08]

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_tracker.cc (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.cc:75: if (interval <
base::TimeDelta::FromMilliseconds(4)) {
250Hz seems like something we could hit eventually. How about error at 1kHz or
infinity?

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:397:
TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler.frames"), "BeginFrameArgs",
Is this a new category? If so, are we cluttering the tracing dialog box? Should
we keep it with cc.debug.scheduler?

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:648: begin_impl_frame_args_.Finish();
I think there are various places outside of the deadline where it should be
valid to look at the begin_impl_frame_args_.

AnticipatedDrawTime might get updated in the polling mechanisms multiple times
after the deadline.  Also, AsValueInto might get called after the deadline.

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:790: BeginFrameArgs args =
begin_impl_frame_args_.Get();
Can this be a reference?

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:208: BeginFrameTracker begin_impl_frame_args_;
args_ -> tracker_?

https://codereview.chromium.org/787763006/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/787763006/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:436: // TODO(mithro): Remove these methods which
expose control of the internal
Can you at least remove UpdateCurrentBeginFrameArgs?

https://codereview.chromium.org/787763006/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:535:
BeginFrameTracker(BEGINFRAMETRACKER_FROM_HERE);
According to http://chromium-cpp.appspot.com this isn't allowed yet.

[mithro-old, 2014-12-18 23:15:06]

Currently failing tests locally,

8 tests crashed:
   
LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor.RunMultiThread_DelegatingRenderer_ImplSidePaint
   
LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor.RunMultiThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor.RunMultiThread_DirectRenderer_ImplSidePaint
   
LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor.RunMultiThread_DirectRenderer_MainThreadPaint
   
LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled.RunMultiThread_DelegatingRenderer_ImplSidePaint
   
LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled.RunMultiThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled.RunMultiThread_DirectRenderer_ImplSidePaint
   
LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled.RunMultiThread_DirectRenderer_MainThreadPaint

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_tracker.cc (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.cc:75: if (interval <
base::TimeDelta::FromMilliseconds(4)) {
On 2014/12/18 21:57:08, brianderson wrote:
> 250Hz seems like something we could hit eventually. How about error at 1kHz or
> infinity?

I picked 250Hz because in theory 240Hz projectors exist (basically 3d ready
120Hz display). The 200Hz TV was the fastest I could find which I actually buy.
I'm pretty sure we'd collapse in a heap trying to do 250Hz (actually probably
even at 120Hz...).

Happy to change to 1kHz if you prefer.

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:397:
TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler.frames"), "BeginFrameArgs",
On 2014/12/18 21:57:08, brianderson wrote:
> Is this a new category? If so, are we cluttering the tracing dialog box?
Should
> we keep it with cc.debug.scheduler?

This category was added a while back after discussion with Sami showed the
traces in BeginFrameSource were adding about 10-15% overhead.

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:648: begin_impl_frame_args_.Finish();
On 2014/12/18 21:57:08, brianderson wrote:
> I think there are various places outside of the deadline where it should be
> valid to look at the begin_impl_frame_args_.

As discuss, maybe we should add something like DangerousGetCurrentOrLast()
method for this case.


> AnticipatedDrawTime might get updated in the polling mechanisms multiple times
> after the deadline. 

Yeah, I'm not sure what the correct solution there is. I have a work around
which seems to make the tests pass but the coverage here seems pretty poor.

> Also, AsValueInto might get called after the deadline.

I've already fixed AsValueInto in the latest patch.

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:790: BeginFrameArgs args =
begin_impl_frame_args_.Get();
On 2014/12/18 21:57:08, brianderson wrote:
> Can this be a reference?

Turns out this is location which needs the DangerousGetCurrentOrLast type
method.

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.h...
cc/scheduler/scheduler.h:208: BeginFrameTracker begin_impl_frame_args_;
On 2014/12/18 21:57:08, brianderson wrote:
> args_ -> tracker_?

Done. Sorry was being lazy.

https://codereview.chromium.org/787763006/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/787763006/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:436: // TODO(mithro): Remove these methods which
expose control of the internal
On 2014/12/18 21:57:08, brianderson wrote:
> Can you at least remove UpdateCurrentBeginFrameArgs?

Done.

https://codereview.chromium.org/787763006/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:535:
BeginFrameTracker(BEGINFRAMETRACKER_FROM_HERE);
On 2014/12/18 21:57:08, brianderson wrote:
> According to http://chromium-cpp.appspot.com this isn't allowed yet.

Done.

[sunnyps, 2014-12-18 23:23:00]

Fun fact: All of those tests have the use_external_begin_frame_source flag
set to true in the tests. The ExternalBeginFrameSourceForTest only sends
the first BeginFrame after SetNeedsBeginFrames(true) is called and doesn't
send any BeginFrames after that. I had to deal with this test for my
changes to WebView too. I made the ExternalBeginFrameSourceForTest behave
more like regular BeginFrameSources.

There's one more test which has this flag but that also disables vsync
throttling which means that it uses the BackToBackFrameSource instead of
the ExternalBeginFrameSourceForTest and does not exhibit this issue.

On Thu Dec 18 2014 at 3:15:06 PM <mithro@mithis.com> wrote:

> Currently failing tests locally,
>
> 8 tests crashed:
>
> LayerTreeHostTestAbortedCommitDoesntStallSynchronousComposit
> or.RunMultiThread_DelegatingRenderer_ImplSidePaint
>
> LayerTreeHostTestAbortedCommitDoesntStallSynchronousComposit
> or.RunMultiThread_DelegatingRenderer_MainThreadPaint
>
> LayerTreeHostTestAbortedCommitDoesntStallSynchronousComposit
> or.RunMultiThread_DirectRenderer_ImplSidePaint
>
> LayerTreeHostTestAbortedCommitDoesntStallSynchronousComposit
> or.RunMultiThread_DirectRenderer_MainThreadPaint
>
> LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled.
> RunMultiThread_DelegatingRenderer_ImplSidePaint
>
> LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled.
> RunMultiThread_DelegatingRenderer_MainThreadPaint
>
> LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled.
> RunMultiThread_DirectRenderer_ImplSidePaint
>
> LayerTreeHostTestBeginFrameNotificationShutdownWhileEnabled.
> RunMultiThread_DirectRenderer_MainThreadPaint
>
>
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> scheduler/begin_frame_tracker.cc
> File cc/scheduler/begin_frame_tracker.cc (right):
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> scheduler/begin_frame_tracker.cc#newcode75
> cc/scheduler/begin_frame_tracker.cc:75
>
<https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...>:
> if (interval <
> base::TimeDelta::FromMilliseconds(4)) {
> On 2014/12/18 21:57:08, brianderson wrote:
> > 250Hz seems like something we could hit eventually. How about error at
> 1kHz or
> > infinity?
>
> I picked 250Hz because in theory 240Hz projectors exist (basically 3d
> ready 120Hz display). The 200Hz TV was the fastest I could find which I
> actually buy. I'm pretty sure we'd collapse in a heap trying to do 250Hz
> (actually probably even at 120Hz...).
>
> Happy to change to 1kHz if you prefer.
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> scheduler/scheduler.cc#newcode397
> cc/scheduler/scheduler.cc:397
>
<https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...>
> :
> TRACE_DISABLED_BY_DEFAULT("cc.debug.scheduler.frames"),
> "BeginFrameArgs",
> On 2014/12/18 21:57:08, brianderson wrote:
> > Is this a new category? If so, are we cluttering the tracing dialog
> box? Should
> > we keep it with cc.debug.scheduler?
>
> This category was added a while back after discussion with Sami showed
> the traces in BeginFrameSource were adding about 10-15% overhead.
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> scheduler/scheduler.cc#newcode648
> cc/scheduler/scheduler.cc:648
>
<https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...>:
> begin_impl_frame_args_.Finish();
> On 2014/12/18 21:57:08, brianderson wrote:
> > I think there are various places outside of the deadline where it
> should be
> > valid to look at the begin_impl_frame_args_.
>
> As discuss, maybe we should add something like
> DangerousGetCurrentOrLast() method for this case.
>
>
> > AnticipatedDrawTime might get updated in the polling mechanisms
> multiple times
> > after the deadline.
>
> Yeah, I'm not sure what the correct solution there is. I have a work
> around which seems to make the tests pass but the coverage here seems
> pretty poor.
>
> > Also, AsValueInto might get called after the deadline.
>
> I've already fixed AsValueInto in the latest patch.
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> scheduler/scheduler.cc#newcode790
> cc/scheduler/scheduler.cc:790
>
<https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...>:
> BeginFrameArgs args =
> begin_impl_frame_args_.Get();
> On 2014/12/18 21:57:08, brianderson wrote:
> > Can this be a reference?
>
> Turns out this is location which needs the DangerousGetCurrentOrLast
> type method.
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> scheduler/scheduler.h
> File cc/scheduler/scheduler.h (right):
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> scheduler/scheduler.h#newcode208
> cc/scheduler/scheduler.h:208
>
<https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.h...>:
> BeginFrameTracker begin_impl_frame_args_;
> On 2014/12/18 21:57:08, brianderson wrote:
> > args_ -> tracker_?
>
> Done. Sorry was being lazy.
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> trees/layer_tree_host_impl.h
> File cc/trees/layer_tree_host_impl.h (right):
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> trees/layer_tree_host_impl.h#newcode436
> cc/trees/layer_tree_host_impl.h:436
>
<https://codereview.chromium.org/787763006/diff/60001/cc/trees/layer_tree_host...>:
> // TODO(mithro): Remove these
> methods which expose control of the internal
> On 2014/12/18 21:57:08, brianderson wrote:
> > Can you at least remove UpdateCurrentBeginFrameArgs?
>
> Done.
>
> https://codereview.chromium.org/787763006/diff/60001/cc/
> trees/layer_tree_host_impl.h#newcode535
> cc/trees/layer_tree_host_impl.h:535
>
<https://codereview.chromium.org/787763006/diff/60001/cc/trees/layer_tree_host...>
> :
> BeginFrameTracker(BEGINFRAMETRACKER_FROM_HERE);
> On 2014/12/18 21:57:08, brianderson wrote:
> > According to http://chromium-cpp.appspot.com this isn't allowed yet.
>
> Done.
>
> https://codereview.chromium.org/787763006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mithro-old, 2014-12-18 23:52:05]

On 2014/12/18 23:23:00, sunnyps wrote:
> Fun fact: All of those tests have the use_external_begin_frame_source flag
> set to true in the tests. The ExternalBeginFrameSourceForTest only sends
> the first BeginFrame after SetNeedsBeginFrames(true) is called and doesn't
> send any BeginFrames after that. I had to deal with this test for my
> changes to WebView too. I made the ExternalBeginFrameSourceForTest behave
> more like regular BeginFrameSources.
> 
> There's one more test which has this flag but that also disables vsync
> throttling which means that it uses the BackToBackFrameSource instead of
> the ExternalBeginFrameSourceForTest and does not exhibit this issue.

In https://codereview.chromium.org/798323003/ I fixed the
"cc/test/fake_external_begin_frame_source.h" to produce multiple frames to make
Simon Hong's sending to BeginFrames to children work.

It also seem that we now have *two* fake externalBFS implementations. I logged
the following bug about it;
https://code.google.com/p/chromium/issues/detail?id=443185 - I was pretty tired
when I did that (so hopefully it makes sense).

[brianderson, 2014-12-19 01:18:25]

lgtm once you can get the tests to pass.

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_tracker.cc (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.cc:75: if (interval <
base::TimeDelta::FromMilliseconds(4)) {
On 2014/12/18 23:15:05, mithro wrote:
> On 2014/12/18 21:57:08, brianderson wrote:
> > 250Hz seems like something we could hit eventually. How about error at 1kHz
or
> > infinity?
> 
> I picked 250Hz because in theory 240Hz projectors exist (basically 3d ready
> 120Hz display). The 200Hz TV was the fastest I could find which I actually
buy.
> I'm pretty sure we'd collapse in a heap trying to do 250Hz (actually probably
> even at 120Hz...).
> 
> Happy to change to 1kHz if you prefer.
> 
> 

1kHz seems less risky.

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:790: BeginFrameArgs args =
begin_impl_frame_args_.Get();
On 2014/12/18 23:15:05, mithro wrote:
> On 2014/12/18 21:57:08, brianderson wrote:
> > Can this be a reference?
> 
> Turns out this is location which needs the DangerousGetCurrentOrLast type
> method.

Yes, an "unsafe" or "dangerous" accessor would be nice.

https://codereview.chromium.org/787763006/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/787763006/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:299: begin_impl_frame_tracker_.HasFinished())
Just a note:

This might adversely affect the old texture upload path when the uploads take
more than a frame and we have impl-side updates. (uploads might monopolize more
of the impl thread than they would before since they think they think the impl
thread isn't busy). That path should be going away soon though, so that's okay.

At least it shouldn't stall the texture uploads.

[mithro-old, 2014-12-19 03:40:55]

Pretty much ready to land now. 

Just needs 
  * 792803008: cc: A commit waits for an ImplFrame it should cause
BeginFrameNeeded to become true.

to land first.

Tim

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
File cc/scheduler/begin_frame_tracker.cc (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
cc/scheduler/begin_frame_tracker.cc:75: if (interval <
base::TimeDelta::FromMilliseconds(4)) {
On 2014/12/19 01:18:25, brianderson wrote:
> On 2014/12/18 23:15:05, mithro wrote:
> > On 2014/12/18 21:57:08, brianderson wrote:
> > > 250Hz seems like something we could hit eventually. How about error at
1kHz
> or
> > > infinity?
> > 
> > I picked 250Hz because in theory 240Hz projectors exist (basically 3d ready
> > 120Hz display). The 200Hz TV was the fastest I could find which I actually
> buy.
> > I'm pretty sure we'd collapse in a heap trying to do 250Hz (actually
probably
> > even at 120Hz...).
> > 
> > Happy to change to 1kHz if you prefer.
> > 
> > 
> 
> 1kHz seems less risky.

Done.

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:790: BeginFrameArgs args =
begin_impl_frame_args_.Get();
On 2014/12/19 01:18:25, brianderson wrote:
> On 2014/12/18 23:15:05, mithro wrote:
> > On 2014/12/18 21:57:08, brianderson wrote:
> > > Can this be a reference?
> > 
> > Turns out this is location which needs the DangerousGetCurrentOrLast type
> > method.
> 
> Yes, an "unsafe" or "dangerous" accessor would be nice.

Done.

https://codereview.chromium.org/787763006/diff/80001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/787763006/diff/80001/cc/scheduler/scheduler.c...
cc/scheduler/scheduler.cc:299: begin_impl_frame_tracker_.HasFinished())
On 2014/12/19 01:18:25, brianderson wrote:
> Just a note:
> 
> This might adversely affect the old texture upload path when the uploads take
> more than a frame and we have impl-side updates. (uploads might monopolize
more
> of the impl thread than they would before since they think they think the impl
> thread isn't busy). That path should be going away soon though, so that's
okay.
> 
> At least it shouldn't stall the texture uploads.

Acknowledged.

[mithro-old, 2015-04-09 07:39:22]

On 2014/12/19 03:40:55, mithro wrote:
> Pretty much ready to land now. 
> 
> Just needs 
>   * 792803008: cc: A commit waits for an ImplFrame it should cause
> BeginFrameNeeded to become true.
> 
> to land first.
> 
> Tim
> 
>
https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
> File cc/scheduler/begin_frame_tracker.cc (right):
> 
>
https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/begin_frame...
> cc/scheduler/begin_frame_tracker.cc:75: if (interval <
> base::TimeDelta::FromMilliseconds(4)) {
> On 2014/12/19 01:18:25, brianderson wrote:
> > On 2014/12/18 23:15:05, mithro wrote:
> > > On 2014/12/18 21:57:08, brianderson wrote:
> > > > 250Hz seems like something we could hit eventually. How about error at
> 1kHz
> > or
> > > > infinity?
> > > 
> > > I picked 250Hz because in theory 240Hz projectors exist (basically 3d
ready
> > > 120Hz display). The 200Hz TV was the fastest I could find which I actually
> > buy.
> > > I'm pretty sure we'd collapse in a heap trying to do 250Hz (actually
> probably
> > > even at 120Hz...).
> > > 
> > > Happy to change to 1kHz if you prefer.
> > > 
> > > 
> > 
> > 1kHz seems less risky.
> 
> Done.
> 
> https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/787763006/diff/60001/cc/scheduler/scheduler.c...
> cc/scheduler/scheduler.cc:790: BeginFrameArgs args =
> begin_impl_frame_args_.Get();
> On 2014/12/19 01:18:25, brianderson wrote:
> > On 2014/12/18 23:15:05, mithro wrote:
> > > On 2014/12/18 21:57:08, brianderson wrote:
> > > > Can this be a reference?
> > > 
> > > Turns out this is location which needs the DangerousGetCurrentOrLast type
> > > method.
> > 
> > Yes, an "unsafe" or "dangerous" accessor would be nice.
> 
> Done.
> 
> https://codereview.chromium.org/787763006/diff/80001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/787763006/diff/80001/cc/scheduler/scheduler.c...
> cc/scheduler/scheduler.cc:299: begin_impl_frame_tracker_.HasFinished())
> On 2014/12/19 01:18:25, brianderson wrote:
> > Just a note:
> > 
> > This might adversely affect the old texture upload path when the uploads
take
> > more than a frame and we have impl-side updates. (uploads might monopolize
> more
> > of the impl thread than they would before since they think they think the
impl
> > thread isn't busy). That path should be going away soon though, so that's
> okay.
> > 
> > At least it shouldn't stall the texture uploads.
> 
> Acknowledged.



Finally getting back to this patch now. It seems that the latest version breaks
the following tests;
   
LayerTreeHostContextTestAvoidUnnecessaryComposite.RunSingleThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostContextTestAvoidUnnecessaryComposite.RunSingleThread_DirectRenderer_MainThreadPaint
    LayerTreeHostNoMessageLoopDelegatedLayer.SingleDelegatedLayer
    LayerTreeHostNoMessageLoopSmokeTest.SmokeTest
    LayerTreeHostSynchronousGPUPixelTest.OneContentLayer
    LayerTreeHostSynchronousPixelTest.OneContentLayer
    LayerTreeHostTest.PartialUpdatesWithDelegatingRendererAndGLContent
    LayerTreeHostTest.PartialUpdatesWithDelegatingRendererAndSoftwareContent
    LayerTreeHostTest.PartialUpdatesWithGLRenderer
    LayerTreeHostTest.PartialUpdatesWithSoftwareRenderer
   
LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor.RunMultiThread_DelegatingRenderer_ImplSidePaint
   
LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor.RunMultiThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor.RunMultiThread_DirectRenderer_ImplSidePaint
   
LayerTreeHostTestAbortedCommitDoesntStallSynchronousCompositor.RunMultiThread_DirectRenderer_MainThreadPaint
   
LayerTreeHostTestSynchronousCompositeSwapPromise.RunSingleThread_DelegatingRenderer_MainThreadPaint
   
LayerTreeHostTestSynchronousCompositeSwapPromise.RunSingleThread_DirectRenderer_MainThreadPaint
    PictureLayerTest.NonMonotonicSourceFrameNumber
    ScaledScrollbarLayerTestResourceCreation.ScaledResourceUpload
    ScaledScrollbarLayerTestScaledRasterization.TestLostPrecisionInClip
    ScrollbarLayerTest.LayerDrivenSolidColorDrawQuads
    ScrollbarLayerTest.ResolveScrollLayerPointer
    ScrollbarLayerTest.ResolveScrollLayerPointer_ReverseOrder
    ScrollbarLayerTest.ScrollOffsetSynchronization
    ScrollbarLayerTest.ShouldScrollNonOverlayOnMainThread
    ScrollbarLayerTest.SolidColorDrawQuads
    ScrollbarLayerTest.ThumbRect
    ScrollbarLayerTest.UpdatePropertiesOfScrollBarWhenThumbRemoved
    ScrollbarLayerTestResourceCreationAndRelease.ResourceUpload
    ScrollbarLayerTestResourceCreationAndRelease.SolidColorNoResourceUpload
    ScrollbarLayerTestResourceCreationAndRelease.TestResourceUpdate

Starting to investigate why.

[brianderson, 2015-05-01 02:05:21]

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_tracker.cc (right):

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_tracker.cc:38: current_updated_at_ =
base::TimeTicks::NowFromSystemTraceTime();
This might be in a different timebase than base::FrameTime::Now(). Is that okay?

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_tracker.cc:42: // TODO(mithro): Add UMA tracking of
delta between current_updated_at_ time
I think we'll want to track this as part of LatencyInfo, which we can use for
telemetry, UMA, and scheduling decisions.

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_tracker.h (right):

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_tracker.h:26: //  * The BFA is only used inside a given
period (
Incomplete comment?

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:806:
begin_impl_frame_tracker_.DangerousMethodCurrentOrLast();
Is there any way to make this not a dangerous call? I see that it's because of
the order BeginImplFramewithDeadline calls things in to be compatibvle with
BeginImplFrameSynchronous. Would callling begin_impl_frame_tracker_.Start(args)
outside BeginImplFrame help?

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:2592: base::TimeDelta initial_interval =
scheduler_->Interval();
scheduler_->Interval() has an ambiguous name. It's not clear if that the display
interval or the current frame interval.

https://codereview.chromium.org/787763006/diff/260001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/787763006/diff/260001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:394: //  << "Called animate with unknown frame
time!?";
Delete?

https://codereview.chromium.org/787763006/diff/260001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3110: // TODO(mithro): Replace call with
current_begin_frame_tracker_.Current()
For my education, what are the places that call this outside of an impl frame?

[mithro-old, 2015-05-01 03:27:56]

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_tracker.cc (right):

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_tracker.cc:38: current_updated_at_ =
base::TimeTicks::NowFromSystemTraceTime();
On 2015/05/01 02:05:21, brianderson wrote:
> This might be in a different timebase than base::FrameTime::Now(). Is that
okay?

At the moment current_updated_at_ and current_finished_at_ are never compared to
anything, they are just used to track the point in time these events occurred
for tracing information (hence why they are using the tracing time).

It might cause an issue if we try and to the TODO below, but lets worry about it
when that happens.

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_tracker.cc:42: // TODO(mithro): Add UMA tracking of
delta between current_updated_at_ time
On 2015/05/01 02:05:21, brianderson wrote:
> I think we'll want to track this as part of LatencyInfo, which we can use for
> telemetry, UMA, and scheduling decisions.

Lets figure that out after this patch lands.

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_tracker.h (right):

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_tracker.h:26: //  * The BFA is only used inside a given
period (
On 2015/05/01 02:05:21, brianderson wrote:
> Incomplete comment?

Done.

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/scheduler....
cc/scheduler/scheduler.cc:806:
begin_impl_frame_tracker_.DangerousMethodCurrentOrLast();
On 2015/05/01 02:05:21, brianderson wrote:
> Is there any way to make this not a dangerous call? I see that it's because of
> the order BeginImplFramewithDeadline calls things in to be compatibvle with
> BeginImplFrameSynchronous. Would callling
begin_impl_frame_tracker_.Start(args)
> outside BeginImplFrame help?

Yes, I have a number of ideas about how to do that, but don't want to make this
CL any bigger.

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/787763006/diff/260001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:2592: base::TimeDelta initial_interval =
scheduler_->Interval();
On 2015/05/01 02:05:21, brianderson wrote:
> scheduler_->Interval() has an ambiguous name. It's not clear if that the
display
> interval or the current frame interval.

Fixed.

https://codereview.chromium.org/787763006/diff/260001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/787763006/diff/260001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:394: //  << "Called animate with unknown frame
time!?";
On 2015/05/01 02:05:21, brianderson wrote:
> Delete?

Added a TODO to enable these checks when I track down why Animate is getting
called at random times.

https://codereview.chromium.org/787763006/diff/260001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:3110: // TODO(mithro): Replace call with
current_begin_frame_tracker_.Current()
On 2015/05/01 02:05:21, brianderson wrote:
> For my education, what are the places that call this outside of an impl frame?

There are a number of places mostly around cc/layers and unit tests. List of
files which have callees are;

cc/resources/tile_manager_unittest.cc
cc/layers/heads_up_display_layer_impl.cc
cc/layers/picture_layer_impl.cc
cc/layers/picture_layer_impl_unittest.cc
cc/test/fake_layer_tree_host_impl.cc
cc/test/fake_layer_tree_host_impl.h
cc/trees/layer_tree_impl.h
cc/trees/layer_tree_host_impl.cc
cc/trees/layer_tree_host_impl.h
cc/trees/layer_tree_impl.cc
cc/trees/layer_tree_host_unittest.cc
cc/trees/layer_tree_host_impl_unittest.cc
cc/trees/single_thread_proxy.cc
cc/trees/thread_proxy.cc

[mithro-old, 2015-05-05 01:15:32]

TODO for myself: Update the patch comment to be more comprehensive and include
instructions about what to do if this patch is causing failures.

[brianderson, 2015-05-05 20:45:22]

lgtm

[mithro-old, 2015-05-08 02:08:12]

The CQ bit was checked by mithro@mithis.com

[mithro-old, 2015-05-08 02:08:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/787763006/#ps320001
(title: "Small fixes.")

[commit-bot: I haz the power, 2015-05-08 02:08:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/787763006/320001

[commit-bot: I haz the power, 2015-05-08 03:13:22]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2015-05-08 03:14:28]

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

[Kunihiko Sakamoto, 2015-05-08 08:13:53]

A revert of this CL (patchset #17 id:320001) has been created in
https://codereview.chromium.org/1127313003/ by ksakamoto@chromium.org.

The reason for reverting is: May have caused Instrumentation test
AndroidWebViewTest failures.
This is a speculative revert; I will re-land if nothing gets fixed.

http://build.chromium.org/p/chromium.linux/builders/Android%20Tests%20%28dbg%...

failures:
org.chromium.android_webview.test.AndroidScrollIntegrationTest#testFlingScrollOnPopup
org.chromium.android_webview.test.PopupWindowTest#testOnPageFinishedCalledOnDomModificationAfterNavigation
org.chromium.android_webview.test.PopupWindowTest#testPopupWindow
org.chromium.android_webview.test.PostMessageTest#testPostMessageToIframeInsidePopup
.

[mithro-old, 2015-05-27 11:57:20]

The CQ bit was checked by mithro@mithis.com to run a CQ dry run

[mithro-old, 2015-05-27 11:57:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/787763006/#ps340001
(title: "Rebase onto master.")

[commit-bot: I haz the power, 2015-05-27 11:57:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/787763006/340001

[commit-bot: I haz the power, 2015-05-27 12:51:59]

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

[commit-bot: I haz the power, 2015-05-27 12:52:00]

Dry run: This issue passed the CQ dry run.

[mithro-old, 2015-05-27 13:04:28]

I've tried really hard to reproduce the failures identified by Kunihiko Sakamoto
but have not been able to reproduce them. (I've tried on debug and release
builds, I've tried on Nexus 4 and Nexus 6 devices, I've run it on all the
Chromium try bots with android in the name.)

I'm going to try and reland this patch now.

[mithro-old, 2015-05-27 13:04:38]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2015-05-27 13:04:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/787763006/340001

[commit-bot: I haz the power, 2015-05-27 13:08:11]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2015-05-27 13:09:14]

Patchset 18 (id:??) landed as
https://crrev.com/0bdb49d47d685409d8f69dd0eb749a4f9e7bca6a
Cr-Commit-Position: refs/heads/master@{#331563}