cc: Make PrepareToDraw return an enum for why it aborts

cc: Make PrepareToDraw return an enum for why it aborts

This is mostly a cosmetic patch to change a boolean into an enum.

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

[enne (OOO), 2014-01-29 00:22:31]

The only thing I'm not so sure about is what to do about aborting due to CanDraw
/ !renderer / !root_layer.  In this first cut, I added them as additional
reasons that PrepareToDraw could abort.

In practice, the ThreadProxy guards PrepareToDraw with CanDraw which checks all
those conditions.  Should CalculateRenderPasses assert those instead of early
outing? (And SingleThreadProxy would then have a similar CanDraw guard for
PrepareToDraw?)

[brianderson, 2014-01-29 01:31:21]

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

https://codereview.chromium.org/131683005/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:1039: // Nothing to do in these cases. 
External state needs to change first.
I suppose this is one of the few logic changes, but it makes more sense like
this.

https://codereview.chromium.org/131683005/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:1053:
settings_.maximum_number_of_failed_draws_before_draw_is_forced_) {
Should we change the name of this setting? Doesn't have to be part of this
patch.

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

https://codereview.chromium.org/131683005/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.h:291: int
consecutive_checkerboard_animations_;
I like this name change.

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

https://codereview.chromium.org/131683005/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine_unittest.cc:188:
DrawSwapReadbackResult::DrawResult aborts[] = {
Thanks for testing all the new abort results.

https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:1147: layer_tree_host_impl_->PrepareToDraw(&frame,
readback_rect);
It feels funny to have PrepareToDraw return DID_DRAW when the draw hasn't
occurred yet, but I don't see how to make it any better without needlessly
complicating the code.

[brianderson, 2014-01-29 01:34:07]

On 2014/01/29 00:22:31, enne wrote:
> The only thing I'm not so sure about is what to do about aborting due to
CanDraw
> / !renderer / !root_layer.  In this first cut, I added them as additional
> reasons that PrepareToDraw could abort.
> 
> In practice, the ThreadProxy guards PrepareToDraw with CanDraw which checks
all
> those conditions.  Should CalculateRenderPasses assert those instead of early
> outing? (And SingleThreadProxy would then have a similar CanDraw guard for
> PrepareToDraw?)

If we can assert and remove the early outs, I think that would be preferable.

[enne (OOO), 2014-01-29 01:40:18]

https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:1147: layer_tree_host_impl_->PrepareToDraw(&frame,
readback_rect);
On 2014/01/29 01:31:22, Brian Anderson wrote:
> It feels funny to have PrepareToDraw return DID_DRAW when the draw hasn't
> occurred yet, but I don't see how to make it any better without needlessly
> complicating the code.

Yeah, I thought that too.  I could change DID_DRAW to DRAW_SUCCESS, and then it
might make sense before drawing and after drawing?

[danakj, 2014-01-29 01:42:35]

https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:1147: layer_tree_host_impl_->PrepareToDraw(&frame,
readback_rect);
On 2014/01/29 01:40:19, enne wrote:
> On 2014/01/29 01:31:22, Brian Anderson wrote:
> > It feels funny to have PrepareToDraw return DID_DRAW when the draw hasn't
> > occurred yet, but I don't see how to make it any better without needlessly
> > complicating the code.
> 
> Yeah, I thought that too.  I could change DID_DRAW to DRAW_SUCCESS, and then
it
> might make sense before drawing and after drawing?

WILL_DRAW?

[enne (OOO), 2014-01-29 01:47:20]

https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc#new...
cc/trees/thread_proxy.cc:1147: layer_tree_host_impl_->PrepareToDraw(&frame,
readback_rect);
On 2014/01/29 01:42:35, danakj wrote:
> On 2014/01/29 01:40:19, enne wrote:
> > On 2014/01/29 01:31:22, Brian Anderson wrote:
> > > It feels funny to have PrepareToDraw return DID_DRAW when the draw hasn't
> > > occurred yet, but I don't see how to make it any better without needlessly
> > > complicating the code.
> > 
> > Yeah, I thought that too.  I could change DID_DRAW to DRAW_SUCCESS, and then
> it
> > might make sense before drawing and after drawing?
> 
> WILL_DRAW?

WILL_DRAW doesn't make sense as a result after DrawAndSwapIfPossible.  This
seems like the reverse of why DID_DRAW doesn't make sense for PrepareToDraw.

[brianderson, 2014-01-29 01:54:29]

On 2014/01/29 01:47:20, enne wrote:
> https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc
> File cc/trees/thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/131683005/diff/1/cc/trees/thread_proxy.cc#new...
> cc/trees/thread_proxy.cc:1147: layer_tree_host_impl_->PrepareToDraw(&frame,
> readback_rect);
> On 2014/01/29 01:42:35, danakj wrote:
> > On 2014/01/29 01:40:19, enne wrote:
> > > On 2014/01/29 01:31:22, Brian Anderson wrote:
> > > > It feels funny to have PrepareToDraw return DID_DRAW when the draw
hasn't
> > > > occurred yet, but I don't see how to make it any better without
needlessly
> > > > complicating the code.
> > > 
> > > Yeah, I thought that too.  I could change DID_DRAW to DRAW_SUCCESS, and
then
> > it
> > > might make sense before drawing and after drawing?
> > 
> > WILL_DRAW?
> 
> WILL_DRAW doesn't make sense as a result after DrawAndSwapIfPossible.  This
> seems like the reverse of why DID_DRAW doesn't make sense for PrepareToDraw.

DRAW_SUCCESS seems like a good compromise.

[enne (OOO), 2014-01-29 20:57:05]

Removed early outs and just asserted.  Changed DID_DRAW to DRAW_SUCCESS.  PTAL

[danakj, 2014-01-29 21:56:28]

LGTM

https://codereview.chromium.org/131683005/diff/40001/cc/output/delegating_ren...
File cc/output/delegating_renderer_unittest.cc (right):

https://codereview.chromium.org/131683005/diff/40001/cc/output/delegating_ren...
cc/output/delegating_renderer_unittest.cc:114: return
DrawSwapReadbackResult::DRAW_SUCCESS;
nit: can you make this return result instead?

https://codereview.chromium.org/131683005/diff/40001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/131683005/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:1029: // Uninitialized
DrawSwapReadbackResult.
nit: put this string as NOTREACHED() << "Uninit... "; instead of as a comment?

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

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:703: DCHECK(CanDraw());
yay

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1124: // this
wrapping

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

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:537: // Returning false will result in a
forced draw.
"false"

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:592: // Returning false will result in a
forced draw.
"false"

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:2597: return
DrawSwapReadbackResult::DRAW_SUCCESS;
nit: return draw_result?

(was long-standing wish item to change how i did this)

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

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1127: return
DrawSwapReadbackResult::DRAW_SUCCESS;
nit: return draw_result?

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1220: return
DrawSwapReadbackResult::DRAW_SUCCESS;
nit: return draw_result?

https://codereview.chromium.org/131683005/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/131683005/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1113: result.did_swap = false;
these already init to false right? maybe we don't need to set them here?

[enne (OOO), 2014-01-29 23:09:22]

https://codereview.chromium.org/131683005/diff/40001/cc/output/delegating_ren...
File cc/output/delegating_renderer_unittest.cc (right):

https://codereview.chromium.org/131683005/diff/40001/cc/output/delegating_ren...
cc/output/delegating_renderer_unittest.cc:114: return
DrawSwapReadbackResult::DRAW_SUCCESS;
On 2014/01/29 21:56:29, danakj wrote:
> nit: can you make this return result instead?

Done.

https://codereview.chromium.org/131683005/diff/40001/cc/scheduler/scheduler_s...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/131683005/diff/40001/cc/scheduler/scheduler_s...
cc/scheduler/scheduler_state_machine.cc:1029: // Uninitialized
DrawSwapReadbackResult.
On 2014/01/29 21:56:29, danakj wrote:
> nit: put this string as NOTREACHED() << "Uninit... "; instead of as a comment?

Done.

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

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1124: // this
On 2014/01/29 21:56:29, danakj wrote:
> wrapping

80col

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

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:537: // Returning false will result in a
forced draw.
On 2014/01/29 21:56:29, danakj wrote:
> "false"

Done.

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:592: // Returning false will result in a
forced draw.
On 2014/01/29 21:56:29, danakj wrote:
> "false"

Done.

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest.cc:2597: return
DrawSwapReadbackResult::DRAW_SUCCESS;
On 2014/01/29 21:56:29, danakj wrote:
> nit: return draw_result?
> 
> (was long-standing wish item to change how i did this)

Done.

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

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1127: return
DrawSwapReadbackResult::DRAW_SUCCESS;
On 2014/01/29 21:56:29, danakj wrote:
> nit: return draw_result?

Done.

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_context.cc:1220: return
DrawSwapReadbackResult::DRAW_SUCCESS;
On 2014/01/29 21:56:29, danakj wrote:
> nit: return draw_result?

Done.

https://codereview.chromium.org/131683005/diff/40001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/131683005/diff/40001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1113: result.did_swap = false;
On 2014/01/29 21:56:29, danakj wrote:
> these already init to false right? maybe we don't need to set them here?

Done.

[danakj, 2014-01-29 23:10:47]

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

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1124: // this
On 2014/01/29 23:09:23, enne wrote:
> On 2014/01/29 21:56:29, danakj wrote:
> > wrapping
> 
> 80col

i meant the next line could join this one tho? "this" isn't quite 80 col long :)

[enne (OOO), 2014-01-29 23:11:27]

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

https://codereview.chromium.org/131683005/diff/40001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1124: // this
On 2014/01/29 23:09:23, enne wrote:
> On 2014/01/29 21:56:29, danakj wrote:
> > wrapping
> 
> 80col

<_<
>_>
Done.

[brianderson, 2014-01-30 00:15:17]

lgtm

[commit-bot: I haz the power, 2014-01-30 00:18:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/131683005/80001

[commit-bot: I haz the power, 2014-01-30 01:27:15]

Retried try job too often on android_clang_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2014-01-30 01:39:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/131683005/80001

[commit-bot: I haz the power, 2014-01-30 03:03:52]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-01-30 03:09:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/131683005/80001

[commit-bot: I haz the power, 2014-01-30 07:22:33]

Change committed as 247869