Add SwapPromise support to LayerTreeHost and LayerTreeImpl

Add SwapPromise support to LayerTreeHost and LayerTreeImpl

When a change to the compositor's state/invalidation/whatever happens, a                                                                                                                                
Swap Promise can be inserted into LayerTreeHost/LayerTreeImpl, to track                                                                                                                                 
whether the compositor's reply to the new state/invaliadtion/whatever is                                                                                                                                
completed in the compositor, i.e. the compositor knows it has been sent                                                                                                                                 
to its output or not.                                                                                                                                                                                   
                                                                                                                                                                                                         
If the new compositor state is sent to the output, SwapPromise::DidSwap()                                                                                                                               
will be called, and if the compositor fails to send its new state to the                                                                                                                                
output, e.g. commit fails on main thread, new frame data has no actual
damage so LayerTreeHostImpl::SwapBuffers() bails out early, then
Promise::DidNotSwap() will be called.

BUG=246034, 271583
TEST=unittests pass.

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

[enne (OOO), 2013-11-06 18:11:56]

How does this compare with LayerTreeHost::SetNextCommitForcesRedraw?

[Yufeng Shen (Slow to review), 2013-11-06 19:09:39]

On 2013/11/06 18:11:56, enne wrote:
> How does this compare with LayerTreeHost::SetNextCommitForcesRedraw?

I am not familiar with SetNextCommitForcesRedraw, but I assume the difference
is that SwapPromise is more like a tool to say that "oh, in this case I think
there might be a commit/swap buffer (e.g. SetNeedsCommit is set), but it might
not actually happen (no actual visible change), and lets do some logging/cleanup
or whatever is suitable to help debug why the commit/swap does not happen",
while
SetNextCommitForcesRedraw means the next commit/swap buffer must happen.

[Yufeng Shen (Slow to review), 2013-11-07 21:54:09]

On 2013/11/06 19:09:39, Yufeng Shen wrote:
> On 2013/11/06 18:11:56, enne wrote:
> > How does this compare with LayerTreeHost::SetNextCommitForcesRedraw?
> 
> I am not familiar with SetNextCommitForcesRedraw, but I assume the difference
> is that SwapPromise is more like a tool to say that "oh, in this case I think
> there might be a commit/swap buffer (e.g. SetNeedsCommit is set), but it might
> not actually happen (no actual visible change), and lets do some
logging/cleanup
> or whatever is suitable to help debug why the commit/swap does not happen",
> while
> SetNextCommitForcesRedraw means the next commit/swap buffer must happen.


James, mind giving a review for this?

[jamesr, 2013-11-07 22:03:13]

Can you specify the contract for this interface in the code?  The comment in the
patch description is nice, but it'll be hard to refer to later when modifying
this code and it ignores lots and lots of cases.  If I understand correctly the
intent here is that when a caller registers a swap promise, they're expecting a
swap to result as a direct causal relationship of the current state of the world
(i.e. someone's requested a frame).  What happens if someone registers a swap
promise when the compositor has no reason to believe it needs to make a frame?  

The description mentions an aborted commit, but what about other situations that
cause the compositor to become quiescent like losing a context or becoming
invisible?

[Yufeng Shen (Slow to review), 2013-11-07 22:31:25]

On 2013/11/07 22:03:13, jamesr wrote:
> Can you specify the contract for this interface in the code?  The comment in
the
> patch description is nice, but it'll be hard to refer to later when modifying
> this code and it ignores lots and lots of cases.  If I understand correctly
the
> intent here is that when a caller registers a swap promise, they're expecting
a
> swap to result as a direct causal relationship of the current state of the
world
> (i.e. someone's requested a frame).  What happens if someone registers a swap
> promise when the compositor has no reason to believe it needs to make a frame?
 
> 

Add comments to QueueSwapPromise() to indicate that one should call this
function
when he is expecting there to be a swap buffer.

> The description mentions an aborted commit, but what about other situations
that
> cause the compositor to become quiescent like losing a context or becoming
> invisible?

Aborted commit covers the case that the compositor becomes invisible. There is
an
early out (commit aborted) in ThreadProxy::BeginMainFrame() due to NotVisible,
in
which case the SwapPromise is considered to be broken.

Not quite sure about losing context. Can you point me to the relevant code path
?

[jamesr, 2013-11-12 03:15:04]

I'm having trouble imagining how client code could use this, especially given
the somewhat confusing threading here.  Given that this patch only exposes
something on cc::LayerTreeHost, do you expect to only use this from the main
thread (and thus not, for example, from the compositor thread's input filter)? 
Or is that intended for a future patch?  I'm not sure how the unit test code is
threadsafe since it looks like the promise counts could be modified from
different threads without any synchronization, as far as I can tell.

https://codereview.chromium.org/60513007/diff/70001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/60513007/diff/70001/cc/base/swap_promise.h#ne...
cc/base/swap_promise.h:24: base::Callback<void(const std::string&)>
swap_aborted_callback;
why does this take a std::string? from this patch it appears that only a small
set of strings could possibly be passed here, which would indicate that an enum
would be a better fit

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:45: const size_t kMaxQueuedSwapPromiseSize = 100;
where does this number come from?

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1301: NOTREACHED() <<
"LayerTreeHost::swap_promise_list overflows";
i don't understand this. why have this limit?

does it ever make sense to have more than one swap promise on the main thread?

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:334: // did_swap_callback in the SwapPromise to be
called when swap buffer happens,
what thread will these callbacks be invoked on?

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:335: // and a swap_aborted_callback to be called when
the swap buffer is aborted (
what about when the context is lost? see
LayerTreeHostClient::DidInitializeOutputSurface()

https://codereview.chromium.org/60513007/diff/70001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/60513007/diff/70001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:756: SwapPromiseChecker
swap_promise_checker(layer_tree_host_);
it looks like this will invoke the aborted callback on the main thread

https://codereview.chromium.org/60513007/diff/70001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1140:
layer_tree_host_impl_->active_tree()->BreakSwapPromise("DID NOT SWAP");
whereas these calls will invoke callbacks from the compositor thread. is the
caller supposed to deal with this themselves? that seems like an extremely
error-prone interface if the callback is run and bound arguments destroyed from
different threads depending on what happens

what if we never make it to this point? for example, what if we lose the
OutputSurface in between the commit and the draw, or become invisible, or any
other case where the scheduler decides not to advance to drawing?

[Yufeng Shen (Slow to review), 2013-11-13 21:04:52]

On 2013/11/12 03:15:04, jamesr wrote:
> I'm having trouble imagining how client code could use this, especially given
> the somewhat confusing threading here.

Right. I have changed the interface of SwapPromise in the new patch set and
put comments there to encourage the clients to let the SwapPromise subclass
carry thread-safe member data so that DidSwap() and DidNotSwap() can operate
on the member data safely no matter from which thread the calls are invoked.

> Given that this patch only exposes
> something on cc::LayerTreeHost, do you expect to only use this from the main
> thread (and thus not, for example, from the compositor thread's input filter)?

> Or is that intended for a future patch? 

There is also an interface  LayerTreeImpl::QueueSwapPromise.

> I'm not sure how the unit test code is
> threadsafe since it looks like the promise counts could be modified from
> different threads without any synchronization, as far as I can tell.
> 

Yes. I reworked the unittests to follow the guidance in the comments of
swap_promise.h. So it should be thread-safe now.


> https://codereview.chromium.org/60513007/diff/70001/cc/base/swap_promise.h
> File cc/base/swap_promise.h (right):
> 
>
https://codereview.chromium.org/60513007/diff/70001/cc/base/swap_promise.h#ne...
> cc/base/swap_promise.h:24: base::Callback<void(const std::string&)>
> swap_aborted_callback;
> why does this take a std::string? from this patch it appears that only a small
> set of strings could possibly be passed here, which would indicate that an
enum
> would be a better fit
> 
>
https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host.cc
> File cc/trees/layer_tree_host.cc (right):
> 
>
https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
> cc/trees/layer_tree_host.cc:45: const size_t kMaxQueuedSwapPromiseSize = 100;
> where does this number come from?
> 
>
https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
> cc/trees/layer_tree_host.cc:1301: NOTREACHED() <<
> "LayerTreeHost::swap_promise_list overflows";
> i don't understand this. why have this limit?
> 
> does it ever make sense to have more than one swap promise on the main thread?
> 
> https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host.h
> File cc/trees/layer_tree_host.h (right):
> 
>
https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
> cc/trees/layer_tree_host.h:334: // did_swap_callback in the SwapPromise to be
> called when swap buffer happens,
> what thread will these callbacks be invoked on?
> 
>
https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
> cc/trees/layer_tree_host.h:335: // and a swap_aborted_callback to be called
when
> the swap buffer is aborted (
> what about when the context is lost? see
> LayerTreeHostClient::DidInitializeOutputSurface()
> 
> https://codereview.chromium.org/60513007/diff/70001/cc/trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/60513007/diff/70001/cc/trees/thread_proxy.cc#...
> cc/trees/thread_proxy.cc:756: SwapPromiseChecker
> swap_promise_checker(layer_tree_host_);
> it looks like this will invoke the aborted callback on the main thread
> 
>
https://codereview.chromium.org/60513007/diff/70001/cc/trees/thread_proxy.cc#...
> cc/trees/thread_proxy.cc:1140:
> layer_tree_host_impl_->active_tree()->BreakSwapPromise("DID NOT SWAP");
> whereas these calls will invoke callbacks from the compositor thread. is the
> caller supposed to deal with this themselves? that seems like an extremely
> error-prone interface if the callback is run and bound arguments destroyed
from
> different threads depending on what happens
> 
> what if we never make it to this point? for example, what if we lose the
> OutputSurface in between the commit and the draw, or become invisible, or any
> other case where the scheduler decides not to advance to drawing?

[Yufeng Shen (Slow to review), 2013-11-13 21:05:04]

https://codereview.chromium.org/60513007/diff/70001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/60513007/diff/70001/cc/base/swap_promise.h#ne...
cc/base/swap_promise.h:24: base::Callback<void(const std::string&)>
swap_aborted_callback;
On 2013/11/12 03:15:04, jamesr wrote:
> why does this take a std::string? from this patch it appears that only a small
> set of strings could possibly be passed here, which would indicate that an
enum
> would be a better fit

Done.

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:45: const size_t kMaxQueuedSwapPromiseSize = 100;
On 2013/11/12 03:15:04, jamesr wrote:
> where does this number come from?

see comments below.

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.cc:1301: NOTREACHED() <<
"LayerTreeHost::swap_promise_list overflows";
On 2013/11/12 03:15:04, jamesr wrote:
> i don't understand this. why have this limit?
> 
> does it ever make sense to have more than one swap promise on the main thread?

If SwapPromise is supposed to be used by different clients, then
there would be multiple clients inserting SwapPromise into the queue with
different behaviors of DidSwap() and DidNotSwap().

There is concern that in some rare case we could keep increasing the queue
unbounded without clearing them thus causing memory stress. So the limit is just
a guard against that.

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:334: // did_swap_callback in the SwapPromise to be
called when swap buffer happens,
On 2013/11/12 03:15:04, jamesr wrote:
> what thread will these callbacks be invoked on?

Please see the comments in thread_proxy.cc

https://codereview.chromium.org/60513007/diff/70001/cc/trees/layer_tree_host....
cc/trees/layer_tree_host.h:335: // and a swap_aborted_callback to be called when
the swap buffer is aborted (
On 2013/11/12 03:15:04, jamesr wrote:
> what about when the context is lost? see
> LayerTreeHostClient::DidInitializeOutputSurface()

We started an email thread "Swap Promises" discussing this.

https://codereview.chromium.org/60513007/diff/70001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/60513007/diff/70001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:756: SwapPromiseChecker
swap_promise_checker(layer_tree_host_);
On 2013/11/12 03:15:04, jamesr wrote:
> it looks like this will invoke the aborted callback on the main thread

See comments below.

https://codereview.chromium.org/60513007/diff/70001/cc/trees/thread_proxy.cc#...
cc/trees/thread_proxy.cc:1140:
layer_tree_host_impl_->active_tree()->BreakSwapPromise("DID NOT SWAP");
On 2013/11/12 03:15:04, jamesr wrote:
> whereas these calls will invoke callbacks from the compositor thread. is the
> caller supposed to deal with this themselves? that seems like an extremely
> error-prone interface if the callback is run and bound arguments destroyed
from
> different threads depending on what happens

Right.  I have in the new patch set changed the interface of
SwapPromise so that SwapPromise is now an abstract class that
has virtual interface  DidSwap() and DidNotSwap() which can
be called accordingly when swap buffer happens and fails.

I also put comments/warning that DidNotSwap() can happen
on main/impl thread. So it is better to have the subclass of
SwapPromise to carry the thread-safe member data (e.g. in
LatencyInfo case, copy the LatencyInfo into the a subclass
InputLatencySwapPromise), and so that when DidSwap/DidNotSwap()
are operating on the member data, it does not matter which thread
it is on.

> 
> what if we never make it to this point? for example, what if we lose the
> OutputSurface in between the commit and the draw, or become invisible, or any
> other case where the scheduler decides not to advance to drawing?

An email thread "Swap Promises" was started for discussing this.

[danakj, 2013-11-13 23:22:07]

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:12: // A SwapPromise can be queued into LTH/LTI to track
if an expected swap
spell out LayerTreeHost(Impl) here

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:13: // buffer will happen or not. If the swap buffer
happens, DidSwap() will
Can you reword this comment to match what I wrote in email about this? It's not
tracking a swap buffers so much as tracking that the current state of the tree
has been sent to the compositor's output.

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:38: SwapPromiseType type() {
If there's only one type, what's the point of this?

https://codereview.chromium.org/60513007/diff/140001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/60513007/diff/140001/cc/cc.gyp#newcode67
cc/cc.gyp:67: 'base/swap_promise.cc',
I don't see this file in the diff

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:390:
sync_tree->TakeSwapPromise(&swap_promise_list_);
This sounds like we're taking the swap promises from the sync tree.. can this be
PassSwapPromises()?

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1276: NOTREACHED() <<
"LayerTreeHost::swap_promise_list overflows";
If you expect this to be hit validly, then don't NOTREACHED(), and just handle
it as you have.

If you expect this to not be hit validly, then NOTREACHED() and you don't need
to add infrastructure to deal with it.

ie. pick one please

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1286: void LayerTreeHost::FinishSwapPromise() {
FinishSwapPromises?

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1293: void
LayerTreeHost::BreakSwapPromise(SwapPromise::DidNotSwapReason reason) {
BreakSwapPromises?

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:293: // LTH will take the ownership of
|swap_promise|.
Spell out LayerTreeHost.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:294: void QueueSwapPromise(SwapPromise*
swap_promise);
If ownership passes, why is this a raw pointer and not a scoped_ptr?

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:447: ScopedVector<SwapPromise> swap_promise_list_;
ScopedPtrVector?

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5260: memset(&swap_promise_result_, 0,
sizeof(swap_promise_result_));
Make a constructor on SwapPromiseResult instead of needing to memset() elsewhere

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5264:
settings->begin_impl_frame_scheduling_enabled = true;
Why is this needed?

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5275: commit_count_++;
You can use layer_tree_host()->source_frame_number() instead of tracking that
again here as commit_count_

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5285: // This commit will be aborted.
I think that a better way to make an aborted commit would be to SetNeedsUpdate()
on the layer tree host with no invalidations on the layers.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5297: // The first commit completes and
causes swap buffer wihch finishes
which

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:24: const size_t kMaxQueuedSwapPromiseNumber = 100;
Can we share these constants instead of duplicating? They need to be the same
size.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:718: void
LayerTreeImpl::QueueSwapPromise(SwapPromise* swap_promise) {
Same comments here as in LTH

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.h:285: ScopedVector<SwapPromise> swap_promise_list_;
ScopedPtrVector?

https://codereview.chromium.org/60513007/diff/140001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:397:
layer_tree_host_impl_->active_tree()->FinishSwapPromise();
Why doesn't LTHI do this directly inside SwapBuffers?

https://codereview.chromium.org/60513007/diff/140001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:752: // If the commit finishes, LTH will transfer its
swap promises to LTI.
Spell out LayerTreeHost and LayerTreeImpl

https://codereview.chromium.org/60513007/diff/140001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:753: // The dtor of SwapPromiseChecker will check LTH's
swap promises, and
destructor

check the LayerTreeHost's swap promises

[danakj, 2013-11-13 23:23:35]

https://codereview.chromium.org/60513007/diff/140001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:44: if (layer_tree_host_->HasQueuedSwapPromise())
Do you even need this? You could just always call Break and it just does nothing
if its empty?

[danakj, 2013-11-13 23:25:22]

Since the SwapPromise can live on either thread, would it make sense to have the
SwapPromise itself PostTask internally when things happen to the thread it was
created on, then call DidSwap() or DidNotSwap() on that correct thread, so the
embedder doesn't have to think about these things?

[Yufeng Shen (Slow to review), 2013-11-14 22:09:54]

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:12: // A SwapPromise can be queued into LTH/LTI to track
if an expected swap
On 2013/11/13 23:22:09, danakj wrote:
> spell out LayerTreeHost(Impl) here

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:13: // buffer will happen or not. If the swap buffer
happens, DidSwap() will
On 2013/11/13 23:22:09, danakj wrote:
> Can you reword this comment to match what I wrote in email about this? It's
not
> tracking a swap buffers so much as tracking that the current state of the tree
> has been sent to the compositor's output.

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:38: SwapPromiseType type() {
On 2013/11/13 23:22:09, danakj wrote:
> If there's only one type, what's the point of this?

It is intended that subclass added in the future will add
more types here.

https://codereview.chromium.org/60513007/diff/140001/cc/cc.gyp
File cc/cc.gyp (right):

https://codereview.chromium.org/60513007/diff/140001/cc/cc.gyp#newcode67
cc/cc.gyp:67: 'base/swap_promise.cc',
On 2013/11/13 23:22:09, danakj wrote:
> I don't see this file in the diff

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:390:
sync_tree->TakeSwapPromise(&swap_promise_list_);
On 2013/11/13 23:22:09, danakj wrote:
> This sounds like we're taking the swap promises from the sync tree.. can this
be
> PassSwapPromises()?

hmmm, "take" reads more correctly to me than "pass", and the 
implementation of TakeSwapPromise(other) is 
using ScopedPtrVector::insert_and_take(other)
swap_promise_list_.insert_and_take(..., other) 
which also uses "take".

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1276: NOTREACHED() <<
"LayerTreeHost::swap_promise_list overflows";
On 2013/11/13 23:22:09, danakj wrote:
> If you expect this to be hit validly, then don't NOTREACHED(), and just handle
> it as you have.
> 
> If you expect this to not be hit validly, then NOTREACHED() and you don't need
> to add infrastructure to deal with it.
> 
> ie. pick one please

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1286: void LayerTreeHost::FinishSwapPromise() {
On 2013/11/13 23:22:09, danakj wrote:
> FinishSwapPromises?

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1293: void
LayerTreeHost::BreakSwapPromise(SwapPromise::DidNotSwapReason reason) {
On 2013/11/13 23:22:09, danakj wrote:
> BreakSwapPromises?

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:293: // LTH will take the ownership of
|swap_promise|.
On 2013/11/13 23:22:09, danakj wrote:
> Spell out LayerTreeHost.
> 

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:294: void QueueSwapPromise(SwapPromise*
swap_promise);
On 2013/11/13 23:22:09, danakj wrote:
> If ownership passes, why is this a raw pointer and not a scoped_ptr?

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:447: ScopedVector<SwapPromise> swap_promise_list_;
On 2013/11/13 23:22:09, danakj wrote:
> ScopedPtrVector?

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5260: memset(&swap_promise_result_, 0,
sizeof(swap_promise_result_));
On 2013/11/13 23:22:09, danakj wrote:
> Make a constructor on SwapPromiseResult instead of needing to memset()
elsewhere

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5264:
settings->begin_impl_frame_scheduling_enabled = true;
On 2013/11/13 23:22:09, danakj wrote:
> Why is this needed?

removed.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5275: commit_count_++;
On 2013/11/13 23:22:09, danakj wrote:
> You can use layer_tree_host()->source_frame_number() instead of tracking that
> again here as commit_count_

I think here what we want is that after we have tried twice BeginMainFrame, we
want schedule a third commit that will finish.

Notice that even though we aborted the second commit, DidCommit()
was still called, so we have the right number commit_count_ = 2,
while source frame number remains 1 after the second commit aborts,
so we can't tell from it how many commits we have tried.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5285: // This commit will be aborted.
On 2013/11/13 23:22:09, danakj wrote:
> I think that a better way to make an aborted commit would be to
SetNeedsUpdate()
> on the layer tree host with no invalidations on the layers.

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:5297: // The first commit completes and
causes swap buffer wihch finishes
On 2013/11/13 23:22:09, danakj wrote:
> which

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:24: const size_t kMaxQueuedSwapPromiseNumber = 100;
On 2013/11/13 23:22:09, danakj wrote:
> Can we share these constants instead of duplicating? They need to be the same
> size.

Moved to swap_promise.h

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:718: void
LayerTreeImpl::QueueSwapPromise(SwapPromise* swap_promise) {
On 2013/11/13 23:22:09, danakj wrote:
> Same comments here as in LTH

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl.h
File cc/trees/layer_tree_impl.h (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.h:285: ScopedVector<SwapPromise> swap_promise_list_;
On 2013/11/13 23:22:09, danakj wrote:
> ScopedPtrVector?

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/single_thread_p...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/single_thread_p...
cc/trees/single_thread_proxy.cc:397:
layer_tree_host_impl_->active_tree()->FinishSwapPromise();
On 2013/11/13 23:22:09, danakj wrote:
> Why doesn't LTHI do this directly inside SwapBuffers?

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:44: if (layer_tree_host_->HasQueuedSwapPromise())
On 2013/11/13 23:23:35, danakj wrote:
> Do you even need this? You could just always call Break and it just does
nothing
> if its empty?

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:752: // If the commit finishes, LTH will transfer its
swap promises to LTI.
On 2013/11/13 23:22:09, danakj wrote:
> Spell out LayerTreeHost and LayerTreeImpl

Done.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:753: // The dtor of SwapPromiseChecker will check LTH's
swap promises, and
On 2013/11/13 23:22:09, danakj wrote:
> destructor
> 
> check the LayerTreeHost's swap promises

Done.

https://codereview.chromium.org/60513007/diff/310001/cc/base/swap_promise.cc
File cc/base/swap_promise.cc (right):

https://codereview.chromium.org/60513007/diff/310001/cc/base/swap_promise.cc#...
cc/base/swap_promise.cc:6: 
Please ignore that this file is replacing ui/gfx/render_text_ozone.cc.

Locally I only create the swap_promise.cc but when I git upload, it appears this
file is replacing mysteriously
ui/gfx/render_text_ozone.cc.
I will figure this out.

[Yufeng Shen (Slow to review), 2013-11-14 22:27:41]

On 2013/11/13 23:25:22, danakj wrote:
> Since the SwapPromise can live on either thread, would it make sense to have
the
> SwapPromise itself PostTask internally when things happen to the thread it was
> created on, then call DidSwap() or DidNotSwap() on that correct thread, so the
> embedder doesn't have to think about these things?

So I am not sure if it is worth adding the complication. Considering currently
the
main planned usage would be for LatencyInfo tracking, something like

class InputLatencySwapPromise : public SwapPromise {
public:
  InputLatencySwapPromise(const LatencyInfo& latency) : latency_(latency) {}

  void DidSwap() {}
  void DidNotSwap { 
    // end the latency tracking for |latency_|.
    // it does not matter this is invoked on main/impl thread.
  }  

private:
  LatencyInfo latency_;
};

Advocating subclass of SwapPromise to carry and operate on thread-safe memeber
data seems good enough for this use case.

[danakj, 2013-11-20 03:00:55]

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:38: SwapPromiseType type() {
On 2013/11/14 22:09:55, Yufeng Shen wrote:
> On 2013/11/13 23:22:09, danakj wrote:
> > If there's only one type, what's the point of this?
> 
> It is intended that subclass added in the future will add
> more types here.

Then I prefer we add it in a later patch that will use it instead. That will
give appropriate context and no point in adding it here if it's not needed to
implement the SwapPromise system.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:390:
sync_tree->TakeSwapPromise(&swap_promise_list_);
On 2013/11/14 22:09:55, Yufeng Shen wrote:
> On 2013/11/13 23:22:09, danakj wrote:
> > This sounds like we're taking the swap promises from the sync tree.. can
this
> be
> > PassSwapPromises()?
> 
> hmmm, "take" reads more correctly to me than "pass", and the 
> implementation of TakeSwapPromise(other) is 
> using ScopedPtrVector::insert_and_take(other)
> swap_promise_list_.insert_and_take(..., other) 
> which also uses "take".

I think insert_and_take() says "insert these things and call Take() on them to
take them from me". It's delegating the Take() responsibility rather than doing
a Take() action itself.

Whereas ScopedPtrVector::take() takes something out of the set.
Also LayerImpl::TakeCopyRequests..() takes the copy requests and returns them to
the caller.
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...

So I think the pattern is Take() means give me something back generally. And
Pass() means to give my thing to someone else.

[Yufeng Shen (Slow to review), 2013-11-20 19:15:40]

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h#n...
cc/base/swap_promise.h:38: SwapPromiseType type() {
On 2013/11/20 03:00:55, danakj wrote:
> On 2013/11/14 22:09:55, Yufeng Shen wrote:
> > On 2013/11/13 23:22:09, danakj wrote:
> > > If there's only one type, what's the point of this?
> > 
> > It is intended that subclass added in the future will add
> > more types here.
> 
> Then I prefer we add it in a later patch that will use it instead. That will
> give appropriate context and no point in adding it here if it's not needed to
> implement the SwapPromise system.

removed.

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:390:
sync_tree->TakeSwapPromise(&swap_promise_list_);
On 2013/11/20 03:00:55, danakj wrote:
> On 2013/11/14 22:09:55, Yufeng Shen wrote:
> > On 2013/11/13 23:22:09, danakj wrote:
> > > This sounds like we're taking the swap promises from the sync tree.. can
> this
> > be
> > > PassSwapPromises()?
> > 
> > hmmm, "take" reads more correctly to me than "pass", and the 
> > implementation of TakeSwapPromise(other) is 
> > using ScopedPtrVector::insert_and_take(other)
> > swap_promise_list_.insert_and_take(..., other) 
> > which also uses "take".
> 
> I think insert_and_take() says "insert these things and call Take() on them to
> take them from me". It's delegating the Take() responsibility rather than
doing
> a Take() action itself.
> 
> Whereas ScopedPtrVector::take() takes something out of the set.
> Also LayerImpl::TakeCopyRequests..() takes the copy requests and returns them
to
> the caller.
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...
> 
> So I think the pattern is Take() means give me something back generally. And
> Pass() means to give my thing to someone else.
> 

Done.

[danakj1, 2013-11-22 08:33:07]

In LTH.h and LTImpl.h, I think mentioning that LTH/I takes ownership of the
SwapPromise is kinda redundant since we're using a scoped_ptr (yay) so
remove that piece of the comments?

The TestSwapPromise should use a Lock to access the result. The AfterTest
method should use the same lock.

// Take the |new_swap_promise| and appended it to |swap_promise_list_|.
^should be "and append it to"

What calls FinishSwapPromises on LayerTreeHost? Is that dead code we can
remove? I'm not sure what it would mean to have a promise finish while
still on the main thread side.

I think this looks good once these are resolved.

https://codereview.chromium.org/60513007/diff/140001/cc/base/swap_promise.h
File cc/base/swap_promise.h (right):

https://codereview.chromium.org/60513007/diff/140001/cc/
base/swap_promise.h#newcode38
cc/base/swap_promise.h:38: SwapPromiseType type() {
On 2013/11/20 03:00:55, danakj wrote:

> On 2013/11/14 22:09:55, Yufeng Shen wrote:
> > On 2013/11/13 23:22:09, danakj wrote:
> > > If there's only one type, what's the point of this?
> >
> > It is intended that subclass added in the future will add
> > more types here.
>

 Then I prefer we add it in a later patch that will use it instead.
>
That will

> give appropriate context and no point in adding it here if it's not
>
needed to

> implement the SwapPromise system.
>

removed.

https://codereview.chromium.org/60513007/diff/140001/cc/
trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/140001/cc/
trees/layer_tree_host.cc#newcode390
cc/trees/layer_tree_host.cc:390:
sync_tree->TakeSwapPromise(&swap_promise_list_);
On 2013/11/20 03:00:55, danakj wrote:

> On 2013/11/14 22:09:55, Yufeng Shen wrote:
> > On 2013/11/13 23:22:09, danakj wrote:
> > > This sounds like we're taking the swap promises from the sync
>
tree.. can

> this
> > be
> > > PassSwapPromises()?
> >
> > hmmm, "take" reads more correctly to me than "pass", and the
> > implementation of TakeSwapPromise(other) is
> > using ScopedPtrVector::insert_and_take(other)
> > swap_promise_list_.insert_and_take(..., other)
> > which also uses "take".
>

 I think insert_and_take() says "insert these things and call Take() on
>
them to

> take them from me". It's delegating the Take() responsibility rather
>
than doing

> a Take() action itself.
>

 Whereas ScopedPtrVector::take() takes something out of the set.
> Also LayerImpl::TakeCopyRequests..() takes the copy requests and
>
returns them to

> the caller.
>

https://code.google.com/p/chromium/codesearch#chromium/
src/cc/layers/layer_impl.cc&rcl=1384866048&l=205

 So I think the pattern is Take() means give me something back
>
generally. And

> Pass() means to give my thing to someone else.
>


Done.

https://codereview.chromium.org/60513007/

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

[Yufeng Shen (Slow to review), 2013-11-22 20:40:19]

On 2013/11/22 08:33:07, danakj1 wrote:
> In LTH.h and LTImpl.h, I think mentioning that LTH/I takes ownership of the
> SwapPromise is kinda redundant since we're using a scoped_ptr (yay) so
> remove that piece of the comments?
> 

removed.

> The TestSwapPromise should use a Lock to access the result. The AfterTest
> method should use the same lock.
> 

done.

> // Take the |new_swap_promise| and appended it to |swap_promise_list_|.
> ^should be "and append it to"
> 

done.

> What calls FinishSwapPromises on LayerTreeHost? Is that dead code we can
> remove? I'm not sure what it would mean to have a promise finish while
> still on the main thread side.

removed.

> 
> I think this looks good once these are resolved.
>

[danakj, 2013-11-23 10:22:06]

https://codereview.chromium.org/60513007/diff/870001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/870001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1284: if (swap_promise_list_[i])
I'm confused by this change. Why is there a NULL SwapPromise ever? Can we
DCHECK(!NULL) when we QueueSwapPromise?

[Yufeng Shen (Slow to review), 2013-11-23 17:21:35]

https://codereview.chromium.org/60513007/diff/870001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/870001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1284: if (swap_promise_list_[i])
On 2013/11/23 10:22:07, danakj wrote:
> I'm confused by this change. Why is there a NULL SwapPromise ever? Can we
> DCHECK(!NULL) when we QueueSwapPromise?

I must be thinking of guarding against future change that might
accidentally reset swap_promise_list[i] to NULL, e.g. call 
swap_promise_list_.take() somehow. Change this to a 
DCHECK(!NULL) at places where we dereference swap_promise_list_[i]
in the new patch set.

[danakj, 2013-11-26 16:24:37]

https://codereview.chromium.org/60513007/diff/1130001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/1130001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1284: DCHECK(swap_promise_list_[i]);
Can you DCHECK this when they are added (QueueSwapPromise) instead of here? That
will find anything adding a NULL swap promise to the list faster, and give a
better stack trace. DCHECK before a null deref doesn't add too much value.

https://codereview.chromium.org/60513007/diff/1130001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/60513007/diff/1130001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:729: DCHECK(swap_promise_list_[i]);
Same here, move to QueueSwapPromise?

[Yufeng Shen (Slow to review), 2013-11-26 17:30:34]

https://codereview.chromium.org/60513007/diff/1130001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/1130001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1284: DCHECK(swap_promise_list_[i]);
On 2013/11/26 16:24:37, danakj wrote:
> Can you DCHECK this when they are added (QueueSwapPromise) instead of here?
That
> will find anything adding a NULL swap promise to the list faster, and give a
> better stack trace. DCHECK before a null deref doesn't add too much value.

Done.

https://codereview.chromium.org/60513007/diff/1130001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/60513007/diff/1130001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:729: DCHECK(swap_promise_list_[i]);
On 2013/11/26 16:24:37, danakj wrote:
> Same here, move to QueueSwapPromise?

Done.

[danakj, 2013-11-26 17:34:19]

LGTM

https://codereview.chromium.org/60513007/diff/1270001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/1270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1277: DCHECK(swap_promise.get());
nit: no .get() needed

https://codereview.chromium.org/60513007/diff/1270001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/60513007/diff/1270001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:715: DCHECK(swap_promise.get());
nit: no .get() needed for scoped_ptr

[Yufeng Shen (Slow to review), 2013-11-26 17:38:26]

https://codereview.chromium.org/60513007/diff/1270001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/60513007/diff/1270001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.cc:1277: DCHECK(swap_promise.get());
On 2013/11/26 17:34:19, danakj wrote:
> nit: no .get() needed

Done.

https://codereview.chromium.org/60513007/diff/1270001/cc/trees/layer_tree_imp...
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/60513007/diff/1270001/cc/trees/layer_tree_imp...
cc/trees/layer_tree_impl.cc:715: DCHECK(swap_promise.get());
On 2013/11/26 17:34:19, danakj wrote:
> nit: no .get() needed for scoped_ptr

Done.

[commit-bot: I haz the power, 2013-11-26 17:38:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/60513007/1290001

[commit-bot: I haz the power, 2013-11-26 17:54:01]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-26 18:12:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miletus@chromium.org/60513007/1310001

[commit-bot: I haz the power, 2013-11-27 00:05:21]

Change committed as 237444