Removing ContentViewCore dependencies from few functions which acts as direct wrapper to WebContents

content/browser/android/content_view_core_impl.cc

 // Copyright 2012 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 "content/browser/android/content_view_core_impl.h"
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/android/scoped_java_ref.h"
@@ skipping 801 matching lines @@
 }
 
 scoped_refptr<cc::Layer> ContentViewCoreImpl::GetLayer() const {
   return root_layer_.get();
 }
 
 // ----------------------------------------------------------------------------
 // Methods called from Java via JNI
 // ----------------------------------------------------------------------------
 
-void ContentViewCoreImpl::SelectPopupMenuItems(JNIEnv* env, jobject obj,

[Yaron, 2014/08/01 16:21:12]

Sorry but it's unclear why this is moving.

[AKVT, 2014/08/01 17:09:58]

I thought to untouch only RWHVA functions. This function where shall we migrate
? Also one more doubt I have is who will own dpi_scale and other related members
? Please share your view.

[Ted C, 2014/08/01 17:26:20]

The goal of the java WebContents class is to act as a simple bridge to the
native implementation of those methods.  Ideally, the methods on the java side
would have a one to one mapping with those in web_contents.cc.  As this is
simply using the web_contents as a means for getting the RenderViewHost, it does
not belong in the WebContents class.

In cases like this, it might be more appropriate to extract the popup methods
out to a completely separate class that handles all of the interactions and
makes them a single isolated unit that doesn't get muddled with the rest of the
calls.  Granted, that may not be possible due to the message routing etc, but it
would certainly be nice to start building smaller components instead of
monolithic classes.

[AKVT, 2014/08/02 10:03:59]

Thanks Ted. I have removed this function from the patch to take care of only
direct dependent WebContents functions. As discussed will take care of RWHVA
functions to new class and remaining independent functions I will club into one
Class for better restructuring.

-                                               jintArray indices) {
-  RenderViewHostImpl* rvhi = static_cast<RenderViewHostImpl*>(
-      web_contents_->GetRenderViewHost());
-  DCHECK(rvhi);
-  if (indices == NULL) {
-    rvhi->DidCancelPopupMenu();
-    return;
-  }
-
-  int selected_count = env->GetArrayLength(indices);
-  std::vector<int> selected_indices;
-  jint* indices_ptr = env->GetIntArrayElements(indices, NULL);
-  for (int i = 0; i < selected_count; ++i)
-    selected_indices.push_back(indices_ptr[i]);
-  env->ReleaseIntArrayElements(indices, indices_ptr, JNI_ABORT);
-  rvhi->DidSelectPopupMenuItems(selected_indices);
-}
-
 void ContentViewCoreImpl::LoadUrl(
     JNIEnv* env, jobject obj,
     jstring url,
     jint load_url_type,
     jint transition_type,
     jstring j_referrer_url,
     jint referrer_policy,
     jint ua_override_option,
     jstring extra_headers,
     jbyteArray post_data,
@@ skipping 37 matching lines @@
     params.referrer = content::Referrer(
         GURL(ConvertJavaStringToUTF8(env, j_referrer_url)),
         static_cast<blink::WebReferrerPolicy>(referrer_policy));
   }
 
   params.is_renderer_initiated = is_renderer_initiated;
 
   LoadUrl(params);
 }
 
-ScopedJavaLocalRef<jstring> ContentViewCoreImpl::GetURL(
-    JNIEnv* env, jobject) const {
-  return ConvertUTF8ToJavaString(env, GetWebContents()->GetURL().spec());
-}
-
-jboolean ContentViewCoreImpl::IsIncognito(JNIEnv* env, jobject obj) {
-  return GetWebContents()->GetBrowserContext()->IsOffTheRecord();
-}
-
 WebContents* ContentViewCoreImpl::GetWebContents() const {
   return web_contents_;
 }
 
 void ContentViewCoreImpl::SetFocus(JNIEnv* env, jobject obj, jboolean focused) {
   SetFocusInternal(focused);
 }
 
 void ContentViewCoreImpl::SetFocusInternal(bool focused) {
   if (!GetRenderWidgetHostViewAndroid())
@@ skipping 575 matching lines @@
       static_cast<int>(x / dpi_scale()),
       static_cast<int>(y / dpi_scale()),
       static_cast<int>((width > 0 && width < dpi_scale()) ?
           1 : (int)(width / dpi_scale())),
       static_cast<int>((height > 0 && height < dpi_scale()) ?
           1 : (int)(height / dpi_scale())));
   GetWebContents()->Send(new ViewMsg_ExtractSmartClipData(
       GetWebContents()->GetRoutingID(), rect));
 }
 
-void ContentViewCoreImpl::ResumeResponseDeferredAtStart(JNIEnv* env,
-                                                        jobject obj) {
-  static_cast<WebContentsImpl*>(GetWebContents())->
-      ResumeResponseDeferredAtStart();
-}
-
-void ContentViewCoreImpl::SetHasPendingNavigationTransitionForTesting(
-    JNIEnv* env,
-    jobject obj) {
-  RenderFrameHost* frame = static_cast<WebContentsImpl*>(GetWebContents())->
-      GetMainFrame();
-  BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(
-          &TransitionRequestManager::SetHasPendingTransitionRequest,
-          base::Unretained(TransitionRequestManager::GetInstance()),
-          frame->GetProcess()->GetID(),
-          frame->GetRoutingID(),
-          true));
-}
-
 jint ContentViewCoreImpl::GetCurrentRenderProcessId(JNIEnv* env, jobject obj) {
   return GetRenderProcessIdFromRenderViewHost(
       web_contents_->GetRenderViewHost());
 }
 
 void ContentViewCoreImpl::SetBackgroundOpaque(JNIEnv* env, jobject jobj,
     jboolean opaque) {
   if (GetRenderWidgetHostViewAndroid())
     GetRenderWidgetHostViewAndroid()->SetBackgroundOpaque(opaque);
 }
@@ skipping 103 matching lines @@
       reinterpret_cast<ui::WindowAndroid*>(window_android),
       retained_objects_set);
   return reinterpret_cast<intptr_t>(view);
 }
 
 bool RegisterContentViewCore(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 }  // namespace content