Look up for pending navigation contexts instead of creating new ones.

ios/web/web_state/ui/crw_web_controller.mm

 // 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.
 
 #import "ios/web/web_state/ui/crw_web_controller.h"
 
 #import <WebKit/WebKit.h>
 
 #import <objc/runtime.h>
 #include <stddef.h>
@@ skipping 806 matching lines @@
 // Called when WKWebView URL has been changed.
 - (void)webViewURLDidChange;
 // Returns YES if a KVO change to |newURL| could be a 'navigation' within the
 // document (hash change, pushState/replaceState, etc.). This should only be
 // used in the context of a URL KVO callback firing, and only if |isLoading| is
 // YES for the web view (since if it's not, no guesswork is needed).
 - (BOOL)isKVOChangePotentialSameDocumentNavigationToURL:(const GURL&)newURL;
 // Called when a non-document-changing URL change occurs. Updates the
 // _documentURL, and informs the superclass of the change.
 - (void)URLDidChangeWithoutDocumentChange:(const GURL&)URL;
-// Returns YES if there is currently a requested but uncommitted load for
-// |targetURL|.
-- (BOOL)isLoadRequestPendingForURL:(const GURL&)targetURL;
+// Returns context for pending navigation that has |URL|. null if there is no
+// matching pending navigation.
+- (web::NavigationContextImpl*)contextForPendingNavigationWithURL:
+    (const GURL&)URL;
 // Loads request for the URL of the current navigation item. Subclasses may
 // choose to build a new NSURLRequest and call |loadRequest| on the underlying
 // web view, or use native web view navigation where possible (for example,
 // going back and forward through the history stack).
 - (void)loadRequestForCurrentNavigationItem;
 // Reports Navigation.IOSWKWebViewSlowFastBackForward UMA. No-op if pending
 // navigation is not back forward navigation.
 - (void)reportBackForwardNavigationTypeForFastNavigation:(BOOL)isFast;
 
 // Handlers for JavaScript messages. |message| contains a JavaScript command and
@@ skipping 3291 matching lines @@
 
   // Web View should not be created for App Specific URLs.
   if (!web::GetWebClient()->IsAppSpecificURL(URL)) {
     [self ensureWebViewCreated];
     DCHECK(_webView) << "_webView null while trying to load HTML";
   }
   WKNavigation* navigation =
       [_webView loadHTMLString:HTML baseURL:net::NSURLWithGURL(URL)];
   [_navigationStates setState:web::WKNavigationState::REQUESTED
                 forNavigation:navigation];
-  std::unique_ptr<web::NavigationContextImpl> context =
-      web::NavigationContextImpl::CreateNavigationContext(_webStateImpl, URL);
+  std::unique_ptr<web::NavigationContextImpl> context;
+  if (_webStateImpl->HasWebUI()) {
+    // WebUI uses |loadHTML:forURL:| to feed the content to web view. This
+    // should not be treated as a navigation, but WKNavigationDelegate callbacks
+    // still expect a valid context.
+    context =
+        web::NavigationContextImpl::CreateNavigationContext(_webStateImpl, URL);
+  } else {
+    context = [self
+        registerLoadRequestForURL:URL
+                         referrer:web::Referrer()
+                       transition:ui::PageTransition::PAGE_TRANSITION_TYPED];
+  }
   [_navigationStates setContext:std::move(context) forNavigation:navigation];
 }
 
 - (void)loadHTML:(NSString*)HTML forAppSpecificURL:(const GURL&)URL {
   CHECK(web::GetWebClient()->IsAppSpecificURL(URL));
   [self loadHTML:HTML forURL:URL];
 }
 
 - (void)loadHTMLForCurrentURL:(NSString*)HTML {
   [self loadHTML:HTML forURL:self.currentURL];
@@ skipping 359 matching lines @@
   [self setDocumentURL:net::GURLWithNSURL([_webView URL])];
 
   if (!_lastRegisteredRequestURL.is_valid() &&
       _documentURL != _lastRegisteredRequestURL) {
     // if |_lastRegisteredRequestURL| is an invalid URL, then |_documentURL|
     // will be "about:blank".
     [[self sessionController] updatePendingItem:_documentURL];
   }
 
   // If |navigation| is nil (which happens for windows open by DOM), then it
-  // should be the first and the only pending navigaiton.
+  // should be the first and the only pending navigation.
   BOOL isLastNavigation =
       !navigation ||
       [[_navigationStates lastAddedNavigation] isEqual:navigation];
   DCHECK(_documentURL == _lastRegisteredRequestURL ||  // latest navigation
          !isLastNavigation ||                          // previous navigation
          (!_lastRegisteredRequestURL.is_valid() &&     // invalid URL load
           _documentURL.spec() == url::kAboutBlankURL));
 
   // Update HTTP response headers.
   _webStateImpl->UpdateHttpResponseHeaders(_documentURL);
@@ skipping 231 matching lines @@
     // A fast back navigation caused by pushState does not call
     // |didCommitNavigation:|, so signal page change explicitly.
     // TODO(crbug.com/659816): Because back-forward navigation is disabled for
     // pages that used push/replace State API, this code will not be executed.
     DCHECK_EQ(_documentURL.GetOrigin(), webViewURL.GetOrigin());
     BOOL isSameDocumentNavigation =
         [self isKVOChangePotentialSameDocumentNavigationToURL:webViewURL];
     [self setDocumentURL:webViewURL];
     [self webPageChanged];
 
+    web::NavigationContextImpl* context =
+        [self contextForPendingNavigationWithURL:webViewURL];
     // Same document navigation does not contain response headers.
-    // TODO(crbug.com/713836): Instead of creating a new context, find
-    // existing navigation context stored in |_navigationStates| and use it.
-    std::unique_ptr<web::NavigationContextImpl> context =
-        web::NavigationContextImpl::CreateNavigationContext(_webStateImpl,
-                                                            webViewURL);
     net::HttpResponseHeaders* headers =
         isSameDocumentNavigation ? nullptr
                                  : _webStateImpl->GetHttpResponseHeaders();
     context->SetResponseHeaders(headers);
     context->SetIsSameDocument(isSameDocumentNavigation);
-    _webStateImpl->OnNavigationFinished(context.get());
+    _webStateImpl->OnNavigationFinished(context);
   }
 
   [self updateSSLStatusForCurrentNavigationItem];
 
   // Fast back forward navigation may not call |didFinishNavigation:|, so
   // signal did finish navigation explicitly.
   if (_lastRegisteredRequestURL == _documentURL) {
     [self didFinishNavigation:nil];
   }
 }
@@ skipping 126 matching lines @@
   // history changes when a window.history.didPushState or
   // window.history.didReplaceState message is received, which should happen in
   // the next runloop.
   //
   // Otherwise, simulate the whole delegate flow for a load (since the
   // superclass currently doesn't have a clean separation between URL changes
   // and document changes). Note that the order of these calls is important:
   // registering a load request logically comes before updating the document
   // URL, but also must come first since it uses state that is reset on URL
   // changes.
-  std::unique_ptr<web::NavigationContextImpl> navigationContext;
+
+  // |newNavigationContext| only exists if this method has to create a new
+  // context object.
+  std::unique_ptr<web::NavigationContextImpl> newNavigationContext;
   if (!_changingHistoryState) {
-    if ([self isLoadRequestPendingForURL:newURL]) {
+    if ([self contextForPendingNavigationWithURL:newURL]) {
       // |loadWithParams:| was called with URL that has different fragment
-      // comparing to the orevious URL.
-      // TODO(crbug.com/713836): Instead of creating a new context, find
-      // existing navigation context stored in |_navigationStates| and use it.
-      navigationContext = web::NavigationContextImpl::CreateNavigationContext(
-          _webStateImpl, newURL);
+      // comparing to the previous URL.

[kkhorimoto, 2017/05/17 23:05:37]

Can we restructure this if statement since this condition doesn't contain any
code anymore?

[Eugene But (OOO till 7-30), 2017/05/18 14:47:29]

We could but, I wanted to keep a comment to explain the case when context
exists. How do you want to restructure it?

[kkhorimoto, 2017/05/18 18:32:04]

Hmm not a lot of great ways to do it.  Maybe we could have a separate variable
that's just a raw NavigationContextImpl*, and we could store
|-contextForPendingNavigationWithURL:|'s return value there.  Then if we need to
create a new context, we store it in the unique ptr, then copy its value into
the other stack variable?  That way we could avoid needing to call it again
later below.  I think that might be more confusing than just having a no-op
condition though, so feel free to land as is.

[Eugene But (OOO till 7-30), 2017/05/18 18:46:22]

Thanks! Initially I had raw NavigationContextImpl*, but it required to have 2
variables (scoped_ptr and raw ptr), which looked confusing and required
explanation comment.

Landing as it is.

     } else {
       // This could be:
       //   1.) Renderer-initiated fragment change
       //   2.) Assigning same-origin URL to window.location
       //   3.) Incorrectly handled window.location.replace (crbug.com/307072)
       //   4.) Back-forward same document navigation
-      navigationContext = [self registerLoadRequestForURL:newURL];
+      newNavigationContext = [self registerLoadRequestForURL:newURL];
 
       // Use the current title for items created by same document navigations.
       auto* pendingItem = self.navigationManagerImpl->GetPendingItem();
       if (pendingItem)
         pendingItem->SetTitle(_webStateImpl->GetTitle());
     }
-    navigationContext->SetIsSameDocument(true);
   }
 
   [self setDocumentURL:newURL];
 
   if (!_changingHistoryState) {
     [self didStartLoadingURL:_documentURL];
-    _webStateImpl->OnNavigationFinished(navigationContext.get());
+
+    // Pass either newly created context (if it exists) or context that already
+    // existed before.
+    web::NavigationContextImpl* navigationContext = newNavigationContext.get();
+    if (!navigationContext)
+      navigationContext = [self contextForPendingNavigationWithURL:newURL];
+    navigationContext->SetIsSameDocument(true);
+    _webStateImpl->OnNavigationFinished(navigationContext);
+
     [self updateSSLStatusForCurrentNavigationItem];
     [self didFinishNavigation:nil];
   }
 }
 
-- (BOOL)isLoadRequestPendingForURL:(const GURL&)targetURL {
-  if (self.loadPhase != web::LOAD_REQUESTED)
-    return NO;
+- (web::NavigationContextImpl*)contextForPendingNavigationWithURL:
+    (const GURL&)URL {
+  for (id navigation in [_navigationStates pendingNavigations]) {
+    if (navigation == [NSNull null]) {
+      // null is a valid navigation object passed to WKNavigationDelegate
+      // callbacks and represents window opening action.
+      navigation = nil;
+    }
 
-  web::NavigationItem* pendingItem =
-      self.webState->GetNavigationManager()->GetPendingItem();
-  return pendingItem && pendingItem->GetURL() == targetURL;
+    web::NavigationContextImpl* context =
+        [_navigationStates contextForNavigation:navigation];
+    if (context->GetUrl() == URL) {
+      return context;
+    }
+  }
+  return nullptr;
 }
 
 - (void)loadRequestForCurrentNavigationItem {
   DCHECK(_webView);
   DCHECK(self.currentNavItem);
   // If a load is kicked off on a WKWebView with a frame whose size is {0, 0} or
   // that has a negative dimension for a size, rendering issues occur that
   // manifest in erroneous scrolling and tap handling (crbug.com/574996,
   // crbug.com/577793).
   DCHECK_GT(CGRectGetWidth([_webView frame]), 0.0);
@@ skipping 188 matching lines @@
 - (NSUInteger)observerCount {
   DCHECK_EQ(_observerBridges.size(), [_observers count]);
   return [_observers count];
 }
 
 - (NSString*)referrerFromNavigationAction:(WKNavigationAction*)action {
   return [action.request valueForHTTPHeaderField:kReferrerHeaderName];
 }
 
 @end