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..524d1c49939f8103df859642c292050886f7675a 100644
--- a/content/browser/android/web_contents_observer_proxy.cc
+++ b/content/browser/android/web_contents_observer_proxy.cc
@@ -104,71 +104,22 @@ 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_error_description(
+      ConvertUTF16ToJavaString(env, error_description));
   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);
-}
+      ConvertUTF8ToJavaString(env, validated_url.spec()));
 
-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);
+  Java_WebContentsObserverProxy_didFailLoad(
+      env, obj, !render_frame_host->GetParent(), error_code,
+      jstring_error_description, jstring_url);
 }
 
 void WebContentsObserverProxy::DocumentAvailableInMainFrame() {
@@ -177,37 +128,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();
@@ -216,7 +136,7 @@ void WebContentsObserverProxy::DidStartNavigation(
       ConvertUTF8ToJavaString(env, navigation_handle->GetURL().spec()));
   Java_WebContentsObserverProxy_didStartNavigation(
       env, obj, jstring_url, navigation_handle->IsInMainFrame(),
-      navigation_handle->IsErrorPage());
+      navigation_handle->IsSamePage(), navigation_handle->IsErrorPage());
 }
 
 void WebContentsObserverProxy::DidFinishNavigation(
@@ -226,13 +146,28 @@ void WebContentsObserverProxy::DidFinishNavigation(
   ScopedJavaLocalRef<jstring> jstring_url(
       ConvertUTF8ToJavaString(env, navigation_handle->GetURL().spec()));
 
+  bool is_fragment_navigation = navigation_handle->IsSamePage();
+
+  if (navigation_handle->HasCommitted()) {

[boliu, 2017/02/07 18:38:18]

is it safe to skip this check if HasCommitted is not true?

[shaktisahu, 2017/02/07 20:09:49]

Yes, if the HasCommitted is false, then no successful navigation happened, not
even a successful error page. User was left in the previous page. So in that
case, we don't need to worry about if it was for a fragment navigation.

[boliu, 2017/02/07 22:08:23]

What does IsSamePage return in that case? Always true because navigation didn't
happen, or depends on where the intended destimation of failed navigation?

Whatever the case, probably worth spelling it out in the documentation on
WebContentsObserver

[shaktisahu, 2017/02/08 00:05:07]

IsSamePage is set to false for the navigations that don't commit. 

[boliu, 2017/02/08 00:10:38]

I'm guessing NetError and response code are gonna be "normal", or somewhat
meaningless as well? Please put that in a comment on WebContentsObserver.java

[shaktisahu, 2017/02/08 00:51:09]

Done.
NetError will still have the meaningful error code obtained from the net layer.

+    // See http://crbug.com/251330 for why it's determined this way.
+    url::Replacements<char> replacements;
+    replacements.ClearRef();
+    bool urls_same_ignoring_fragment =
+        navigation_handle->GetURL().ReplaceComponents(replacements) ==
+        navigation_handle->GetPreviousURL().ReplaceComponents(replacements);
+    is_fragment_navigation &= urls_same_ignoring_fragment;
+  }
+
   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);
 }
 
 void WebContentsObserverProxy::DidFinishLoad(RenderFrameHost* render_frame_host,
@@ -284,25 +219,6 @@ void WebContentsObserverProxy::DidChangeThemeColor(SkColor color) {
   Java_WebContentsObserverProxy_didChangeThemeColor(env, obj, 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) {
-  JNIEnv* env = AttachCurrentThread();
-  ScopedJavaLocalRef<jobject> obj(java_observer_);
-  ScopedJavaLocalRef<jstring> jstring_error_description(
-      ConvertUTF16ToJavaString(env, description));
-  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);
-}
-
 void WebContentsObserverProxy::DidFirstVisuallyNonEmptyPaint() {
   JNIEnv* env = AttachCurrentThread();
   ScopedJavaLocalRef<jobject> obj(java_observer_);
@@ -331,18 +247,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 =