Update up error page comments and clean up code

Bug: 991608
Change-Id: If185af4f57f6c065373f494387121895b0dbe461
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1760728
Commit-Queue: Yi Su <mrsuyi@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688459}

ios/web/navigation/crw_wk_navigation_handler.mm

@@ -1726,7 +1726,7 @@ void ReportOutOfSyncURLInDidStartProvisionalNavigation(
           net::GURLWithNSURL(webView.URL), errorURL);
     } else {
       command = item->error_retry_state_machine().DidFailNavigation(
-          net::GURLWithNSURL(webView.URL), errorURL);
+          net::GURLWithNSURL(webView.URL));
     }
     [self handleErrorRetryCommand:command
                    navigationItem:item

ios/web/navigation/error_retry_state_machine.h

@@ -9,11 +9,11 @@
 
 namespace web {
 
-// Defines the states of a NavigationItem that failed to load. This is used by
-// CRWWebController to coordinate the display of native error views such that
-// back/forward navigation to a native error view automatically triggers a
-// reload of the original URL. This is achieved in four steps:
-// 1) A NavigationItem is set to kDisplayingError when it first failed to load
+// Defines the states of a NavigationItem that failed to load. This is used to
+// coordinate the display of error page such that back/forward/reload navigation
+// to a loaded error page will load the original URL. This is achieved in four
+// steps:
+// 1) A NavigationItem is set to gkDisplayingError when it first failed to load
 //    and a web error is displayed. If the failure occurred during provisional
 //    navigation, a placeholder entry is inserted into WKBackForwardList for
 //    this item.
@@ -25,7 +25,8 @@ namespace web {
 //    kRetryFailedNavigationItem.
 // 4) Upon completion of the reload, if successful, the item state is changed to
 //    kNoNavigationError.
-// See https://bit.ly/2sxWJgs for the full state transition diagram.
+// Full state transition diagram:
+// https://docs.google.com/spreadsheets/d/1AdwRIuCShbRy4gEFB-SxaupmPmmO2MO7oLZ4F87XjRE/edit?usp=sharing
 enum class ErrorRetryState {
   // This is a new navigation request.
   kNewRequest,
@@ -97,8 +98,7 @@ class ErrorRetryStateMachine {
                                                  const GURL& error_url);
 
   // Runs state transitions upon a failure after the navigation is committed.
-  ErrorRetryCommand DidFailNavigation(const GURL& web_view_url,
-                                      const GURL& error_url);
+  ErrorRetryCommand DidFailNavigation(const GURL& web_view_url);
 
   // Runs state transitions upon a successful navigation.
   ErrorRetryCommand DidFinishNavigation(const GURL& web_view_url);

ios/web/navigation/error_retry_state_machine.mm

@@ -13,6 +13,12 @@
 
 namespace web {
 
+using wk_navigation_util::CreatePlaceholderUrlForUrl;
+using wk_navigation_util::CreateRedirectUrl;
+using wk_navigation_util::IsPlaceholderUrl;
+using wk_navigation_util::IsRestoreSessionUrl;
+using wk_navigation_util::ExtractTargetURL;
+
 ErrorRetryStateMachine::ErrorRetryStateMachine()
     : state_(ErrorRetryState::kNewRequest) {}
 
@@ -59,7 +65,7 @@ ErrorRetryCommand ErrorRetryStateMachine::DidFailProvisionalNavigation(
     const GURL& error_url) {
   switch (state_) {
     case ErrorRetryState::kNewRequest:
-      if (web_view_url == wk_navigation_util::CreateRedirectUrl(error_url)) {
+      if (web_view_url == CreateRedirectUrl(error_url)) {
         // Client redirect in restore_session.html failed. A placeholder is not
         // needed here because a back/forward item already exists for
         // restore_session.html.
@@ -100,8 +106,7 @@ ErrorRetryCommand ErrorRetryStateMachine::DidFailProvisionalNavigation(
 }
 
 ErrorRetryCommand ErrorRetryStateMachine::DidFailNavigation(
-    const GURL& web_view_url,
-    const GURL& error_url) {
+    const GURL& web_view_url) {
   switch (state_) {
     case ErrorRetryState::kNewRequest:
       state_ = ErrorRetryState::kReadyToDisplayError;
@@ -130,17 +135,15 @@ ErrorRetryCommand ErrorRetryStateMachine::DidFinishNavigation(
   switch (state_) {
     case ErrorRetryState::kLoadingPlaceholder:
       // (1) Placeholder load for initial failure succeeded.
-      DCHECK_EQ(web_view_url,
-                wk_navigation_util::CreatePlaceholderUrlForUrl(url_));
+      DCHECK_EQ(web_view_url, CreatePlaceholderUrlForUrl(url_));
       state_ = ErrorRetryState::kReadyToDisplayError;
       return ErrorRetryCommand::kLoadError;
 
     case ErrorRetryState::kRetryPlaceholderNavigation:
-      if (wk_navigation_util::IsPlaceholderUrl(web_view_url)) {
+      if (IsPlaceholderUrl(web_view_url)) {
         // (11) Explicitly keep the state the same so after rewriting to the non
         // placeholder url the else block will trigger.
-        DCHECK_EQ(web_view_url,
-                  wk_navigation_util::CreatePlaceholderUrlForUrl(url_));
+        DCHECK_EQ(web_view_url, CreatePlaceholderUrlForUrl(url_));
         state_ = ErrorRetryState::kRetryPlaceholderNavigation;
         return ErrorRetryCommand::kRewriteToWebViewURL;
       } else {
@@ -150,7 +153,7 @@ ErrorRetryCommand ErrorRetryStateMachine::DidFinishNavigation(
         return ErrorRetryCommand::kLoadError;
       }
     case ErrorRetryState::kNewRequest:
-      if (wk_navigation_util::IsRestoreSessionUrl(web_view_url)) {
+      if (IsRestoreSessionUrl(web_view_url)) {
         // (8) Initial load of restore_session.html. Don't change state or
         // issue command. Wait for the client-side redirect.
       } else {
@@ -166,18 +169,16 @@ ErrorRetryCommand ErrorRetryStateMachine::DidFinishNavigation(
       break;
 
     case ErrorRetryState::kDisplayingError:
-      if (web_view_url ==
-          wk_navigation_util::CreatePlaceholderUrlForUrl(url_)) {
+      if (web_view_url == CreatePlaceholderUrlForUrl(url_)) {
         // (4) Back/forward to or reload of placeholder URL. Rewrite WebView URL
         // to prepare for retry.
         state_ = ErrorRetryState::kNavigatingToFailedNavigationItem;
         return ErrorRetryCommand::kRewriteToWebViewURL;
       }
 
-      if (wk_navigation_util::IsRestoreSessionUrl(web_view_url)) {
+      if (IsRestoreSessionUrl(web_view_url)) {
         GURL target_url;
-        if (wk_navigation_util::ExtractTargetURL(web_view_url, &target_url) &&
-            target_url == url_) {
+        if (ExtractTargetURL(web_view_url, &target_url) && target_url == url_) {
           // (10) Back/forward navigation to a restored session entry in offline
           // mode. It is OK to consider this load succeeded for now because the
           // failure delegate will be triggered again if the load fails.

ios/web/navigation/error_retry_state_machine_unittest.mm

@@ -80,8 +80,7 @@ TEST_F(ErrorRetryStateMachineTest, OfflineThenReload) {
   // Reload fails after navigation is committed.
   {
     ErrorRetryStateMachine clone(machine);
-    ASSERT_EQ(ErrorRetryCommand::kLoadError,
-              clone.DidFailNavigation(test_url, test_url));
+    ASSERT_EQ(ErrorRetryCommand::kLoadError, clone.DidFailNavigation(test_url));
     ASSERT_EQ(ErrorRetryState::kReadyToDisplayError, clone.state());
   }
 }
@@ -145,8 +144,7 @@ TEST_F(ErrorRetryStateMachineTest, WebErrorPageThenReload) {
   // Reload fails after navigation is committed.
   {
     ErrorRetryStateMachine clone(machine);
-    ASSERT_EQ(ErrorRetryCommand::kLoadError,
-              clone.DidFailNavigation(test_url, test_url));
+    ASSERT_EQ(ErrorRetryCommand::kLoadError, clone.DidFailNavigation(test_url));
     ASSERT_EQ(ErrorRetryState::kReadyToDisplayError, clone.state());
   }
 
@@ -169,7 +167,7 @@ TEST_F(ErrorRetryStateMachineTest, LoadFailedAfterCommit) {
   machine.SetURL(test_url);
 
   ASSERT_EQ(ErrorRetryCommand::kLoadError,
-            machine.DidFailNavigation(GURL::EmptyGURL(), test_url));
+            machine.DidFailNavigation(GURL::EmptyGURL()));
   ASSERT_EQ(ErrorRetryState::kReadyToDisplayError, machine.state());
 }
 
@@ -211,8 +209,7 @@ TEST_F(ErrorRetryStateMachineTest, SuccessThenReloadOffline) {
   // Reload fails after commit.
   {
     ErrorRetryStateMachine clone(machine);
-    ASSERT_EQ(ErrorRetryCommand::kLoadError,
-              clone.DidFailNavigation(test_url, test_url));
+    ASSERT_EQ(ErrorRetryCommand::kLoadError, clone.DidFailNavigation(test_url));
     ASSERT_EQ(ErrorRetryState::kReadyToDisplayError, clone.state());
   }
 }