Android: Remove ContentViewCore::GetLayer() from public interface

chrome/browser/android/compositor/tab_content_manager.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 "chrome/browser/android/compositor/tab_content_manager.h"
 
 #include <android/bitmap.h>
 #include <stddef.h>
 
 #include <utility>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "cc/layers/layer.h"
 #include "chrome/browser/android/compositor/layer/thumbnail_layer.h"
 #include "chrome/browser/android/tab_android.h"
 #include "chrome/browser/android/thumbnail/thumbnail.h"
-#include "content/public/browser/android/content_view_core.h"
 #include "content/public/browser/readback_types.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "jni/TabContentManager_jni.h"
 #include "ui/android/resources/ui_resource_provider.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/geometry/dip_util.h"
 #include "ui/gfx/geometry/rect.h"
 #include "url/gurl.h"
 
 namespace {
 
 const size_t kMaxReadbacks = 1;
 typedef base::Callback<void(float, const SkBitmap&)> TabReadbackCallback;
 
 }  // namespace
 
 namespace chrome {
 namespace android {
 
 class TabContentManager::TabReadbackRequest {
  public:
-  TabReadbackRequest(jobject content_view_core,
+  TabReadbackRequest(content::WebContents* web_contents,
                      float thumbnail_scale,
                      const TabReadbackCallback& end_callback)
       : thumbnail_scale_(thumbnail_scale),
         end_callback_(end_callback),
         drop_after_readback_(false),
         weak_factory_(this) {
-    JNIEnv* env = base::android::AttachCurrentThread();
-    j_content_view_core_.Reset(env, content_view_core);
-  }
-
-  virtual ~TabReadbackRequest() {}
-
-  void Run() {
-    JNIEnv* env = base::android::AttachCurrentThread();
+    DCHECK(web_contents);
     content::ReadbackRequestCallback result_callback =
         base::Bind(&TabReadbackRequest::OnFinishGetTabThumbnailBitmap,
                    weak_factory_.GetWeakPtr());
 
-    if (j_content_view_core_.is_null()) {
-      result_callback.Run(SkBitmap(), content::READBACK_FAILED);
-      return;
-    }
-
-    content::ContentViewCore* view =
-        content::ContentViewCore::GetNativeContentViewCore(
-            env, j_content_view_core_.obj());
-
-    if (!view) {
-      result_callback.Run(SkBitmap(), content::READBACK_FAILED);
-      return;
-    }
-
-    DCHECK(view->GetWebContents());
-    content::RenderWidgetHost* rwh = view->GetWebContents()
-                                         ->GetRenderViewHost()
-                                         ->GetWidget();
+    content::RenderWidgetHost* rwh =
+        web_contents->GetRenderViewHost()->GetWidget();
+    DCHECK(rwh);
 
     SkColorType color_type = kN32_SkColorType;
     gfx::Rect src_rect = rwh->GetView()->GetViewBounds();
     gfx::Size dst_size(
         gfx::ScaleToCeiledSize(src_rect.size(), thumbnail_scale_));
     rwh->CopyFromBackingStore(src_rect, dst_size, result_callback, color_type);
   }
 
+  virtual ~TabReadbackRequest() {}
+
   void OnFinishGetTabThumbnailBitmap(const SkBitmap& bitmap,
                                      content::ReadbackResponse response) {
-    DCHECK(!j_content_view_core_.is_null());
     if (response != content::READBACK_SUCCESS || drop_after_readback_) {
       end_callback_.Run(0.f, SkBitmap());
       return;
     }
 
     SkBitmap result_bitmap = bitmap;
     result_bitmap.setImmutable();
     end_callback_.Run(thumbnail_scale_, bitmap);
   }
 
   void SetToDropAfterReadback() { drop_after_readback_ = true; }
 
  private:
-  base::android::ScopedJavaGlobalRef<jobject> j_content_view_core_;
   const float thumbnail_scale_;
   TabReadbackCallback end_callback_;
   bool drop_after_readback_;
 
   base::WeakPtrFactory<TabReadbackRequest> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(TabReadbackRequest);
 };
 
 // static
@@ skipping 107 matching lines @@
 jboolean TabContentManager::HasFullCachedThumbnail(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,
     jint tab_id) {
   return thumbnail_cache_->Get(tab_id, false, false) != nullptr;
 }
 
 void TabContentManager::CacheTab(JNIEnv* env,
                                  const JavaParamRef<jobject>& obj,
                                  const JavaParamRef<jobject>& tab,
-                                 const JavaParamRef<jobject>& content_view_core,
+                                 const JavaParamRef<jobject>& jweb_contents,
                                  jfloat thumbnail_scale) {
   TabAndroid* tab_android = TabAndroid::GetNativeTab(env, tab);
   DCHECK(tab_android);
   int tab_id = tab_android->GetAndroidId();
   GURL url = tab_android->GetURL();
 
-  content::ContentViewCore* view =
-      content::ContentViewCore::GetNativeContentViewCore(env,
-                                                         content_view_core);
+  content::WebContents* web_contents =
+      content::WebContents::FromJavaWebContents(jweb_contents.obj());

[Ted C, 2016/07/25 23:46:54]

not an entirely fair comment since you're just changing the params, but
webcontents is available from tab via web_contents(), can we just use that
instead of passing tab and web contents?

[no sievers, 2016/07/29 20:54:25]

Good point, even better. Done.

+  DCHECK(web_contents);
 
   if (thumbnail_cache_->CheckAndUpdateThumbnailMetaData(tab_id, url)) {
-    if (!view ||
-        !view->GetWebContents()->GetRenderViewHost() ||
-        !view->GetWebContents()->GetRenderViewHost()->GetWidget() ||
-        !view->GetWebContents()
-             ->GetRenderViewHost()
+    if (!web_contents->GetRenderViewHost() ||
+        !web_contents->GetRenderViewHost()->GetWidget() ||
+        !web_contents->GetRenderViewHost()
              ->GetWidget()
              ->CanCopyFromBackingStore() ||
         pending_tab_readbacks_.find(tab_id) != pending_tab_readbacks_.end() ||
         pending_tab_readbacks_.size() >= kMaxReadbacks) {
       thumbnail_cache_->Remove(tab_id);
       return;
     }
 
     TabReadbackCallback readback_done_callback =
         base::Bind(&TabContentManager::PutThumbnailIntoCache,
                    weak_factory_.GetWeakPtr(), tab_id);
-    std::unique_ptr<TabReadbackRequest> readback_request =
-        base::WrapUnique(new TabReadbackRequest(
-            content_view_core, thumbnail_scale, readback_done_callback));
-    pending_tab_readbacks_.set(tab_id, std::move(readback_request));

[Ted C, 2016/07/25 23:46:53]

any idea why it was done this way in the past?  does the in place set change the
destruction ordering (old way was, destroy old one, run new one -- the new code
looks like, run new one, destroy old one)...but I might be way off here.

[no sievers, 2016/07/29 20:54:24]

Yes, true, but no idea why it did it that way. It shouldn't matter since
anything in the map would have already done its calls to kick off the readback,
and also doesn't do anything in the destructor.

-    pending_tab_readbacks_.get(tab_id)->Run();
+    pending_tab_readbacks_.set(
+        tab_id, base::WrapUnique(new TabReadbackRequest(
+                    web_contents, thumbnail_scale, readback_done_callback)));

[Ted C, 2016/07/25 23:46:53]

this indenting looks off, shouldn't it be like it was before?  or is 4 from it's
"logical" parent the correct thing to do in c++ land?

[no sievers, 2016/07/29 20:54:24]

I'm actually not sure, but clang-format did this, so it *should* be legit.
It's possible that both are ok.

   }
 }
 
 void TabContentManager::CacheTabWithBitmap(JNIEnv* env,
                                            const JavaParamRef<jobject>& obj,
                                            const JavaParamRef<jobject>& tab,
                                            const JavaParamRef<jobject>& bitmap,
                                            jfloat thumbnail_scale) {
   TabAndroid* tab_android = TabAndroid::GetNativeTab(env, tab);
   DCHECK(tab_android);
@@ skipping 91 matching lines @@
            jboolean use_approximation_thumbnail) {
   TabContentManager* manager = new TabContentManager(
       env, obj, default_cache_size, approximation_cache_size,
       compression_queue_max_size, write_queue_max_size,
       use_approximation_thumbnail);
   return reinterpret_cast<intptr_t>(manager);
 }
 
 }  // namespace android
 }  // namespace chrome