[iOS] Refactoring web CRWSessionController user agent code.

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 480 matching lines @@
 + (BOOL)webControllerCanShow:(const GURL&)url;
 // Clears the currently-displayed transient content view.
 - (void)clearTransientContentView;
 // Returns a lazily created CRWTouchTrackingRecognizer.
 - (CRWTouchTrackingRecognizer*)touchTrackingRecognizer;
 // Shows placeholder overlay.
 - (void)addPlaceholderOverlay;
 // Removes placeholder overlay.
 - (void)removePlaceholderOverlay;
 
+// Gets the associated NavigationManagerImpl.
+- (web::NavigationManagerImpl*)navigationManagerImpl;

[Eugene But (OOO till 7-30), 2017/02/17 00:00:24]

Could you please place this after |sessionController| property and make it a
property?

[liaoyuke, 2017/02/17 01:16:51]

Acknowledged.

+
 // Returns the current entry from the underlying session controller.
 // TODO(stuartmorgan): Audit all calls to these methods; these are just wrappers
 // around the same logic as GetActiveEntry, so should probably not be used for
 // the same reason that GetActiveEntry is deprecated. (E.g., page operations
 // should generally be dealing with the last commited entry, not a pending
 // entry).
 - (CRWSessionEntry*)currentSessionEntry;
 // Returns the navigation item for the current page.
 - (web::NavigationItem*)currentNavItem;
 // Returns the current transition type.
@@ skipping 855 matching lines @@
 
 - (void)setDocumentURL:(const GURL&)newURL {
   if (newURL != _documentURL && newURL.is_valid()) {
     _documentURL = newURL;
     _interactionRegisteredSinceLastURLChange = NO;
   }
 }
 
 - (void)setNavigationItemTitle:(NSString*)title {
   DCHECK(title);
-  auto& navigationManager = _webStateImpl->GetNavigationManagerImpl();
-  web::NavigationItem* item = navigationManager.GetLastCommittedItem();
+  web::NavigationItem* item =
+      [self navigationManagerImpl]->GetLastCommittedItem();
   if (!item)
     return;
 
   base::string16 newTitle = base::SysNSStringToUTF16(title);
   if (item->GetTitle() == newTitle)
     return;
 
   item->SetTitle(newTitle);
   // TODO(crbug.com/546218): See if this can be removed; it's not clear that
   // other platforms send this (tab sync triggers need to be compared against
   // upstream).
-  navigationManager.OnNavigationItemChanged();
+  [self navigationManagerImpl]->OnNavigationItemChanged();
 
   if ([_delegate respondsToSelector:@selector(webController:titleDidChange:)]) {
     [_delegate webController:self titleDidChange:title];
   }
 }
 
 - (BOOL)isCurrentNavigationItemPOST {
   // |_pendingNavigationInfo| will be nil if the decidePolicy* delegate methods
   // were not called.
   NSString* HTTPMethod =
@@ skipping 162 matching lines @@
   _loadPhase = web::LOAD_REQUESTED;
   _lastRegisteredRequestURL = requestURL;
 
   if (!(transition & ui::PAGE_TRANSITION_IS_REDIRECT_MASK)) {
     // Record state of outgoing page.
     [self recordStateInHistory];
   }
 
   [_delegate webWillAddPendingURL:requestURL transition:transition];
   // Add or update pending url.
-  if (_webStateImpl->GetNavigationManagerImpl().GetPendingItem()) {
+  if ([self navigationManagerImpl]->GetPendingItem()) {
     // Update the existing pending entry.
     // Typically on PAGE_TRANSITION_CLIENT_REDIRECT.
     [[self sessionController] updatePendingItem:requestURL];
   } else {
     // A new session history entry needs to be created.
-    [[self sessionController] addPendingItem:requestURL
-                                    referrer:referrer
-                                  transition:transition
-                           rendererInitiated:YES];
+    [self navigationManagerImpl]->AddPendingItem(
+        requestURL, referrer, transition,
+        web::ItemInitiationType::RENDERER_INITIATED);
   }
   _webStateImpl->SetIsLoading(true);
   _webStateImpl->OnProvisionalNavigationStarted(requestURL);
 }
 
 - (void)updateHTML5HistoryState {
   web::NavigationItemImpl* currentItem =
       static_cast<web::NavigationItemImpl*>([self currentNavItem]);
   if (!currentItem)
     return;
@@ skipping 17 matching lines @@
   // NavigationItems that were created or updated by calls to pushState() or
   // replaceState().
   BOOL shouldUpdateState = sameDocumentNavigation ||
                            currentItem->IsCreatedFromPushState() ||
                            currentItem->HasStateBeenReplaced();
   if (!shouldUpdateState)
     return;
 
   // TODO(stuartmorgan): Make CRWSessionController manage this internally (or
   // remove it; it's not clear this matches other platforms' behavior).
-  _webStateImpl->GetNavigationManagerImpl().OnNavigationItemCommitted();
+  [self navigationManagerImpl]->OnNavigationItemCommitted();
   // Record that a same-document hashchange event will be fired.  This flag will
   // be reset when resonding to the hashchange message.  Note that resetting the
   // flag in the completion block below is too early, as that block is called
   // before hashchange event listeners have a chance to fire.
   _dispatchingSameDocumentHashChangeEvent = shouldDispatchHashchange;
   // Inject the JavaScript to update the state on the browser side.
   [self injectHTML5HistoryScriptWithHashChange:shouldDispatchHashchange
                         sameDocumentNavigation:sameDocumentNavigation];
 }
 
@@ skipping 275 matching lines @@
   } else {
     // Clear transient view before making any changes to history and navigation
     // manager. TODO(stuartmorgan): Drive Transient Item clearing from
     // navigation system, rather than from WebController.
     [self clearTransientContentView];
 
     // TODO(stuartmorgan): Why doesn't recordStateInHistory get called for
     // forward/back transitions?
     [self recordStateInHistory];
 
-    CRWSessionController* history =
-        _webStateImpl->GetNavigationManagerImpl().GetSessionController();
     if (!self.currentSessionEntry)
       initialNavigation = YES;
-    [history addPendingItem:navUrl
-                   referrer:params.referrer
-                 transition:transition
-          rendererInitiated:params.is_renderer_initiated];
+
+    if (params.is_renderer_initiated) {
+      [self navigationManagerImpl]->AddPendingItem(

[Eugene But (OOO till 7-30), 2017/02/17 00:00:23]

How about this?:

Type navigationInitialtionType = params.is_renderer_initiated ?
    web::ItemInitiationType::RENDERER_INITIATED :
web::ItemInitiationType::USER_INITIATED;
[self navigationManagerImpl]->AddPendingItem(navUrl, params.referrer,
transition,navigationInitialtionType);

[liaoyuke, 2017/02/17 01:16:51]

Done.

+          navUrl, params.referrer, transition,
+          web::ItemInitiationType::RENDERER_INITIATED);
+    } else {
+      [self navigationManagerImpl]->AddPendingItem(
+          navUrl, params.referrer, transition,
+          web::ItemInitiationType::USER_INITIATED);
+    }
+
     web::NavigationItemImpl* addedItem =
         [self currentSessionEntry].navigationItemImpl;
     DCHECK(addedItem);
     if (params.extra_headers)
       addedItem->AddHttpRequestHeaders(params.extra_headers);
     if (params.post_data) {
       DCHECK([addedItem->GetHttpRequestHeaders() objectForKey:@"Content-Type"])
           << "Post data should have an associated content type";
       addedItem->SetPostData(params.post_data);
       addedItem->SetShouldSkipRepostFormConfirmation(true);
@@ skipping 118 matching lines @@
   } else if (_requireReloadOnDisplay && _webView) {
     _requireReloadOnDisplay = NO;
     [self addPlaceholderOverlay];
     [self loadCurrentURL];
   }
 }
 
 - (BOOL)shouldReload:(const GURL&)destinationURL
           transition:(ui::PageTransition)transition {
   // Do a reload if the user hits enter in the address bar or re-types a URL.
-  web::NavigationItem* item =
-      _webStateImpl->GetNavigationManagerImpl().GetVisibleItem();
+  web::NavigationItem* item = [self navigationManagerImpl]->GetVisibleItem();
   return (transition & ui::PAGE_TRANSITION_FROM_ADDRESS_BAR) && item &&
          (destinationURL == item->GetURL() ||
           destinationURL == item->GetOriginalRequestURL());
 }
 
 // Reload either the web view or the native content depending on which is
 // displayed.
 - (void)reloadInternal {
   // Clear last user interaction.
   // TODO(crbug.com/546337): Move to after the load commits, in the subclass
   // implementation. This will be inaccurate if the reload fails or is
   // cancelled.
   _lastUserInteraction.reset();
   base::RecordAction(UserMetricsAction("Reload"));
   if (_webView) {
     web::NavigationItem* transientItem =
-        _webStateImpl->GetNavigationManagerImpl().GetTransientItem();
+        [self navigationManagerImpl]->GetTransientItem();
     if (transientItem) {
       // If there's a transient item, a reload is considered a new navigation to
       // the transient item's URL (as on other platforms).
       NavigationManager::WebLoadParams reloadParams(transientItem->GetURL());
       reloadParams.transition_type = ui::PAGE_TRANSITION_RELOAD;
       reloadParams.extra_headers.reset(
           [transientItem->GetHttpRequestHeaders() copy]);
       [self loadWithParams:reloadParams];
     } else {
       // As with back and forward navigation, load the URL manually instead of
@@ skipping 173 matching lines @@
 
 - (void)goDelta:(int)delta {
   if (_isBeingDestroyed)
     return;
 
   if (delta == 0) {
     [self reload];
     return;
   }
 
-  web::NavigationManagerImpl& navigationManager =
-      _webStateImpl->GetNavigationManagerImpl();
-  if (navigationManager.CanGoToOffset(delta)) {
-    [self goToItemAtIndex:navigationManager.GetIndexForOffset(delta)];
+  if ([self navigationManagerImpl]->CanGoToOffset(delta)) {
+    [self

[Eugene But (OOO till 7-30), 2017/02/17 00:00:24]

nit: Do you want to create a local variable for index to improve the formatting?

[liaoyuke, 2017/02/17 01:16:51]

Done.

+        goToItemAtIndex:[self navigationManagerImpl]->GetIndexForOffset(delta)];
   }
 }
 
 - (void)finishHistoryNavigationFromEntry:(CRWSessionEntry*)fromEntry {
   [self webWillFinishHistoryNavigationFromEntry:fromEntry];
 
   // Only load the new URL if it has a different document than |fromEntry| to
   // prevent extra page loads from NavigationItems created by hash changes or
   // calls to window.history.pushState().
-  BOOL shouldLoadURL =
-      ![_webStateImpl->GetNavigationManagerImpl().GetSessionController()
-          isSameDocumentNavigationBetweenItem:fromEntry.navigationItem
-                                      andItem:self.currentNavItem];
+  CRWSessionController* sessionController = self.sessionController;

[Eugene But (OOO till 7-30), 2017/02/17 00:00:24]

nit: Do you need this local variable?

[liaoyuke, 2017/02/17 01:16:51]

Done.

+  BOOL shouldLoadURL = ![sessionController
+      isSameDocumentNavigationBetweenItem:fromEntry.navigationItem
+                                  andItem:self.currentNavItem];
   web::NavigationItemImpl* currentItem =
       self.currentSessionEntry.navigationItemImpl;
   GURL endURL = [self URLForHistoryNavigationFromItem:fromEntry.navigationItem
                                                toItem:currentItem];
   if (shouldLoadURL) {
     ui::PageTransition transition = ui::PageTransitionFromInt(
         ui::PAGE_TRANSITION_RELOAD | ui::PAGE_TRANSITION_FORWARD_BACK);
 
     NavigationManager::WebLoadParams params(endURL);
     if (currentItem) {
@@ skipping 48 matching lines @@
   return _jsInjectionReceiver;
 }
 
 - (BOOL)shouldClosePageOnNativeApplicationLoad {
   // The page should be closed if it was initiated by the DOM and there has been
   // no user interaction with the page since the web view was created, or if
   // the page has no navigation items, as occurs when an App Store link is
   // opened from another application.
   BOOL rendererInitiatedWithoutInteraction =
       self.sessionController.openedByDOM && !_userInteractedWithWebController;
-  BOOL noNavigationItems =
-      !_webStateImpl->GetNavigationManagerImpl().GetItemCount();
+  BOOL noNavigationItems = ![self navigationManagerImpl]->GetItemCount();
   return rendererInitiatedWithoutInteraction || noNavigationItems;
 }
 
 - (BOOL)useDesktopUserAgent {
   web::NavigationItem* item = [self currentNavItem];
   return item && item->IsOverridingUserAgent();
 }
 
 - (web::MojoFacade*)mojoFacade {
   if (!_mojoFacade) {
@@ skipping 623 matching lines @@
     DLOG(WARNING) << "JS message parameter not found: pageUrl or baseUrl";
     return NO;
   }
   GURL replaceURL = web::history_state_util::GetHistoryStateChangeUrl(
       [self currentURL], GURL(baseURL), pageURL);
   // UIWebView seems to choke on unicode characters that haven't been
   // escaped; escape the URL now so the expected load URL is correct.
   replaceURL = URLEscapedForHistory(replaceURL);
   if (!replaceURL.is_valid())
     return YES;
-  const NavigationManagerImpl& navigationManager =
-      _webStateImpl->GetNavigationManagerImpl();
+
   web::NavigationItem* navItem = [self currentNavItem];
   // ReplaceState happened before first navigation entry or called right
   // after window.open when the url is empty/not valid.
-  if (!navItem ||
-      (navigationManager.GetItemCount() <= 1 && navItem->GetURL().is_empty()))
+  if (!navItem || ([self navigationManagerImpl]->GetItemCount() <= 1 &&
+                   navItem->GetURL().is_empty()))
     return YES;
   if (!web::history_state_util::IsHistoryStateChangeValid(
           [self currentNavItem]->GetURL(), replaceURL)) {
     // If the current session entry URL origin still doesn't match
     // replaceURL's origin, ignore the replaceState. This can happen if a
     // new URL is loaded just before the replaceState.
     return YES;
   }
   std::string stateObjectJSON;
   if (!message->GetString("stateObject", &stateObjectJSON)) {
@@ skipping 155 matching lines @@
   GURL requestURL = net::GURLWithNSURL(request.URL);
 
   // External application launcher needs |isNavigationTypeLinkActivated| to
   // decide if the user intended to open the application by clicking on a link.
   BOOL isNavigationTypeLinkActivated =
       action.navigationType == WKNavigationTypeLinkActivated;
 
   // Check if the link navigation leads to a launch of an external app.
   // TODO(crbug.com/607780): Revise the logic of allowing external app launch
   // and move it to externalAppLauncher.
-  BOOL isOpenInNewTabNavigation =
-      !_webStateImpl->GetNavigationManager()->GetItemCount();
+  BOOL isOpenInNewTabNavigation = ![self navigationManagerImpl]->GetItemCount();
   BOOL isPossibleLinkClick = [self isLinkNavigation:action.navigationType];
   if (isPossibleLinkClick || isOpenInNewTabNavigation ||
       PageTransitionCoreTypeIs([self currentTransition],
                                ui::PAGE_TRANSITION_AUTO_BOOKMARK)) {
     web::NavigationItem* item = [self currentNavItem];
     const GURL currentNavigationURL =
         item ? item->GetVirtualURL() : GURL::EmptyGURL();
     // Check If the URL is handled by a native app.
     if ([self urlTriggersNativeAppLaunch:requestURL
                                sourceURL:currentNavigationURL
@@ skipping 450 matching lines @@
     if (_webView) {
       [[self view] addSubview:_webView];
     }
   }
 }
 
 #pragma mark -
 #pragma mark Session Information
 
 - (CRWSessionController*)sessionController {
-  return _webStateImpl
-      ? _webStateImpl->GetNavigationManagerImpl().GetSessionController()
-      : nil;
+  return _webStateImpl ? [self navigationManagerImpl]->GetSessionController()

[Eugene But (OOO till 7-30), 2017/02/17 00:00:23]

return [self navigationManagerImpl] ? [self
navigationManagerImpl]->GetSessionController() : nil;

We should not assume that [self navigationManagerImpl] returns non-nil pointer
if _webStateImpl is not null.

[liaoyuke, 2017/02/17 01:16:51]

Done. And added a local variable to improve formatting.

+                       : nil;
+}
+
+- (NavigationManagerImpl*)navigationManagerImpl {
+  return _webStateImpl ? &(_webStateImpl->GetNavigationManagerImpl()) : nil;
 }
 
 - (CRWSessionEntry*)currentSessionEntry {
   return [[self sessionController] currentEntry];
 }
 
 - (web::NavigationItem*)currentNavItem {
   // This goes through the legacy Session* interface rather than Navigation*
   // because it is itself a legacy method that should not exist, and this
   // avoids needing to add a GetActiveItem to NavigationManager. If/when this
@@ skipping 1645 matching lines @@
 - (void)simulateLoadRequestWithURL:(const GURL&)URL {
   _lastRegisteredRequestURL = URL;
   _loadPhase = web::LOAD_REQUESTED;
 }
 
 - (NSString*)refererFromNavigationAction:(WKNavigationAction*)action {
   return [action.request valueForHTTPHeaderField:@"Referer"];
 }
 
 @end