aw: Fix invalidate getting lost

android_webview/browser/browser_view_renderer.cc

 // 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.
 
 #include "android_webview/browser/browser_view_renderer.h"
 
 #include "android_webview/browser/browser_view_renderer_client.h"
 #include "android_webview/browser/shared_renderer_state.h"
 #include "android_webview/public/browser/draw_gl.h"
 #include "base/android/jni_android.h"
@@ skipping 275 matching lines @@
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::CapturePicture");
 
   // Return empty Picture objects for empty SkPictures.
   if (width <= 0 || height <= 0) {
     return skia::AdoptRef(new SkPicture);
   }
 
   // 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);
   if (has_compositor_)
     CompositeSW(rec_canvas);
   return skia::AdoptRef(recorder.endRecording());
 }
 
 void BrowserViewRenderer::EnableOnNewPicture(bool enabled) {
   on_new_picture_enable_ = enabled;
@@ skipping 103 matching lines @@
 void BrowserViewRenderer::DidDestroyCompositor(
     content::SynchronousCompositor* compositor) {
   TRACE_EVENT0("android_webview", "BrowserViewRenderer::DidDestroyCompositor");
   DCHECK(has_compositor_);
   DCHECK(ui_task_runner_->BelongsToCurrentThread());
   has_compositor_ = false;
   shared_renderer_state_->SetCompositorOnUiThread(NULL);
   SynchronousCompositorMemoryPolicy zero_policy;
   DCHECK(shared_renderer_state_->GetMemoryPolicy() == zero_policy);
 }
 

[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));
+  {
+    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 (ui_task_runner_->BelongsToCurrentThread()) {
+    EnsureContinuousInvalidation(false);
     return;
   }
-  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);
+  ui_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&BrowserViewRenderer::EnsureContinuousInvalidation,
+                 ui_thread_weak_ptr_,
+                 false));
 }
 
 void BrowserViewRenderer::SetDipScale(float dip_scale) {
   dip_scale_ = dip_scale;
   CHECK(dip_scale_ > 0);
 }
 
 gfx::Vector2d BrowserViewRenderer::max_scroll_offset() const {
   DCHECK_GT(dip_scale_, 0);
   return gfx::ToCeiledVector2d(gfx::ScaleVector2d(
@@ skipping 15 matching lines @@
     scroll_offset_dip.set_y((scroll_offset.y() * max_scroll_offset_dip_.y()) /
                             max_offset.y());
   }
 
   DCHECK_LE(0, scroll_offset_dip.x());
   DCHECK_LE(0, scroll_offset_dip.y());
   DCHECK_LE(scroll_offset_dip.x(), max_scroll_offset_dip_.x());
   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;
 
     scroll_offset_dip_ = scroll_offset_dip;
   }
 
   if (has_compositor_)
     shared_renderer_state_->GetCompositor()->
         DidChangeRootLayerScrollOffset();
 }
@@ skipping 37 matching lines @@
   if (!ui_task_runner_->BelongsToCurrentThread()) {
     ui_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&BrowserViewRenderer::SetTotalRootLayerScrollOffset,
                    ui_thread_weak_ptr_,
                    scroll_offset_dip));
     return;
   }
 
   {
-    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)
       return;
 
     scroll_offset_dip_ = scroll_offset_dip;
   }
 
   gfx::Vector2d max_offset = max_scroll_offset();
   gfx::Vector2d scroll_offset;
@@ skipping 13 matching lines @@
   DCHECK(0 <= scroll_offset.y());
   // Disabled because the conditions are being violated while running
   // AwZoomTest.testMagnification, see http://crbug.com/340648
   // DCHECK(scroll_offset.x() <= max_offset.x());
   // DCHECK(scroll_offset.y() <= max_offset.y());
 
   client_->ScrollContainerViewTo(scroll_offset);
 }
 
 gfx::Vector2dF BrowserViewRenderer::GetTotalRootLayerScrollOffset() {
-  base::AutoLock lock(scroll_offset_dip_lock_);
+  base::AutoLock lock(render_thread_lock_);
   return scroll_offset_dip_;
 }
 
 bool BrowserViewRenderer::IsExternalFlingActive() const {
   if (!ui_task_runner_->BelongsToCurrentThread()) {
     // TODO(boliu): This is short term hack since we cannot call into
     // view system on non-UI thread.
     return false;
   }
   return client_->IsFlingActive();
@@ skipping 168 matching lines @@
     base::StringAppendF(&str,
                         "surface width height: [%d %d] ",
                         draw_info->width,
                         draw_info->height);
     base::StringAppendF(&str, "is_layer: %d ", draw_info->is_layer);
   }
   return str;
 }
 
 }  // namespace android_webview