cc: Adding a CommitEarlyOutReason enum to replace bool in BeginMainFrameAbort method.
The bool is confusing and has meant a bunch of tests have ended up not testing
what they thought they were testing. The new enum clearly delineates between
cases were we abort without handing the commit and when we abort because the
commit was an effective nop.
This also allows new early out reasons to be easily added.
Using a C++ "enum class" to make sure all BeginMainFrameAborted cases are
converted by preventing implicit conversion to bool. Allowed on;
https://chromium-cpp.appspot.com/
BUG=416749
Review URL: https://codereview.chromium.org/793243006
Cr-Commit-Position: refs/heads/master@{#308739}
diff --git a/cc/BUILD.gn b/cc/BUILD.gn
index 47dcb6dc9..9aa3fde 100644
--- a/cc/BUILD.gn
+++ b/cc/BUILD.gn
@@ -462,6 +462,7 @@
"resources/zero_copy_tile_task_worker_pool.h",
"scheduler/begin_frame_source.cc",
"scheduler/begin_frame_source.h",
+ "scheduler/commit_earlyout_reason.h",
"scheduler/delay_based_time_source.cc",
"scheduler/delay_based_time_source.h",
"scheduler/draw_result.h",
diff --git a/cc/cc.gyp b/cc/cc.gyp
index 62655d7..1fee038 100644
--- a/cc/cc.gyp
+++ b/cc/cc.gyp
@@ -491,6 +491,7 @@
'resources/zero_copy_tile_task_worker_pool.h',
'scheduler/begin_frame_source.cc',
'scheduler/begin_frame_source.h',
+ 'scheduler/commit_earlyout_reason.h',
'scheduler/delay_based_time_source.cc',
'scheduler/delay_based_time_source.h',
'scheduler/draw_result.h',
diff --git a/cc/scheduler/commit_earlyout_reason.h b/cc/scheduler/commit_earlyout_reason.h
new file mode 100644
index 0000000..b45e96f
--- /dev/null
+++ b/cc/scheduler/commit_earlyout_reason.h
@@ -0,0 +1,37 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CC_SCHEDULER_COMMIT_EARLYOUT_REASON_H_
+#define CC_SCHEDULER_COMMIT_EARLYOUT_REASON_H_
+
+#include "base/logging.h"
+
+namespace cc {
+
+enum class CommitEarlyOutReason {
+ ABORTED_OUTPUT_SURFACE_LOST,
+ ABORTED_NOT_VISIBLE,
+ FINISHED_NO_UPDATES,
+};
+
+inline const char* CommitEarlyOutReasonToString(CommitEarlyOutReason reason) {
+ switch (reason) {
+ case CommitEarlyOutReason::ABORTED_OUTPUT_SURFACE_LOST:
+ return "CommitEarlyOutReason::ABORTED_OUTPUT_SURFACE_LOST";
+ case CommitEarlyOutReason::ABORTED_NOT_VISIBLE:
+ return "CommitEarlyOutReason::ABORTED_NOT_VISIBLE";
+ case CommitEarlyOutReason::FINISHED_NO_UPDATES:
+ return "CommitEarlyOutReason::FINISHED_NO_UPDATES";
+ }
+ NOTREACHED();
+ return "???";
+}
+
+inline bool CommitEarlyOutHandledCommit(CommitEarlyOutReason reason) {
+ return reason == CommitEarlyOutReason::FINISHED_NO_UPDATES;
+}
+
+} // namespace cc
+
+#endif // CC_SCHEDULER_COMMIT_EARLYOUT_REASON_H_
diff --git a/cc/scheduler/scheduler.cc b/cc/scheduler/scheduler.cc
index 59a22de..884c618 100644
--- a/cc/scheduler/scheduler.cc
+++ b/cc/scheduler/scheduler.cc
@@ -260,9 +260,10 @@
ProcessScheduledActions();
}
-void Scheduler::BeginMainFrameAborted(bool did_handle) {
- TRACE_EVENT0("cc", "Scheduler::BeginMainFrameAborted");
- state_machine_.BeginMainFrameAborted(did_handle);
+void Scheduler::BeginMainFrameAborted(CommitEarlyOutReason reason) {
+ TRACE_EVENT1("cc", "Scheduler::BeginMainFrameAborted", "reason",
+ CommitEarlyOutReasonToString(reason));
+ state_machine_.BeginMainFrameAborted(reason);
ProcessScheduledActions();
}
diff --git a/cc/scheduler/scheduler.h b/cc/scheduler/scheduler.h
index dea7c3e54..133b676 100644
--- a/cc/scheduler/scheduler.h
+++ b/cc/scheduler/scheduler.h
@@ -128,7 +128,7 @@
void SetImplLatencyTakesPriority(bool impl_latency_takes_priority);
void NotifyReadyToCommit();
- void BeginMainFrameAborted(bool did_handle);
+ void BeginMainFrameAborted(CommitEarlyOutReason reason);
void DidPrepareTiles();
void DidLoseOutputSurface();
diff --git a/cc/scheduler/scheduler_state_machine.cc b/cc/scheduler/scheduler_state_machine.cc
index dc3fa36..323def6 100644
--- a/cc/scheduler/scheduler_state_machine.cc
+++ b/cc/scheduler/scheduler_state_machine.cc
@@ -550,8 +550,8 @@
return;
case ACTION_COMMIT: {
- bool commit_was_aborted = false;
- UpdateStateOnCommit(commit_was_aborted);
+ bool commit_has_no_updates = false;
+ UpdateStateOnCommit(commit_has_no_updates);
return;
}
@@ -586,13 +586,13 @@
}
}
-void SchedulerStateMachine::UpdateStateOnCommit(bool commit_was_aborted) {
+void SchedulerStateMachine::UpdateStateOnCommit(bool commit_has_no_updates) {
commit_count_++;
- if (!commit_was_aborted && HasAnimatedThisFrame())
+ if (!commit_has_no_updates && HasAnimatedThisFrame())
did_commit_after_animating_ = true;
- if (commit_was_aborted || settings_.main_frame_before_activation_enabled) {
+ if (commit_has_no_updates || settings_.main_frame_before_activation_enabled) {
commit_state_ = COMMIT_STATE_IDLE;
} else if (settings_.impl_side_painting) {
commit_state_ = COMMIT_STATE_WAITING_FOR_ACTIVATION;
@@ -604,7 +604,7 @@
// If we are impl-side-painting but the commit was aborted, then we behave
// mostly as if we are not impl-side-painting since there is no pending tree.
- has_pending_tree_ = settings_.impl_side_painting && !commit_was_aborted;
+ has_pending_tree_ = settings_.impl_side_painting && !commit_has_no_updates;
// Update state related to forced draws.
if (forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_COMMIT) {
@@ -627,7 +627,7 @@
// Update state if we have a new active tree to draw, or if the active tree
// was unchanged but we need to do a forced draw.
if (!has_pending_tree_ &&
- (!commit_was_aborted ||
+ (!commit_has_no_updates ||
forced_redraw_state_ == FORCED_REDRAW_STATE_WAITING_FOR_DRAW)) {
needs_redraw_ = true;
active_tree_needs_first_draw_ = true;
@@ -1025,14 +1025,18 @@
DCHECK(ShouldCommit());
}
-void SchedulerStateMachine::BeginMainFrameAborted(bool did_handle) {
+void SchedulerStateMachine::BeginMainFrameAborted(CommitEarlyOutReason reason) {
DCHECK_EQ(commit_state_, COMMIT_STATE_BEGIN_MAIN_FRAME_SENT);
- if (did_handle) {
- bool commit_was_aborted = true;
- UpdateStateOnCommit(commit_was_aborted);
- } else {
- commit_state_ = COMMIT_STATE_IDLE;
- SetNeedsCommit();
+ switch (reason) {
+ case CommitEarlyOutReason::ABORTED_OUTPUT_SURFACE_LOST:
+ case CommitEarlyOutReason::ABORTED_NOT_VISIBLE:
+ commit_state_ = COMMIT_STATE_IDLE;
+ SetNeedsCommit();
+ return;
+ case CommitEarlyOutReason::FINISHED_NO_UPDATES:
+ bool commit_has_no_updates = true;
+ UpdateStateOnCommit(commit_has_no_updates);
+ return;
}
}
diff --git a/cc/scheduler/scheduler_state_machine.h b/cc/scheduler/scheduler_state_machine.h
index 7ee039a..4d7bf6f 100644
--- a/cc/scheduler/scheduler_state_machine.h
+++ b/cc/scheduler/scheduler_state_machine.h
@@ -12,6 +12,7 @@
#include "base/time/time.h"
#include "cc/base/cc_export.h"
#include "cc/output/begin_frame_args.h"
+#include "cc/scheduler/commit_earlyout_reason.h"
#include "cc/scheduler/draw_result.h"
#include "cc/scheduler/scheduler_settings.h"
@@ -206,8 +207,7 @@
// Call this only in response to receiving an ACTION_SEND_BEGIN_MAIN_FRAME
// from NextAction if the client rejects the BeginMainFrame message.
- // If did_handle is false, then another commit will be retried soon.
- void BeginMainFrameAborted(bool did_handle);
+ void BeginMainFrameAborted(CommitEarlyOutReason reason);
// Set that we can create the first OutputSurface and start the scheduler.
void SetCanStart() { can_start_ = true; }
@@ -286,7 +286,7 @@
bool HasRequestedSwapThisFrame() const;
bool HasSwappedThisFrame() const;
- void UpdateStateOnCommit(bool commit_was_aborted);
+ void UpdateStateOnCommit(bool commit_had_no_updates);
void UpdateStateOnActivation();
void UpdateStateOnDraw(bool did_request_swap);
void UpdateStateOnPrepareTiles();
diff --git a/cc/scheduler/scheduler_state_machine_unittest.cc b/cc/scheduler/scheduler_state_machine_unittest.cc
index 2d896a2..5a602768 100644
--- a/cc/scheduler/scheduler_state_machine_unittest.cc
+++ b/cc/scheduler/scheduler_state_machine_unittest.cc
@@ -1104,9 +1104,7 @@
// Become invisible and abort BeginMainFrame.
state.SetVisible(false);
- // False means that the BeginMainFrame was sent but we never forwarded it to
- // the main thread because by the time it arrived we couldn't process it.
- state.BeginMainFrameAborted(false);
+ state.BeginMainFrameAborted(CommitEarlyOutReason::ABORTED_NOT_VISIBLE);
// NeedsCommit should now be true again because we never actually did a
// commit.
@@ -1162,7 +1160,7 @@
// Abort the commit, true means that the BeginMainFrame was sent but there
// was no work to do on the main thread.
- state.BeginMainFrameAborted(true);
+ state.BeginMainFrameAborted(CommitEarlyOutReason::FINISHED_NO_UPDATES);
// NeedsCommit should now be false because the commit was actually handled.
EXPECT_FALSE(state.NeedsCommit());
@@ -1622,7 +1620,7 @@
// Since only the scroll offset changed, the main thread will abort the
// commit.
- state.BeginMainFrameAborted(true);
+ state.BeginMainFrameAborted(CommitEarlyOutReason::FINISHED_NO_UPDATES);
// Since the commit was aborted, we should draw right away instead of waiting
// for the deadline.
diff --git a/cc/test/layer_tree_test.cc b/cc/test/layer_tree_test.cc
index 0613f85..e2fc182 100644
--- a/cc/test/layer_tree_test.cc
+++ b/cc/test/layer_tree_test.cc
@@ -179,9 +179,9 @@
test_hooks_->WillBeginImplFrameOnThread(this, args);
}
- void BeginMainFrameAborted(bool did_handle) override {
- LayerTreeHostImpl::BeginMainFrameAborted(did_handle);
- test_hooks_->BeginMainFrameAbortedOnThread(this, did_handle);
+ void BeginMainFrameAborted(CommitEarlyOutReason reason) override {
+ LayerTreeHostImpl::BeginMainFrameAborted(reason);
+ test_hooks_->BeginMainFrameAbortedOnThread(this, reason);
}
void BeginCommit() override {
diff --git a/cc/test/layer_tree_test.h b/cc/test/layer_tree_test.h
index ddeebba..1587f2c 100644
--- a/cc/test/layer_tree_test.h
+++ b/cc/test/layer_tree_test.h
@@ -43,7 +43,7 @@
virtual void WillBeginImplFrameOnThread(LayerTreeHostImpl* host_impl,
const BeginFrameArgs& args) {}
virtual void BeginMainFrameAbortedOnThread(LayerTreeHostImpl* host_impl,
- bool did_handle) {}
+ CommitEarlyOutReason reason) {}
virtual void BeginCommitOnThread(LayerTreeHostImpl* host_impl) {}
virtual void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) {}
virtual void WillActivateTreeOnThread(LayerTreeHostImpl* host_impl) {}
diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc
index c9d76c6..fe85134 100644
--- a/cc/trees/layer_tree_host_impl.cc
+++ b/cc/trees/layer_tree_host_impl.cc
@@ -278,11 +278,11 @@
DestroyTileManager();
}
-void LayerTreeHostImpl::BeginMainFrameAborted(bool did_handle) {
+void LayerTreeHostImpl::BeginMainFrameAborted(CommitEarlyOutReason reason) {
// If the begin frame data was handled, then scroll and scale set was applied
// by the main thread, so the active tree needs to be updated as if these sent
// values were applied and committed.
- if (did_handle) {
+ if (CommitEarlyOutHandledCommit(reason)) {
active_tree_->ApplySentScrollAndScaleDeltasFromAbortedCommit();
active_tree_->ResetContentsTexturesPurged();
}
diff --git a/cc/trees/layer_tree_host_impl.h b/cc/trees/layer_tree_host_impl.h
index 54a16aa..03ddaa0 100644
--- a/cc/trees/layer_tree_host_impl.h
+++ b/cc/trees/layer_tree_host_impl.h
@@ -32,6 +32,7 @@
#include "cc/quads/render_pass.h"
#include "cc/resources/resource_provider.h"
#include "cc/resources/tile_manager.h"
+#include "cc/scheduler/commit_earlyout_reason.h"
#include "cc/scheduler/draw_result.h"
#include "skia/ext/refptr.h"
#include "third_party/skia/include/core/SkColor.h"
@@ -190,7 +191,7 @@
void AppendRenderPass(scoped_ptr<RenderPass> render_pass) override;
};
- virtual void BeginMainFrameAborted(bool did_handle);
+ virtual void BeginMainFrameAborted(CommitEarlyOutReason reason);
virtual void BeginCommit();
virtual void CommitComplete();
virtual void Animate(base::TimeTicks monotonic_time);
diff --git a/cc/trees/layer_tree_host_unittest.cc b/cc/trees/layer_tree_host_unittest.cc
index a343466..2308a7a7 100644
--- a/cc/trees/layer_tree_host_unittest.cc
+++ b/cc/trees/layer_tree_host_unittest.cc
@@ -2508,7 +2508,7 @@
}
void BeginMainFrameAbortedOnThread(LayerTreeHostImpl* host_impl,
- bool did_handle) override {
+ CommitEarlyOutReason reason) override {
commit_abort_count_++;
// Initiate another abortable commit.
host_impl->SetNeedsCommit();
@@ -4810,7 +4810,7 @@
}
void BeginMainFrameAbortedOnThread(LayerTreeHostImpl* host_impl,
- bool did_handle) override {
+ CommitEarlyOutReason reason) override {
EndTest();
}
@@ -4851,7 +4851,7 @@
}
void BeginMainFrameAbortedOnThread(LayerTreeHostImpl* host_impl,
- bool did_handle) override {
+ CommitEarlyOutReason reason) override {
// This is needed so that the impl-thread state matches main-thread state.
host_impl->DidLoseOutputSurface();
EndTest();
diff --git a/cc/trees/single_thread_proxy.cc b/cc/trees/single_thread_proxy.cc
index 816050b..a6e4fef 100644
--- a/cc/trees/single_thread_proxy.cc
+++ b/cc/trees/single_thread_proxy.cc
@@ -12,6 +12,7 @@
#include "cc/quads/draw_quad.h"
#include "cc/resources/prioritized_resource_manager.h"
#include "cc/resources/resource_update_controller.h"
+#include "cc/scheduler/commit_earlyout_reason.h"
#include "cc/trees/layer_tree_host.h"
#include "cc/trees/layer_tree_host_single_thread_client.h"
#include "cc/trees/layer_tree_impl.h"
@@ -677,14 +678,16 @@
if (!layer_tree_host_->visible()) {
TRACE_EVENT_INSTANT0("cc", "EarlyOut_NotVisible", TRACE_EVENT_SCOPE_THREAD);
- BeginMainFrameAbortedOnImplThread();
+ BeginMainFrameAbortedOnImplThread(
+ CommitEarlyOutReason::ABORTED_NOT_VISIBLE);
return;
}
if (layer_tree_host_->output_surface_lost()) {
TRACE_EVENT_INSTANT0(
"cc", "EarlyOut_OutputSurfaceLost", TRACE_EVENT_SCOPE_THREAD);
- BeginMainFrameAbortedOnImplThread();
+ BeginMainFrameAbortedOnImplThread(
+ CommitEarlyOutReason::ABORTED_OUTPUT_SURFACE_LOST);
return;
}
@@ -716,22 +719,23 @@
timing_history_.DidBeginMainFrame();
+ // TODO(enne): SingleThreadProxy does not support cancelling commits yet,
+ // search for CommitEarlyOutReason::FINISHED_NO_UPDATES inside
+ // thread_proxy.cc
if (scheduler_on_impl_thread_) {
scheduler_on_impl_thread_->NotifyBeginMainFrameStarted();
scheduler_on_impl_thread_->NotifyReadyToCommit();
}
}
-void SingleThreadProxy::BeginMainFrameAbortedOnImplThread() {
+void SingleThreadProxy::BeginMainFrameAbortedOnImplThread(
+ CommitEarlyOutReason reason) {
DebugScopedSetImplThread impl(this);
DCHECK(scheduler_on_impl_thread_->CommitPending());
DCHECK(!layer_tree_host_impl_->pending_tree());
- // TODO(enne): SingleThreadProxy does not support cancelling commits yet so
- // did_handle is always false.
- bool did_handle = false;
- layer_tree_host_impl_->BeginMainFrameAborted(did_handle);
- scheduler_on_impl_thread_->BeginMainFrameAborted(did_handle);
+ layer_tree_host_impl_->BeginMainFrameAborted(reason);
+ scheduler_on_impl_thread_->BeginMainFrameAborted(reason);
}
DrawResult SingleThreadProxy::ScheduledActionDrawAndSwapIfPossible() {
diff --git a/cc/trees/single_thread_proxy.h b/cc/trees/single_thread_proxy.h
index 82691a2..6f6e91f 100644
--- a/cc/trees/single_thread_proxy.h
+++ b/cc/trees/single_thread_proxy.h
@@ -120,7 +120,7 @@
scoped_ptr<BeginFrameSource> external_begin_frame_source);
void BeginMainFrame();
- void BeginMainFrameAbortedOnImplThread();
+ void BeginMainFrameAbortedOnImplThread(CommitEarlyOutReason reason);
void DoAnimate();
void DoBeginMainFrame(const BeginFrameArgs& begin_frame_args);
void DoCommit();
diff --git a/cc/trees/thread_proxy.cc b/cc/trees/thread_proxy.cc
index 73bf79e..389f296e 100644
--- a/cc/trees/thread_proxy.cc
+++ b/cc/trees/thread_proxy.cc
@@ -20,6 +20,7 @@
#include "cc/output/output_surface.h"
#include "cc/quads/draw_quad.h"
#include "cc/resources/prioritized_resource_manager.h"
+#include "cc/scheduler/commit_earlyout_reason.h"
#include "cc/scheduler/delay_based_time_source.h"
#include "cc/scheduler/scheduler.h"
#include "cc/trees/blocking_task_runner.h"
@@ -697,24 +698,21 @@
if (!layer_tree_host()->visible()) {
TRACE_EVENT_INSTANT0("cc", "EarlyOut_NotVisible", TRACE_EVENT_SCOPE_THREAD);
- bool did_handle = false;
Proxy::ImplThreadTaskRunner()->PostTask(
- FROM_HERE,
- base::Bind(&ThreadProxy::BeginMainFrameAbortedOnImplThread,
- impl_thread_weak_ptr_,
- did_handle));
+ FROM_HERE, base::Bind(&ThreadProxy::BeginMainFrameAbortedOnImplThread,
+ impl_thread_weak_ptr_,
+ CommitEarlyOutReason::ABORTED_NOT_VISIBLE));
return;
}
if (layer_tree_host()->output_surface_lost()) {
TRACE_EVENT_INSTANT0(
"cc", "EarlyOut_OutputSurfaceLost", TRACE_EVENT_SCOPE_THREAD);
- bool did_handle = false;
Proxy::ImplThreadTaskRunner()->PostTask(
FROM_HERE,
base::Bind(&ThreadProxy::BeginMainFrameAbortedOnImplThread,
impl_thread_weak_ptr_,
- did_handle));
+ CommitEarlyOutReason::ABORTED_OUTPUT_SURFACE_LOST));
return;
}
@@ -786,12 +784,10 @@
if (!updated && can_cancel_this_commit) {
TRACE_EVENT_INSTANT0("cc", "EarlyOut_NoUpdates", TRACE_EVENT_SCOPE_THREAD);
- bool did_handle = true;
Proxy::ImplThreadTaskRunner()->PostTask(
- FROM_HERE,
- base::Bind(&ThreadProxy::BeginMainFrameAbortedOnImplThread,
- impl_thread_weak_ptr_,
- did_handle));
+ FROM_HERE, base::Bind(&ThreadProxy::BeginMainFrameAbortedOnImplThread,
+ impl_thread_weak_ptr_,
+ CommitEarlyOutReason::FINISHED_NO_UPDATES));
// Although the commit is internally aborted, this is because it has been
// detected to be a no-op. From the perspective of an embedder, this commit
@@ -884,17 +880,19 @@
impl().scheduler->AnticipatedDrawTime());
}
-void ThreadProxy::BeginMainFrameAbortedOnImplThread(bool did_handle) {
- TRACE_EVENT0("cc", "ThreadProxy::BeginMainFrameAbortedOnImplThread");
+void ThreadProxy::BeginMainFrameAbortedOnImplThread(
+ CommitEarlyOutReason reason) {
+ TRACE_EVENT1("cc", "ThreadProxy::BeginMainFrameAbortedOnImplThread", "reason",
+ CommitEarlyOutReasonToString(reason));
DCHECK(IsImplThread());
DCHECK(impl().scheduler);
DCHECK(impl().scheduler->CommitPending());
DCHECK(!impl().layer_tree_host_impl->pending_tree());
- if (did_handle)
+ if (CommitEarlyOutHandledCommit(reason))
SetInputThrottledUntilCommitOnImplThread(false);
- impl().layer_tree_host_impl->BeginMainFrameAborted(did_handle);
- impl().scheduler->BeginMainFrameAborted(did_handle);
+ impl().layer_tree_host_impl->BeginMainFrameAborted(reason);
+ impl().scheduler->BeginMainFrameAborted(reason);
}
void ThreadProxy::ScheduledActionAnimate() {
diff --git a/cc/trees/thread_proxy.h b/cc/trees/thread_proxy.h
index 14733f7..6ca21d5 100644
--- a/cc/trees/thread_proxy.h
+++ b/cc/trees/thread_proxy.h
@@ -14,6 +14,7 @@
#include "cc/base/completion_event.h"
#include "cc/base/delayed_unique_notifier.h"
#include "cc/resources/resource_update_controller.h"
+#include "cc/scheduler/commit_earlyout_reason.h"
#include "cc/scheduler/scheduler.h"
#include "cc/trees/layer_tree_host_impl.h"
#include "cc/trees/proxy.h"
@@ -255,7 +256,7 @@
void StartCommitOnImplThread(CompletionEvent* completion,
ResourceUpdateQueue* queue);
- void BeginMainFrameAbortedOnImplThread(bool did_handle);
+ void BeginMainFrameAbortedOnImplThread(CommitEarlyOutReason reason);
void FinishAllRenderingOnImplThread(CompletionEvent* completion);
void InitializeImplOnImplThread(CompletionEvent* completion);
void SetLayerTreeHostClientReadyOnImplThread();