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

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java

 // 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.
 
 package org.chromium.content.browser;
 
 import android.annotation.SuppressLint;
 import android.app.Activity;
 import android.app.SearchManager;
 import android.content.ClipboardManager;
@@ skipping 213 matching lines @@
      * extractSmartClipData are available.
      */
     public interface SmartClipDataListener {
         public void onSmartClipDataExtracted(String text, String html, Rect clipRect);
     }
 
     /**
      * An interface that allows the embedder to be notified of navigation transition
      * related events and respond to them.
      */
     public interface NavigationTransitionDelegate {

[shatch, 2014/08/05 14:54:24]

If you're going to move didDeferAfterResponseStarted, etc... to
WebContents.java, shouldn't you also move this delegate there? (and associated
member variable, set function, etc...)

[AKVT, 2014/08/05 16:07:23]

I already moved the interface in WebContents, but since ContentViewCore is the
main interaction point at the moment Chrome App layer may be still relying on
ContentViewcore NavigationTransitionDelegate, so to avoid Compilation issues for
Chrome App, I have kept it here also. If build bot will pass by moving this
completely, then I can remove this. Please share your thoughts on this

[Yaron, 2014/08/06 00:35:45]

I don't see any references to it in chrome/ directory

         /**
          * Called when the navigation is deferred immediately after the response started.
          *
          * @param enteringColor The background color of the entering document, as a String
          *                      representing a legal CSS color value. This is inserted into
          *                      the transition layer's markup after the entering stylesheets
          *                      have been applied.
          */
         public void didDeferAfterResponseStarted(String enteringColor);
 
@@ skipping 111 matching lines @@
     private boolean mTouchScrollInProgress;
 
     // The outstanding fling start events that hasn't got fling end yet. It may be > 1 because
     // onNativeFlingStopped() is called asynchronously.
     private int mPotentiallyActiveFlingCount;
 
     private ViewAndroid mViewAndroid;
 
     private SmartClipDataListener mSmartClipDataListener = null;
 
     private NavigationTransitionDelegate mNavigationTransitionDelegate = null;

[shatch, 2014/08/05 14:54:24]

Move this as well?

[AKVT, 2014/08/05 16:07:23]

Currently I have duplicated by fearing build failure. If I remove this if no
build breaks, then I am happy to remove. Please share your thoughts.

[Yaron, 2014/08/06 00:35:45]

Which build breaks are you referring to? If everything in chromium builds, we
should be fine. YOu can just omit the parameter to ninja to build everything:
ninja -C out/Debug

If it's in the internal repo, don't worry, I'll fix it to work with changes in
this patch when I approve it

 
     // This holds the state of editable text (e.g. contents of <input>, contenteditable) of
     // a focused element.
     // Every time the user, IME, javascript (Blink), autofill etc. modifies the content, the new
     //  state must be reflected to this to keep consistency.
     private final Editable mEditable;
 
     /**
      * PID used to indicate an invalid render process.
      */
@@ skipping 522 matching lines @@
                 params.mBaseUrlForDataUrl,
                 params.mVirtualUrlForDataUrl,
                 params.mCanLoadLocalResources,
                 params.mIsRendererInitiated);
     }
 
     /**
      * Stops loading the current web contents.
      */
     public void stopLoading() {
-        if (mWebContents != null) mWebContents.stop();
+        assert mWebContents != null;
+        mWebContents.stop();
     }
 
     /**
      * Get the URL of the current page.
      *
      * @return The URL of the current page.
      */
     public String getUrl() {
-        if (mNativeContentViewCore != 0) return nativeGetURL(mNativeContentViewCore);
-        return null;
+        assert mWebContents != null;
+        return mWebContents.getUrl();
     }
 
     /**
      * Get the title of the current page.
      *
      * @return The title of the current page.
      */
     public String getTitle() {
-        return mWebContents == null ? null : mWebContents.getTitle();
+        assert mWebContents != null;
+        return mWebContents.getTitle();
     }
 
     /**
      * Shows an interstitial page driven by the passed in delegate.
      *
      * @param url The URL being blocked by the interstitial.
      * @param delegate The delegate handling the interstitial.
      */
     @VisibleForTesting
     public void showInterstitialPage(
@@ skipping 1206 matching lines @@
                 if (getContentViewClient().doesPerformWebSearch()) return true;
                 Intent intent = new Intent(Intent.ACTION_WEB_SEARCH);
                 intent.putExtra(SearchManager.EXTRA_NEW_SEARCH, true);
                 return getContext().getPackageManager().queryIntentActivities(intent,
                         PackageManager.MATCH_DEFAULT_ONLY).size() > 0;
             }
         };
         mActionMode = null;
         // On ICS, startActionMode throws an NPE when getParent() is null.
         if (mContainerView.getParent() != null) {
+            assert mWebContents != null;
             mActionMode = mContainerView.startActionMode(
                     getContentViewClient().getSelectActionModeCallback(getContext(), actionHandler,
-                            nativeIsIncognito(mNativeContentViewCore)));
+                            mWebContents.isIncognito()));
         }
         mUnselectAllOnActionModeDismiss = true;
         if (mActionMode == null) {
             // There is no ActionMode, so remove the selection.
             mImeAdapter.unselect();
         } else {
             getContentViewClient().onContextualActionBarShown();
         }
     }
 
@@ skipping 884 matching lines @@
         mSmartClipDataListener = listener;
     }
 
     public void setBackgroundOpaque(boolean opaque) {
         if (mNativeContentViewCore != 0) {
             nativeSetBackgroundOpaque(mNativeContentViewCore, opaque);
         }
     }
 
     @CalledByNative
-    private void didDeferAfterResponseStarted(String enteringColor) {
-        if (mNavigationTransitionDelegate != null ) {
-            mNavigationTransitionDelegate.didDeferAfterResponseStarted(enteringColor);
-        }
-    }
-
-    @CalledByNative
     public void didStartNavigationTransitionForFrame(long frameId) {

[shatch, 2014/08/05 14:54:24]

Seems like this would need to move as well?

[AKVT, 2014/08/05 16:07:24]

Earlier Yaron suggested to move 2 functions which are dependent on the previous
change, so I have taken care only those 2 methods only. I can move this one as
well, as all are relative functions.

[Yaron, 2014/08/06 00:35:45]

Well logically it doesn't make sense to split these functions up as they control
the same functionality. I agree with Simon. 

         if (mNavigationTransitionDelegate != null ) {
             mNavigationTransitionDelegate.didStartNavigationTransitionForFrame(frameId);
         }
     }
 
-    @CalledByNative
-    private boolean willHandleDeferAfterResponseStarted() {
-        if (mNavigationTransitionDelegate == null) return false;
-        return mNavigationTransitionDelegate.willHandleDeferAfterResponseStarted();
-    }
-
     @VisibleForTesting
     void setHasPendingNavigationTransitionForTesting() {
-        if (mNativeContentViewCore == 0) return;
-        nativeSetHasPendingNavigationTransitionForTesting(mNativeContentViewCore);
+        assert mWebContents != null;
+        mWebContents.setHasPendingNavigationTransitionForTesting();
     }
 
     public void setNavigationTransitionDelegate(NavigationTransitionDelegate delegate) {
         mNavigationTransitionDelegate = delegate;

[shatch, 2014/08/05 14:54:24]

And this?

[AKVT, 2014/08/05 16:07:23]

This too I duplicated as I mentioned on top. Please share your thoughts on this
about build issues.

[Yaron, 2014/08/06 00:35:45]

I don't see the benefit of duplicating this. Please take a step back and
consider that we are moving this functionality out of ContentViewCore into
WebContents.

     }
 
-    @CalledByNative
-    private void addEnteringStylesheetToTransition(String stylesheet) {
-        if (mNavigationTransitionDelegate != null ) {
-            mNavigationTransitionDelegate.addEnteringStylesheetToTransition(stylesheet);
-        }
-    }
-
     /**
      * Offer a long press gesture to the embedding View, primarily for WebView compatibility.
      *
      * @return true if the embedder handled the event.
      */
     private boolean offerLongPressToEmbedder() {
         return mContainerView.performLongClick();
     }
 
     /**
@@ skipping 35 matching lines @@
         mPotentiallyActiveFlingCount--;
         updateGestureStateListener(GestureEventType.FLING_END);
     }
 
     @Override
     public void onScreenOrientationChanged(int orientation) {
         sendOrientationChangeEvent(orientation);
     }
 
     public void resumeResponseDeferredAtStart() {
-        if (mNativeContentViewCore == 0) return;
-        nativeResumeResponseDeferredAtStart(mNativeContentViewCore);
+        assert mWebContents != null;
+        mWebContents.resumeResponseDeferredAtStart();
     }
 
     /**
      * Set whether the ContentViewCore requires the WebContents to be fullscreen in order to lock
      * the screen orientation.
      */
     public void setFullscreenRequiredForOrientationLock(boolean value) {
         mFullscreenRequiredForOrientationLock = value;
     }
 
@@ skipping 14 matching lines @@
             String referrerUrl,
             int referrerPolicy,
             int uaOverrideOption,
             String extraHeaders,
             byte[] postData,
             String baseUrlForDataUrl,
             String virtualUrlForDataUrl,
             boolean canLoadLocalResources,
             boolean isRendererInitiated);
 
-    private native String nativeGetURL(long nativeContentViewCoreImpl);
-
-    private native boolean nativeIsIncognito(long nativeContentViewCoreImpl);
-
     private native void nativeSetFocus(long nativeContentViewCoreImpl, boolean focused);
 
     private native void nativeSendOrientationChangeEvent(
             long nativeContentViewCoreImpl, int orientation);
 
     // All touch events (including flings, scrolls etc) accept coordinates in physical pixels.
     private native boolean nativeOnTouchEvent(
             long nativeContentViewCoreImpl, MotionEvent event,
             long timeMs, int action, int pointerCount, int historySize, int actionIndex,
             float x0, float y0, float x1, float y1,
@@ skipping 42 matching lines @@
             float anchorX, float anchorY, float deltaScale);
 
     private native void nativeSelectBetweenCoordinates(
             long nativeContentViewCoreImpl, float x1, float y1, float x2, float y2);
 
     private native void nativeMoveCaret(long nativeContentViewCoreImpl, float x, float y);
 
     private native void nativeHideTextHandles(long nativeContentViewCoreImpl);
 
     private native void nativeResetGestureDetection(long nativeContentViewCoreImpl);
+
     private native void nativeSetDoubleTapSupportEnabled(
             long nativeContentViewCoreImpl, boolean enabled);
+
     private native void nativeSetMultiTouchZoomSupportEnabled(
             long nativeContentViewCoreImpl, boolean enabled);
 
     private native void nativeSelectPopupMenuItems(long nativeContentViewCoreImpl, int[] indices);
 
     private native void nativeClearHistory(long nativeContentViewCoreImpl);
 
     private native void nativeEvaluateJavaScript(long nativeContentViewCoreImpl,
             String script, JavaScriptCallback callback, boolean startRenderer);
 
     private native void nativePostMessageToFrame(long nativeContentViewCoreImpl, String frameId,
             String message, String sourceOrigin, String targetOrigin);
 
     private native long nativeGetNativeImeAdapter(long nativeContentViewCoreImpl);
 
     private native int nativeGetCurrentRenderProcessId(long nativeContentViewCoreImpl);
 
     private native void nativeSetUseDesktopUserAgent(long nativeContentViewCoreImpl,
             boolean enabled, boolean reloadOnChange);
+
     private native boolean nativeGetUseDesktopUserAgent(long nativeContentViewCoreImpl);
 
     private native void nativeClearSslPreferences(long nativeContentViewCoreImpl);
 
     private native void nativeSetAllowJavascriptInterfacesInspection(
             long nativeContentViewCoreImpl, boolean allow);
 
     private native void nativeAddJavascriptInterface(long nativeContentViewCoreImpl, Object object,
             String name, Class requiredAnnotation);
 
     private native void nativeRemoveJavascriptInterface(long nativeContentViewCoreImpl,
             String name);
 
     private native int nativeGetNavigationHistory(long nativeContentViewCoreImpl, Object context);
+
     private native void nativeGetDirectedNavigationHistory(long nativeContentViewCoreImpl,
             Object context, boolean isForward, int maxEntries);
+
     private native String nativeGetOriginalUrlForActiveNavigationEntry(
             long nativeContentViewCoreImpl);
 
     private native void nativeWasResized(long nativeContentViewCoreImpl);
 
     private native void nativeSetAccessibilityEnabled(
             long nativeContentViewCoreImpl, boolean enabled);
 
     private native void nativeExtractSmartClipData(long nativeContentViewCoreImpl,
             int x, int y, int w, int h);
+
     private native void nativeSetBackgroundOpaque(long nativeContentViewCoreImpl, boolean opaque);
-
-    private native void nativeResumeResponseDeferredAtStart(
-            long nativeContentViewCoreImpl);
-    private native void nativeSetHasPendingNavigationTransitionForTesting(
-            long nativeContentViewCoreImpl);
 }