PlzNavigate: Chrome UI changes for new methods of WebContentsObserver

content/browser/android/web_contents_observer_proxy.cc

Index: content/browser/android/web_contents_observer_proxy.cc
diff --git a/content/browser/android/web_contents_observer_proxy.cc b/content/browser/android/web_contents_observer_proxy.cc
index bc068304e2716d0a6de2ce45d4bb99e0aab49275..74aef9ae78eb0cfc4ddbd056a5409b7f085a2b76 100644
--- a/content/browser/android/web_contents_observer_proxy.cc
+++ b/content/browser/android/web_contents_observer_proxy.cc
@@ -104,71 +104,14 @@ void WebContentsObserverProxy::DidStopLoading() {
   Java_WebContentsObserverProxy_didStopLoading(env, obj, jstring_url);
 }
 
-void WebContentsObserverProxy::DidFailProvisionalLoad(
-    RenderFrameHost* render_frame_host,
-    const GURL& validated_url,
-    int error_code,
-    const base::string16& error_description,
-    bool was_ignored_by_handler) {
-  DidFailLoadInternal(true, !render_frame_host->GetParent(), error_code,
-                      error_description, validated_url, was_ignored_by_handler);
-}
-
 void WebContentsObserverProxy::DidFailLoad(
     RenderFrameHost* render_frame_host,
     const GURL& validated_url,
     int error_code,
     const base::string16& error_description,
     bool was_ignored_by_handler) {
-  DidFailLoadInternal(false, !render_frame_host->GetParent(), error_code,
-                      error_description, validated_url, was_ignored_by_handler);
-}
-
-void WebContentsObserverProxy::DidNavigateMainFrame(
-    const LoadCommittedDetails& details,
-    const FrameNavigateParams& params) {
-  JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jobject> obj(java_observer_);
-  ScopedJavaLocalRef<jstring> jstring_url(
-      ConvertUTF8ToJavaString(env, params.url.spec()));
-  ScopedJavaLocalRef<jstring> jstring_base_url(
-      ConvertUTF8ToJavaString(env, params.base_url.spec()));
-
-  // See http://crbug.com/251330 for why it's determined this way.
-  url::Replacements<char> replacements;
-  replacements.ClearRef();
-  bool urls_same_ignoring_fragment =
-      params.url.ReplaceComponents(replacements) ==
-      details.previous_url.ReplaceComponents(replacements);
-
-  // is_fragment_navigation is indicative of the intent of this variable.
-  // However, there isn't sufficient information here to determine whether this
-  // is actually a fragment navigation, or a history API navigation to a URL
-  // that would also be valid for a fragment navigation.
-  bool is_fragment_navigation =
-      urls_same_ignoring_fragment && details.is_in_page;
-
-  Java_WebContentsObserverProxy_didNavigateMainFrame(
-      env, obj, jstring_url, jstring_base_url,
-      details.is_navigation_to_different_page(), is_fragment_navigation,
-      details.http_status_code);
-}
-
-void WebContentsObserverProxy::DidNavigateAnyFrame(
-    RenderFrameHost* render_frame_host,
-    const LoadCommittedDetails& details,
-    const FrameNavigateParams& params) {
-  JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jobject> obj(java_observer_);
-  ScopedJavaLocalRef<jstring> jstring_url(
-      ConvertUTF8ToJavaString(env, params.url.spec()));
-  ScopedJavaLocalRef<jstring> jstring_base_url(
-      ConvertUTF8ToJavaString(env, params.base_url.spec()));
-  jboolean jboolean_is_reload = ui::PageTransitionCoreTypeIs(
-      params.transition, ui::PAGE_TRANSITION_RELOAD);
-
-  Java_WebContentsObserverProxy_didNavigateAnyFrame(
-      env, obj, jstring_url, jstring_base_url, jboolean_is_reload);
+  DidFailLoadInternal(!render_frame_host->GetParent(), error_code,

[Ted C, 2017/01/31 18:57:48]

let's delete DidFailLoadInternal and inline it here as it should only be the one
caller now right?

[shaktisahu, 2017/02/03 06:56:17]

Done.

+                      error_description, validated_url);
 }
 
 void WebContentsObserverProxy::DocumentAvailableInMainFrame() {
@@ -177,37 +120,6 @@ void WebContentsObserverProxy::DocumentAvailableInMainFrame() {
   Java_WebContentsObserverProxy_documentAvailableInMainFrame(env, obj);
 }
 
-void WebContentsObserverProxy::DidStartProvisionalLoadForFrame(
-    RenderFrameHost* render_frame_host,
-    const GURL& validated_url,
-    bool is_error_page) {
-  JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jobject> obj(java_observer_);
-  ScopedJavaLocalRef<jstring> jstring_url(
-      ConvertUTF8ToJavaString(env, validated_url.spec()));
-  // TODO(dcheng): Does Java really need the parent frame ID? It doesn't appear
-  // to be used at all, and it just adds complexity here.
-  Java_WebContentsObserverProxy_didStartProvisionalLoadForFrame(
-      env, obj, render_frame_host->GetRoutingID(),
-      render_frame_host->GetParent()
-          ? render_frame_host->GetParent()->GetRoutingID()
-          : -1,
-      !render_frame_host->GetParent(), jstring_url, is_error_page);
-}
-
-void WebContentsObserverProxy::DidCommitProvisionalLoadForFrame(
-    RenderFrameHost* render_frame_host,
-    const GURL& url,
-    ui::PageTransition transition_type) {
-  JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jobject> obj(java_observer_);
-  ScopedJavaLocalRef<jstring> jstring_url(
-      ConvertUTF8ToJavaString(env, url.spec()));
-  Java_WebContentsObserverProxy_didCommitProvisionalLoadForFrame(
-      env, obj, render_frame_host->GetRoutingID(),
-      !render_frame_host->GetParent(), jstring_url, transition_type);
-}
-
 void WebContentsObserverProxy::DidStartNavigation(
     NavigationHandle* navigation_handle) {
   JNIEnv* env = AttachCurrentThread();
@@ -226,13 +138,29 @@ void WebContentsObserverProxy::DidFinishNavigation(
   ScopedJavaLocalRef<jstring> jstring_url(
       ConvertUTF8ToJavaString(env, navigation_handle->GetURL().spec()));
 
+  CR_DEFINE_STATIC_LOCAL(GURL, last_committed_url, (GURL()));

[Ted C, 2017/01/31 18:57:48]

Hmm...I'd ask clamy@ or jam@ whether there is a way to get this information from
NavigationHandle (likely transitively).  I'm curious if we look at how the
previous LoadCommittedDetails was being generated to see if we can pull the
previous_url here in the same way.

[shaktisahu, 2017/02/03 06:56:17]

I checked with jam@ and they will be adding a getter in NavigationHandle to get
the previous URL. I will wait for that change.

+
+  url::Replacements<char> replacements;
+  replacements.ClearRef();
+  bool urls_same_ignoring_fragment =
+      navigation_handle->GetURL().ReplaceComponents(replacements) ==
+      last_committed_url.ReplaceComponents(replacements);
+
+  last_committed_url = navigation_handle->GetURL();
+
+  bool is_fragment_navigation =
+      urls_same_ignoring_fragment && navigation_handle->IsSamePage();

[shaktisahu, 2017/01/31 01:57:53]

This is the only param so far that didn't come from NavigationHandle class which
implies if we reorganize the params into a Java NavigationHandle class, this
would still be needed to be passed separately. 

[Ted C, 2017/01/31 18:57:48]

As far as I can tell, this seems to only be used in WebView.  If needed, I
wonder if we could push this caching to them.

+
   Java_WebContentsObserverProxy_didFinishNavigation(
       env, obj, jstring_url, navigation_handle->IsInMainFrame(),
       navigation_handle->IsErrorPage(), navigation_handle->HasCommitted(),
-      navigation_handle->IsSamePage(),
+      navigation_handle->IsSamePage(), is_fragment_navigation,
       navigation_handle->HasCommitted() ? navigation_handle->GetPageTransition()
                                         : -1,
-      navigation_handle->GetNetErrorCode());
+      navigation_handle->GetNetErrorCode(),
+      navigation_handle->GetResponseHeaders()
+          ? navigation_handle->GetResponseHeaders()->response_code()
+          : 200);

[shaktisahu, 2017/01/31 01:57:53]

May be we need a better default value other than 200. I tried using 0, but for
some tests (ConextualSearchManager#testTapContentVisibility and few more), it
gets flaky when the device is running in offline mode. 

[Ted C, 2017/01/31 18:57:48]

I think we likely want a unset value here much like -1/null for page transition.
 These are only valid if the page has committed right?

I do think 200 implies success when it is not appropriate.  How do the
contextual search tests fail?

[shaktisahu, 2017/02/03 06:56:17]

The test fails only for Nexus 5 whenever the device goes offline. I think the
contextual search tests depend on the internet connectivity, which causes this
flakiness. 
Prior to this CL, in didNavigateMainFrame, even though an error occurs, we would
still get 200 since the error page loads successfully.
But now since we switched to didFinishNavigation, that is not the case. If an
error occurs, then navigation_handle->GetResponseHeaders() will return null. It
doesn't fail all the time though in offline mode.
I think I should make this default value to 200 and file a bug to fix those 5
test cases. Let me know.

[Ted C, 2017/02/08 05:31:15]

File a bug, and put a TODO above this line with a reference to the bug number.

 }
 
 void WebContentsObserverProxy::DidFinishLoad(RenderFrameHost* render_frame_host,
@@ -285,12 +213,10 @@ void WebContentsObserverProxy::DidChangeThemeColor(SkColor color) {
 }
 
 void WebContentsObserverProxy::DidFailLoadInternal(
-    bool is_provisional_load,
     bool is_main_frame,
     int error_code,
     const base::string16& description,
-    const GURL& url,
-    bool was_ignored_by_handler) {
+    const GURL& url) {
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobject> obj(java_observer_);
   ScopedJavaLocalRef<jstring> jstring_error_description(
@@ -298,9 +224,9 @@ void WebContentsObserverProxy::DidFailLoadInternal(
   ScopedJavaLocalRef<jstring> jstring_url(
       ConvertUTF8ToJavaString(env, url.spec()));
 
-  Java_WebContentsObserverProxy_didFailLoad(
-      env, obj, is_provisional_load, is_main_frame, error_code,
-      jstring_error_description, jstring_url, was_ignored_by_handler);
+  Java_WebContentsObserverProxy_didFailLoad(env, obj, is_main_frame, error_code,
+                                            jstring_error_description,
+                                            jstring_url);
 }
 
 void WebContentsObserverProxy::DidFirstVisuallyNonEmptyPaint() {
@@ -331,18 +257,6 @@ void WebContentsObserverProxy::TitleWasSet(NavigationEntry* entry,
   Java_WebContentsObserverProxy_titleWasSet(env, obj, jstring_title);
 }
 
-void WebContentsObserverProxy::DidStartNavigationToPendingEntry(
-    const GURL& url,
-    ReloadType reload_type) {
-  JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jobject> obj(java_observer_);
-  ScopedJavaLocalRef<jstring> jstring_url(
-      ConvertUTF8ToJavaString(env, url.spec()));
-
-  Java_WebContentsObserverProxy_didStartNavigationToPendingEntry(env, obj,
-                                                                 jstring_url);
-}
-
 void WebContentsObserverProxy::SetToBaseURLForDataURLIfNeeded(
     std::string* url) {
   NavigationEntry* entry =