Pass WebState to NativeAppNavigationController

ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm

Index: ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm
diff --git a/ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm b/ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm
index 4515dcc5c0fa477a8dc60d7dd546d39145ec807c..56c2104cbbf91b9eb37db1c872676a0cf37c5e9f 100644
--- a/ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm
+++ b/ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm
@@ -9,6 +9,7 @@
 #include "base/metrics/user_metrics.h"
 #include "base/metrics/user_metrics_action.h"
 #include "components/infobars/core/infobar_manager.h"
+#include "ios/chrome/browser/infobars/infobar_manager_impl.h"
 #import "ios/chrome/browser/installation_notifier.h"
 #include "ios/chrome/browser/native_app_launcher/native_app_infobar_delegate.h"
 #import "ios/chrome/browser/open_url_util.h"
@@ -19,6 +20,7 @@
 #import "ios/public/provider/chrome/browser/native_app_launcher/native_app_whitelist_manager.h"
 #import "ios/web/navigation/navigation_manager_impl.h"
 #include "ios/web/public/navigation_item.h"
+#include "ios/web/public/web_state/web_state.h"

[Eugene But (OOO till 7-30), 2017/01/21 02:30:08]

s/include/import

[pkl (ping after 24h if needed), 2017/01/26 00:30:17]

Since web_state.h is pure C++ class WebState, shouldn't #include be used?

 #import "net/base/mac/url_conversions.h"
 #include "net/url_request/url_request_context_getter.h"
 
@@ -44,28 +46,29 @@ using base::UserMetricsAction;
 @end
 
 @implementation NativeAppNavigationController {
+  // WebState provides access to the *TabHelper objects. This will eventually
+  // replace the need to have |_tab| in this object.
+  web::WebState* _webState;
   // A reference to the URLRequestContextGetter needed to fetch icons.
   scoped_refptr<net::URLRequestContextGetter> _requestContextGetter;
-  // Tab hosting the infobar.
+  // DEPRECATED: Tab hosting the infobar and is also used for accessing Tab
+  // states such as navigation manager and whether it is a pre-rendered tab.
+  // Use |webState| whenever possible.
   __unsafe_unretained Tab* _tab;  // weak
   base::scoped_nsprotocol<id<NativeAppMetadata>> _metadata;
   // A set of appIds encoded as NSStrings.
   base::scoped_nsobject<NSMutableSet> _appsPossiblyBeingInstalled;
 }
 
-// This prevents incorrect initialization of this object.
-- (id)init {
-  NOTREACHED();
-  return nil;
-}
-
 // Designated initializer. Use this instead of -init.
-- (id)initWithRequestContextGetter:(net::URLRequestContextGetter*)context
-                               tab:(Tab*)tab {
+- (instancetype)initWithWebState:(web::WebState*)webState

[rohitrao (ping after 24h), 2017/01/24 14:24:56]

I can't decide whether we should pass both WebState and Tab, or if we should
just pass Tab and get the WebState from it.  Either way, we'll need to change
callers at some point in the future.

If you do leave it this way, we should at least DCHECK_EQ(webState,
tab.webState).

[pkl (ping after 24h if needed), 2017/01/26 00:30:17]

My intent is that we can stop passing in |tab| eventually.
NativeAppNavigationController should not need a |tab| as ivar. Everything can be
done through |webState|.
Added a couple of DCHECKs here.

+            requestContextGetter:(net::URLRequestContextGetter*)context
+                             tab:(Tab*)tab {
   self = [super init];
   if (self) {
     DCHECK(context);
     _requestContextGetter = context;
+    _webState = webState;
     // Allows |tab| to be nil for unit testing.
     _tab = tab;
     _appsPossiblyBeingInstalled.reset([[NSMutableSet alloc] init]);
@@ -101,7 +104,7 @@ using base::UserMetricsAction;
 
 - (void)showInfoBarIfNecessary {
   // Find a potential matching native app.
-  GURL pageURL = _tab.webState->GetLastCommittedURL();
+  GURL pageURL = _webState->GetLastCommittedURL();
   _metadata.reset(
       [ios::GetChromeBrowserProvider()->GetNativeAppWhitelistManager()
           newNativeAppForURL:pageURL]);
@@ -124,7 +127,8 @@ using base::UserMetricsAction;
   // and ignored behavior can be handled.
   [_metadata willBeShownInInfobarOfType:type];
   // Display the proper infobar.
-  infobars::InfoBarManager* infoBarManager = [_tab infoBarManager];
+  infobars::InfoBarManager* infoBarManager =
+      InfoBarManagerImpl::FromWebState(_webState);
   NativeAppInfoBarDelegate::Create(infoBarManager, self, pageURL, type);
   [self recordInfobarDisplayedOfType:type
                     onLinkNavigation:[self isLinkNavigation]];
@@ -180,6 +184,8 @@ using base::UserMetricsAction;
     return;
   DCHECK(![_appsPossiblyBeingInstalled containsObject:appIdString]);
   [_appsPossiblyBeingInstalled addObject:appIdString];
+  // TODO(pkl): Preferred method is to add a helper object to WebState

[Eugene But (OOO till 7-30), 2017/01/21 02:30:08]

Please use |TODO(crbug.com/):| format

[pkl (ping after 24h if needed), 2017/01/26 00:30:17]

Done.

+  // and use the helper object to launch Store Kit.

[rohitrao (ping after 24h), 2017/01/24 14:24:56]

The intent is to do this in a separate CL?

[pkl (ping after 24h if needed), 2017/01/26 00:30:17]

Yes. I've updated the comment to include a crbug reference to check.

   [_tab openAppStore:appIdString];
 }
 
@@ -209,16 +215,25 @@ using base::UserMetricsAction;
 }
 
 - (BOOL)isLinkNavigation {
-  if (![_tab navigationManager])
-    return NO;
-  web::NavigationItem* userItem = [_tab navigationManager]->GetLastUserItem();
-  if (!userItem)
-    return NO;
-  ui::PageTransition currentTransition = userItem->GetTransitionType();
-  return PageTransitionCoreTypeIs(currentTransition,
-                                  ui::PAGE_TRANSITION_LINK) ||
-         PageTransitionCoreTypeIs(currentTransition,
-                                  ui::PAGE_TRANSITION_AUTO_BOOKMARK);
+  web::NavigationManager* navigationManager = _webState->GetNavigationManager();

[Eugene But (OOO till 7-30), 2017/01/21 02:30:08]

Should we move this code to utility function, so it can be tested? The logic
here is quite non trivial...

[pkl (ping after 24h if needed), 2017/01/26 00:30:17]

Done.

+  if (navigationManager) {

[Eugene But (OOO till 7-30), 2017/01/21 02:30:08]

nit: do you want to keep original logic with early return to avoid indentations?

[pkl (ping after 24h if needed), 2017/01/26 00:30:17]

Reverted.

+    NSInteger index = navigationManager->GetCurrentItemIndex();

[Eugene But (OOO till 7-30), 2017/01/21 02:30:08]

Maybe s/NSInteger/int ?

[pkl (ping after 24h if needed), 2017/01/26 00:30:17]

Done.

+    while (index >= 0) {
+      web::NavigationItem* item = navigationManager->GetItemAtIndex(index);
+      DCHECK(item);
+      ui::PageTransition currentTransition = item->GetTransitionType();
+      // Checks all non-redirect entries for transitions that are either links
+      // or bookmarks.
+      if ((currentTransition & ui::PAGE_TRANSITION_IS_REDIRECT_MASK) == 0) {
+        return PageTransitionCoreTypeIs(currentTransition,
+                                        ui::PAGE_TRANSITION_LINK) ||
+               PageTransitionCoreTypeIs(currentTransition,
+                                        ui::PAGE_TRANSITION_AUTO_BOOKMARK);
+      }
+      --index;
+    }
+  }
+  return NO;
 }
 
 - (void)appDidInstall:(NSNotification*)notification {