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

ios/chrome/browser/ui/browser_view_controller.mm

Index: ios/chrome/browser/ui/browser_view_controller.mm
diff --git a/ios/chrome/browser/ui/browser_view_controller.mm b/ios/chrome/browser/ui/browser_view_controller.mm
index cf8e9d41d368fd22e2ca27a27d40771f3d2fddd7..395160dc28b5d7fb15c696f905f12cab15532046 100644
--- a/ios/chrome/browser/ui/browser_view_controller.mm
+++ b/ios/chrome/browser/ui/browser_view_controller.mm
@@ -1106,11 +1106,8 @@ - (BOOL)canUseDesktopUserAgent {
 
 // 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 {
@@ -1589,7 +1586,7 @@ - (void)tabWasAdded:(NSNotification*)notify {
     // 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];
@@ -2530,6 +2527,7 @@ - (BOOL)webState:(web::WebState*)webState
   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)) {
@@ -2543,7 +2541,7 @@ - (BOOL)webState:(web::WebState*)webState
     }
 
     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(
@@ -2593,7 +2591,7 @@ - (BOOL)webState:(web::WebState*)webState
     [_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(
@@ -3118,7 +3116,7 @@ - (BOOL)hasControllerForURL:(const GURL&)url {
   // 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;
@@ -4472,7 +4470,8 @@ - (void)viewSource {
     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
@@ -4519,7 +4518,7 @@ - (ToolbarController*)relinquishedToolbarController {
   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)]) {
@@ -4974,8 +4973,10 @@ - (BOOL)shouldUseDesktopUserAgent {
 // 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