cc: Do not request redraw on commit when impl-side painting

cc/thread_proxy.cc

 // Copyright 2011 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.
 
 #include "cc/thread_proxy.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "cc/delay_based_time_source.h"
 #include "cc/draw_quad.h"
@@ skipping 393 matching lines @@
         m_layerTreeHost->contentsTextureManager()->memoryVisibleBytes(),
         m_layerTreeHost->contentsTextureManager()->memoryVisibleAndNearbyBytes(),
         m_layerTreeHost->contentsTextureManager()->memoryUseBytes());
 }
 
 void ThreadProxy::setNeedsRedraw()
 {
     DCHECK(isMainThread());
     TRACE_EVENT0("cc", "ThreadProxy::setNeedsRedraw");
     Proxy::implThread()->postTask(base::Bind(&ThreadProxy::setFullRootLayerDamageOnImplThread, m_implThreadWeakPtr));
-    Proxy::implThread()->postTask(base::Bind(&ThreadProxy::setNeedsRedrawOnImplThread, m_implThreadWeakPtr));
+    if (!m_layerTreeHost->settings().implSidePainting)

[enne (OOO), 2013/01/10 03:45:31]

I understand moving the conditional in the commit function, but why is this main
thread setNeedsRedraw conditional as well?

[nduca, 2013/01/10 11:01:09]

yeah this makes no sense to me

[brianderson, 2013/01/10 18:55:32]

Ah, I saw this being called after a commit and assumed it was called after every
commit.  It is only called after the first commit after a tab switch.  Will get
rid of this.

+        Proxy::implThread()->postTask(base::Bind(&ThreadProxy::setNeedsRedrawOnImplThread, m_implThreadWeakPtr));
 }
 
 void ThreadProxy::setDeferCommits(bool deferCommits)
 {
     DCHECK(isMainThread());
     DCHECK_NE(m_deferCommits, deferCommits);
     m_deferCommits = deferCommits;
 
     if (m_deferCommits)
         TRACE_EVENT_ASYNC_BEGIN0("cc", "ThreadProxy::setDeferCommits", this);
@@ skipping 300 matching lines @@
 
     m_layerTreeHostImpl->commitComplete();
 
     m_nextFrameIsNewlyCommittedFrameOnImplThread = true;
 
     m_commitCompletionEventOnImplThread->signal();
     m_commitCompletionEventOnImplThread = 0;
 
     // SetVisible kicks off the next scheduler action, so this must be last.
     m_schedulerOnImplThread->setVisible(m_layerTreeHostImpl->visible());
+

[nduca, 2013/01/10 11:01:09]

I dislike this.

I'd rather this was all in the state machine somehow. It smells like what you
need to do is say "if we commit && impl side painting mode, dont set needs
redraw."

[brianderson, 2013/01/10 18:55:32]

I had moved it out here because all the other setNeedsRedraws are initiated from
the this file and it was difficult for me to discover that a redraw was being
initiated from within the state machine itself, two levels lower.

By calling setNeedsRedraw here, do I run the risk of drawing at the wrong time
or drawing when we don't need to?

+    // This will also kick off the a scheduler action.

[enne (OOO), 2013/01/10 03:45:31]

Can you reconcile this with the previous comment that says "this must be last"?

+    if (!m_layerTreeHost->settings().implSidePainting)
+        m_schedulerOnImplThread->setNeedsRedraw();

[enne (OOO), 2013/01/10 03:45:31]

I feel like removing the redraw here needs to correspondingly have a
setNeedsRedraw in activatePendingTree.

[brianderson, 2013/01/10 18:55:32]

This is meant to go on top of my other patch, which does setNeedsRedraw in
onHasPendingTreeStateChanged.

 }
 
 void ThreadProxy::scheduledActionBeginContextRecreation()
 {
     DCHECK(isImplThread());
     Proxy::mainThread()->postTask(base::Bind(&ThreadProxy::beginContextRecreation, m_mainThreadWeakPtr));
 }
 
 ScheduledActionDrawAndSwapResult ThreadProxy::scheduledActionDrawAndSwapInternal(bool forcedDraw)
 {
@@ skipping 296 matching lines @@
 }
 
 void ThreadProxy::capturePictureOnImplThread(CompletionEvent* completion, skia::RefPtr<SkPicture>* picture)
 {
     DCHECK(isImplThread());
     *picture = m_layerTreeHostImpl->capturePicture();
     completion->signal();
 }
 
 }  // namespace cc