Remove usage of Tab's |url| property from BrowserViewController.

ios/chrome/browser/ui/browser_view_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/chrome/browser/ui/browser_view_controller.h"
 
 #import <AssetsLibrary/AssetsLibrary.h>
 #import <MobileCoreServices/MobileCoreServices.h>
 #import <PassKit/PassKit.h>
 #import <Photos/Photos.h>
@@ skipping 1088 matching lines @@
   Tab* tab = [_model currentTab];
   if ([self isTabNativePage:tab])
     return NO;
 
   // If |useDesktopUserAgent| is |NO|, allow useDesktopUserAgent.
   return !tab.usesDesktopUserAgent;
 }
 
 // Whether the sharing menu should be shown.
 - (BOOL)canShowShareMenu {
-  Tab* tab = [_model currentTab];
-  // TODO(shreyasv): Make it so the URL returned by the tab is always valid and
-  // remove check on net::NSURLWithGURL(tab.url) ( http://crbug.com/400999 ).
-  return tab && !tab.url.SchemeIs(kChromeUIScheme) &&
-         net::NSURLWithGURL(tab.url);
+  const GURL& URL = [_model currentTab].lastCommittedURL;
+  return URL.is_valid() && !web::GetWebClient()->IsAppSpecificURL(URL);

[kkhorimoto, 2017/04/14 18:48:19]

We want to share based on the visible content, so I think last committed is good
here.

[Eugene But (OOO till 7-30), 2017/04/14 21:18:24]

This matches with adding bookmark functionality, so I agree 

 }
 
 - (BOOL)canShowFindBar {
   // Make sure web controller can handle find in page.
   Tab* tab = [_model currentTab];
   if (!tab) {
     return NO;
   }
 
   auto* helper = FindTabHelper::FromWebState(tab.webState);
@@ skipping 458 matching lines @@
       [self startVoiceSearch];
     }
   };
 
   self.inNewTabAnimation = YES;
   if (!inBackground) {
     UIView* animationParentView = _contentArea;
     // Create the new page image, and load with the new tab page snapshot.
     CGFloat newPageOffset = 0;
     UIImageView* newPage;
-    if (tab.url == GURL(kChromeUINewTabURL) && !_isOffTheRecord &&
+    if (tab.lastCommittedURL == GURL(kChromeUINewTabURL) && !_isOffTheRecord &&

[kkhorimoto, 2017/04/14 18:48:19]

If a Tab was just added, it's either a new tab, a window.open tab, or an
open-in-new-tab context menu tab.  It can only be an NTP if it's the first
option, and native content URLs are never pending, so last committed is
sufficient.

[Eugene But (OOO till 7-30), 2017/04/14 21:18:25]

In the worst case this will only be a cosmetic bug, but this looks correct to
me.

         !IsIPadIdiom()) {
       animationParentView = self.view;
       newPage = [self pageFullScreenOpenCloseAnimationView];
     } else {
       newPage = [self pageOpenCloseAnimationView];
     }
     newPageOffset = newPage.frame.origin.y;
 
     [tab view].frame = _contentArea.bounds;
     newPage.image = [tab updateSnapshotWithOverlay:YES visibleFrameOnly:YES];
@@ skipping 920 matching lines @@
                                                           params:params];
 
   NSString* title = nil;
   ProceduralBlock action = nil;
 
   __weak BrowserViewController* weakSelf = self;
   GURL link = params.link_url;
   bool isLink = link.is_valid();
   GURL imageUrl = params.src_url;
   bool isImage = imageUrl.is_valid();
+  const GURL& committedURL = [_model currentTab].lastCommittedURL;

[kkhorimoto, 2017/04/14 18:48:19]

Context menu is specific to visible content, so committed is sufficient here.

[Eugene But (OOO till 7-30), 2017/04/14 21:18:25]

This change fixes a bug.

 
   if (isLink) {
     if (link.SchemeIs(url::kJavaScriptScheme)) {
       // Open
       title = l10n_util::GetNSStringWithFixup(IDS_IOS_CONTENT_CONTEXT_OPEN);
       action = ^{
         Record(ACTION_OPEN_JAVASCRIPT, isImage, isLink);
         [weakSelf openJavascript:base::SysUTF8ToNSString(link.GetContent())];
       };
       [_contextMenuCoordinator addItemWithTitle:title action:action];
     }
 
     if (web::UrlHasWebScheme(link)) {
-      web::Referrer referrer([_model currentTab].url, params.referrer_policy);
+      web::Referrer referrer(committedURL, params.referrer_policy);
 
       // Open in New Tab.
       title = l10n_util::GetNSStringWithFixup(
           IDS_IOS_CONTENT_CONTEXT_OPENLINKNEWTAB);
       action = ^{
         Record(ACTION_OPEN_IN_NEW_TAB, isImage, isLink);
         [weakSelf webPageOrderedOpen:link
                             referrer:referrer
                         inBackground:YES
                             appendTo:kCurrentTab];
@@ skipping 29 matching lines @@
     }
     // Copy Link.
     title = l10n_util::GetNSStringWithFixup(IDS_IOS_CONTENT_CONTEXT_COPY);
     action = ^{
       Record(ACTION_COPY_LINK_ADDRESS, isImage, isLink);
       StoreURLInPasteboard(link);
     };
     [_contextMenuCoordinator addItemWithTitle:title action:action];
   }
   if (isImage) {
-    web::Referrer referrer([_model currentTab].url, params.referrer_policy);
+    web::Referrer referrer(committedURL, params.referrer_policy);
     // Save Image.
     if (experimental_flags::IsDownloadRenamingEnabled()) {
       title = l10n_util::GetNSStringWithFixup(
           IDS_IOS_CONTENT_CONTEXT_DOWNLOADIMAGE);
     } else {
       title =
           l10n_util::GetNSStringWithFixup(IDS_IOS_CONTENT_CONTEXT_SAVEIMAGE);
     }
     action = ^{
       Record(ACTION_SAVE_IMAGE, isImage, isLink);
@@ skipping 504 matching lines @@
   //   controller of the appropriate type vended to it (occurs when a native
   //   controller is opened in a new tab from a tab with a matching URL, e.g.
   //   opening an NTP when an NTP is already displayed in the current tab).
   // For normal page loads, history navigations, tab restorations, and crash
   // recoveries, the tab will already exist in the tab model and the tabId can
   // be used as the native controller key.
   // TODO(crbug.com/498568): To reduce complexity here, refactor the flow so
   // that native controllers vended here always correspond to the current tab.
   Tab* currentTab = [_model currentTab];
   NSString* nativeControllerKey = currentTab.tabId;
-  if (!currentTab || currentTab.url != url ||
+  if (!currentTab || currentTab.lastCommittedURL != url ||

[kkhorimoto, 2017/04/14 18:48:18]

Native URLs are never pending, so last committed is sufficient.

[Eugene But (OOO till 7-30), 2017/04/14 21:18:25]

Do you want to use webState->GetLastCommmittedURL? Otherwise looks correct to
me.

       [[_nativeControllersForTabIDs objectForKey:nativeControllerKey]
           isKindOfClass:[nativeController class]]) {
     nativeControllerKey = kNativeControllerTemporaryKey;
   }
   DCHECK(nativeControllerKey);
   [_nativeControllersForTabIDs setObject:nativeController
                                   forKey:nativeControllerKey];
   return nativeController;
 }
 
@@ skipping 1333 matching lines @@
   __weak BrowserViewController* weakSelf = self;
   web::JavaScriptResultBlock completionHandlerBlock = ^(id result, NSError*) {
     Tab* strongTab = weakTab;
     if (!strongTab)
       return;
     if (![result isKindOfClass:[NSString class]])
       result = @"Not an HTML page";
     std::string base64HTML;
     base::Base64Encode(base::SysNSStringToUTF8(result), &base64HTML);
     GURL URL(std::string("data:text/plain;charset=utf-8;base64,") + base64HTML);
-    web::Referrer referrer([strongTab url], web::ReferrerPolicyDefault);
+    web::Referrer referrer([strongTab lastCommittedURL],

[kkhorimoto, 2017/04/14 18:48:18]

View source is specific to visible content, so last committed is sufficient.

[Eugene But (OOO till 7-30), 2017/04/14 21:18:25]

Looks correct, and this is also debug-only feature.

+                           web::ReferrerPolicyDefault);
 
     [[weakSelf tabModel]
         insertTabWithURL:URL
                 referrer:referrer
               transition:ui::PAGE_TRANSITION_LINK
                   opener:strongTab
              openedByDOM:YES
                  atIndex:TabModelConstants::kTabPositionAutomatically
             inBackground:NO];
   };
@@ skipping 26 matching lines @@
 
 #pragma mark - ToolbarOwner
 
 - (ToolbarController*)relinquishedToolbarController {
   if (_isToolbarControllerRelinquished)
     return nil;
 
   ToolbarController* relinquishedToolbarController = nil;
   if ([_toolbarController view].hidden) {
     Tab* currentTab = [_model currentTab];
-    if (currentTab && UrlHasChromeScheme(currentTab.url)) {
+    if (currentTab && UrlHasChromeScheme(currentTab.lastCommittedURL)) {

[kkhorimoto, 2017/04/14 18:48:19]

chrome:// scheme URLs are never pending, so last committed is sufficient

[Eugene But (OOO till 7-30), 2017/04/14 21:18:24]

I think WebUI URLs can be pending.

[rohitrao (ping after 24h), 2017/05/09 15:18:38]

NTP is the only native page that's a ToolbarOwner, but I think pending might be
safer to use here.

Actually, we just changed the logic that specifies exactly when we swap
toolbars.  Does the logic here need to match?

[kkhorimoto, 2017/05/31 21:53:42]

The toolbar visibility is updated when a navigation is committed, so last
committed should work for that.

       // Use the native content controller's toolbar when the BVC's is hidden.
       id nativeController = [self nativeControllerForTab:currentTab];
       if ([nativeController conformsToProtocol:@protocol(ToolbarOwner)]) {
         relinquishedToolbarController =
             [nativeController relinquishedToolbarController];
         _relinquishedToolbarOwner = nativeController;
       }
     }
   } else {
     relinquishedToolbarController = _toolbarController;
@@ skipping 434 matching lines @@
 
 - (BOOL)shouldUseDesktopUserAgent {
   return [_model currentTab].usesDesktopUserAgent;
 }
 
 #pragma mark - BookmarkBridgeMethods
 
 // If an added or removed bookmark is the same as the current url, update the
 // toolbar so the star highlight is kept in sync.
 - (void)bookmarkNodeModified:(const BookmarkNode*)node {
-  if ([_model currentTab] && node->url() == [_model currentTab].url)
+  if ([_model currentTab] &&
+      node->url() == [_model currentTab].lastCommittedURL) {

[kkhorimoto, 2017/04/14 18:48:19]

Depends on what we decide for https://codereview.chromium.org/2816203002; this
should match the accessor we use there. 

[Eugene But (OOO till 7-30), 2017/04/14 21:18:24]

This matches Desktop, so should be fine.

     [self updateToolbar];
+  }
 }
 
 // If all bookmarks are removed, update the toolbar so the star highlight is
 // kept in sync.
 - (void)allBookmarksRemoved {
   [self updateToolbar];
 }
 
 #pragma mark - ShareToDelegate methods
 
@@ skipping 184 matching lines @@
 
 - (UIView*)voiceSearchButton {
   return _voiceSearchButton;
 }
 
 - (id<LogoAnimationControllerOwner>)logoAnimationControllerOwner {
   return [self currentLogoAnimationControllerOwner];
 }
 
 @end