cc: Add more DrawSwapReadbackResult enums

cc: Add more DrawSwapReadbackResult enums

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

[enne (OOO), 2014-02-13 02:40:32]

This is partially to help http://crbug.com/329552, but also because I noticed
that there were some codepaths inside of ThreadProxy::DrawSwapReadbackInternal
that would leave DrawSwapReadbackResult::draw_result unset.  This seems pretty
wrong to me.

I want to admit up front that I feel like there's some code smell before (and
with) this patch, but I'm not sure what to do about it.  (Suggestions are very
welcome.)

The weirdness for me is that I think it's impossible for DrawAndSwapIfPossible
to end up in any of these three new states (because it wouldn't get called if
they were true), even though DrawSwapReadbackInternal can generate them (for
forced draws and forced readback).  That means that the SchedulerStateMachine
no-op code is effectively dead, since it only handles DrawAndSwapIfPossible
results.

I'm not really sure what the right answer here is.  Would it be cleaner to
revert my previous change (https://codereview.chromium.org/131683005) and just
have DrawAndSwapReadbackResult have a bool "did_draw" as before, but also two
"had_checkerboarded_animations" and "had_missing_high_res_content" flags? That
would be enough information for the state machine.

On the other hand, aborting reasons are sort of useful for debugging and I could
stick them in traces as a follow-up.

[danakj, 2014-02-13 20:43:54]

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

https://codereview.chromium.org/162473003/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:1074: // Nothing to do.
Should these needs_redraw_ = true; ? That's what would have happened before you
added these enums right?

[enne (OOO), 2014-02-13 20:47:40]

On 2014/02/13 20:43:54, danakj wrote:
>
https://codereview.chromium.org/162473003/diff/1/cc/scheduler/scheduler_state...
> File cc/scheduler/scheduler_state_machine.cc (right):
> 
>
https://codereview.chromium.org/162473003/diff/1/cc/scheduler/scheduler_state...
> cc/scheduler/scheduler_state_machine.cc:1074: // Nothing to do.
> Should these needs_redraw_ = true; ? That's what would have happened before
you
> added these enums right?

I don't think these ever get hit in the state machine.  If they did, they would
have hit the invalid NOTREACHED case?

[danakj, 2014-02-13 20:49:38]

On Thu, Feb 13, 2014 at 3:47 PM, <enne@chromium.org> wrote:

> On 2014/02/13 20:43:54, danakj wrote:
>
> https://codereview.chromium.org/162473003/diff/1/cc/
> scheduler/scheduler_state_machine.cc
>
>> File cc/scheduler/scheduler_state_machine.cc (right):
>>
>
>
> https://codereview.chromium.org/162473003/diff/1/cc/
> scheduler/scheduler_state_machine.cc#newcode1074
>
>> cc/scheduler/scheduler_state_machine.cc:1074: // Nothing to do.
>> Should these needs_redraw_ = true; ? That's what would have happened
>> before
>>
> you
>
>> added these enums right?
>>
>
> I don't think these ever get hit in the state machine.  If they did, they
> would
> have hit the invalid NOTREACHED case?
>

If you're running in debug.. Should these be NOTREACHED then too?


>
> https://codereview.chromium.org/162473003/
>

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

[brianderson, 2014-02-14 01:41:50]

I really like the aborting reasons and would like to keep them.

It's a little unfortunate that we expose reasons that the state machine
shouldn't care about for DrawAndSwapIfPossible, but passing the reason directly
is so clean otherwise.

Can you add detail to the comments regarding why certain reasons aren't
expected?

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

https://codereview.chromium.org/162473003/diff/1/cc/scheduler/scheduler_state...
cc/scheduler/scheduler_state_machine.cc:1074: // Nothing to do.
I think it would be best to combine these states with the INVALID state use
NOTREACHED for all of them.

[enne (OOO), 2014-02-14 21:31:38]

Ok, everybody loves enums.  I added more checks and NOTREACHED.  I think these
states can never be generated from DrawAndSwapIfPossible so long as !readback
and CanDraw.

Tell me what y'all think?

[brianderson, 2014-02-14 21:41:05]

lgtm

[danakj, 2014-02-14 21:43:35]

LGTM

https://codereview.chromium.org/162473003/diff/120001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/162473003/diff/120001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_state_machine.cc:1044: NOTREACHED() << "Invalid return
value from DrawAndSwapIfPossible.";
Can you include the result value in the ouput?

[enne (OOO), 2014-02-14 21:49:25]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-02-14 21:51:19]

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

[commit-bot: I haz the power, 2014-02-14 21:51:22]

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

[commit-bot: I haz the power, 2014-02-14 21:51:22]

Failed to apply patch for cc/trees/thread_proxy.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/trees/thread_proxy.cc
  Hunk #1 succeeded at 1173 (offset 13 lines).
  Hunk #2 succeeded at 1220 (offset 13 lines).
  Hunk #3 FAILED at 1238.
  Hunk #4 succeeded at 1279 (offset 13 lines).
  Hunk #5 succeeded at 1332 (offset 13 lines).
  1 out of 5 hunks FAILED -- saving rejects to file cc/trees/thread_proxy.cc.rej

Patch:       cc/trees/thread_proxy.cc
Index: cc/trees/thread_proxy.cc
diff --git a/cc/trees/thread_proxy.cc b/cc/trees/thread_proxy.cc
index
860144b6151adc3f4adf99f2c3212f164f516cd9..9bf4d381c1a35d06568f28a2280be81634f96181
100644
--- a/cc/trees/thread_proxy.cc
+++ b/cc/trees/thread_proxy.cc
@@ -1160,17 +1160,22 @@ DrawSwapReadbackResult
ThreadProxy::DrawSwapReadbackInternal(
   LayerTreeHostImpl::FrameData frame;
   bool draw_frame = false;
 
-  if (impl().layer_tree_host_impl->CanDraw() &&
-      (!drawing_for_readback || can_do_readback)) {
-    // If it is for a readback, make sure we draw the portion being read back.
-    gfx::Rect readback_rect;
-    if (drawing_for_readback)
-      readback_rect = impl().readback_request->rect;
-
-    result.draw_result =
-        impl().layer_tree_host_impl->PrepareToDraw(&frame, readback_rect);
-    draw_frame = forced_draw ||
-                 result.draw_result == DrawSwapReadbackResult::DRAW_SUCCESS;
+  if (impl().layer_tree_host_impl->CanDraw()) {
+    if (!drawing_for_readback || can_do_readback) {
+      // If it is for a readback, make sure we draw the portion being read
back.
+      gfx::Rect readback_rect;
+      if (drawing_for_readback)
+        readback_rect = impl().readback_request->rect;
+
+      result.draw_result =
+          impl().layer_tree_host_impl->PrepareToDraw(&frame, readback_rect);
+      draw_frame = forced_draw ||
+                   result.draw_result == DrawSwapReadbackResult::DRAW_SUCCESS;
+    } else {
+      result.draw_result = DrawSwapReadbackResult::DRAW_ABORTED_CANT_READBACK;
+    }
+  } else {
+    result.draw_result = DrawSwapReadbackResult::DRAW_ABORTED_CANT_DRAW;
   }
 
   if (draw_frame) {
@@ -1202,10 +1207,14 @@ DrawSwapReadbackResult
ThreadProxy::DrawSwapReadbackInternal(
   if (drawing_for_readback) {
     DCHECK(!swap_requested);
     result.did_readback = false;
-    if (draw_frame && !impl().layer_tree_host_impl->IsContextLost()) {
-      impl().layer_tree_host_impl->Readback(impl().readback_request->pixels,
-                                            impl().readback_request->rect);
-      result.did_readback = true;
+    if (draw_frame) {
+      if (!impl().layer_tree_host_impl->IsContextLost()) {
+        impl().layer_tree_host_impl->Readback(impl().readback_request->pixels,
+                                              impl().readback_request->rect);
+        result.did_readback = true;
+      } else {
+        result.draw_result = DrawSwapReadbackResult::DRAW_ABORTED_CONTEXT_LOST;
+      }
     }
     impl().readback_request->success = result.did_readback;
     impl().readback_request->completion.Signal();
@@ -1229,9 +1238,10 @@ DrawSwapReadbackResult
ThreadProxy::DrawSwapReadbackInternal(
         base::Bind(&ThreadProxy::DidCommitAndDrawFrame,
main_thread_weak_ptr_));
   }
 
-  if (draw_frame) {
+  if (draw_frame)
     CheckOutputSurfaceStatusOnImplThread();
 
+  if (result.draw_result == DrawSwapReadbackResult::DRAW_SUCCESS) {
     base::TimeDelta draw_duration = impl().timing_history.DidFinishDrawing();
 
     base::TimeDelta draw_duration_overestimate;
@@ -1257,6 +1267,7 @@ DrawSwapReadbackResult
ThreadProxy::DrawSwapReadbackInternal(
                                50);
   }
 
+  DCHECK_NE(DrawSwapReadbackResult::INVALID_RESULT, result.draw_result);
   return result;
 }
 
@@ -1309,6 +1320,12 @@ void ThreadProxy::ScheduledActionManageTiles() {
 
 DrawSwapReadbackResult ThreadProxy::ScheduledActionDrawAndSwapIfPossible() {
   TRACE_EVENT0("cc", "ThreadProxy::ScheduledActionDrawAndSwap");
+
+  // SchedulerStateMachine::DidDrawIfPossibleCompleted isn't set up to
+  // handle DRAW_ABORTED_CANT_DRAW.  Moreover, the scheduler should
+  // never generate this call when it can't draw.
+  DCHECK(impl().layer_tree_host_impl->CanDraw());
+
   bool forced_draw = false;
   bool swap_requested = true;
   bool readback_requested = false;

[enne (OOO), 2014-02-15 08:13:10]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2014-02-15 08:13:11]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2014-02-15 08:13:12]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-02-15 08:13:14]

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

[commit-bot: I haz the power, 2014-02-15 14:00:32]

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

[commit-bot: I haz the power, 2014-02-15 14:00:33]

Retried try job too often on win_rel for step(s) nacl_integration,
sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[enne (OOO), 2014-02-16 07:57:18]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2014-02-16 07:57:19]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2014-02-16 07:57:19]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-02-16 07:57:23]

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

[commit-bot: I haz the power, 2014-02-16 22:09:48]

Change committed as 251619