aw: Fix invalidate getting lost

android_webview/browser/browser_view_renderer.cc

Index: android_webview/browser/browser_view_renderer.cc
diff --git a/android_webview/browser/browser_view_renderer.cc b/android_webview/browser/browser_view_renderer.cc
index b8613b79dd7e8ac07062489603230228901ebb71..8e2234965e1917240c1882e23bdb3641019642e8 100644
--- a/android_webview/browser/browser_view_renderer.cc
+++ b/android_webview/browser/browser_view_renderer.cc
@@ -293,7 +293,7 @@ skia::RefPtr<SkPicture> BrowserViewRenderer::CapturePicture(int width,
   // Reset scroll back to the origin, will go back to the old
   // value when scroll_reset is out of scope.
   AutoResetWithLock scroll_reset(
-      &scroll_offset_dip_, gfx::Vector2dF(), scroll_offset_dip_lock_);
+      &scroll_offset_dip_, gfx::Vector2dF(), render_thread_lock_);
 
   SkPictureRecorder recorder;
   SkCanvas* rec_canvas = recorder.beginRecording(width, height, NULL, 0);
@@ -419,24 +419,28 @@ void BrowserViewRenderer::DidDestroyCompositor(
 }
 

[sgurun-gerrit only, 2014/05/05 17:11:01]

Please add explanation here why you need to set the invalidate under lock. You
don't want somebody else breaking this logic, especially since no test coverage
for it.

[boliu, 2014/05/05 17:13:41]

There is already a comment in the header saying the lock is used for "data that
must be set immediately". Is that good enough for you?

I'm actively working on ubercomp after which we don't need all this thread
hopping.

[sgurun-gerrit only, 2014/05/05 17:24:53]

If it is temporary it is ok, otherwise the explanation on the lock is not that
helpful.

 void BrowserViewRenderer::SetContinuousInvalidate(bool invalidate) {
-  if (!ui_task_runner_->BelongsToCurrentThread()) {
-    ui_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&BrowserViewRenderer::SetContinuousInvalidate,
-                   ui_thread_weak_ptr_,
-                   invalidate));
-    return;
+  {
+    base::AutoLock lock(render_thread_lock_);
+    if (compositor_needs_continuous_invalidate_ == invalidate)
+      return;
+
+    TRACE_EVENT_INSTANT1("android_webview",
+                         "BrowserViewRenderer::SetContinuousInvalidate",
+                         TRACE_EVENT_SCOPE_THREAD,
+                         "invalidate",
+                         invalidate);
+    compositor_needs_continuous_invalidate_ = invalidate;
   }
-  if (compositor_needs_continuous_invalidate_ == invalidate)
-    return;
 
-  TRACE_EVENT_INSTANT1("android_webview",
-                       "BrowserViewRenderer::SetContinuousInvalidate",
-                       TRACE_EVENT_SCOPE_THREAD,
-                       "invalidate",
-                       invalidate);
-  compositor_needs_continuous_invalidate_ = invalidate;
-  EnsureContinuousInvalidation(false);
+  if (ui_task_runner_->BelongsToCurrentThread()) {
+    EnsureContinuousInvalidation(false);
+    return;
+  }
+  ui_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&BrowserViewRenderer::EnsureContinuousInvalidation,
+                 ui_thread_weak_ptr_,
+                 false));
 }
 
 void BrowserViewRenderer::SetDipScale(float dip_scale) {
@@ -472,7 +476,7 @@ void BrowserViewRenderer::ScrollTo(gfx::Vector2d scroll_offset) {
   DCHECK_LE(scroll_offset_dip.y(), max_scroll_offset_dip_.y());
 
   {
-    base::AutoLock lock(scroll_offset_dip_lock_);
+    base::AutoLock lock(render_thread_lock_);
     if (scroll_offset_dip_ == scroll_offset_dip)
       return;
 
@@ -530,7 +534,7 @@ void BrowserViewRenderer::SetTotalRootLayerScrollOffset(
   }
 
   {
-    base::AutoLock lock(scroll_offset_dip_lock_);
+    base::AutoLock lock(render_thread_lock_);
     // TOOD(mkosiba): Add a DCHECK to say that this does _not_ get called during
     // DrawGl when http://crbug.com/249972 is fixed.
     if (scroll_offset_dip_ == scroll_offset_dip)
@@ -564,7 +568,7 @@ void BrowserViewRenderer::SetTotalRootLayerScrollOffset(
 }
 
 gfx::Vector2dF BrowserViewRenderer::GetTotalRootLayerScrollOffset() {
-  base::AutoLock lock(scroll_offset_dip_lock_);
+  base::AutoLock lock(render_thread_lock_);
   return scroll_offset_dip_;
 }