[iOS] Add Request Mobile Site cell to tools menu

ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm

Index: ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm
diff --git a/ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm b/ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm
index e6157e493bbd1ddc227f61e6b8c924c80f2fa7a0..3163c794724b1a898b5837d8fbc41e5b6bb5b9b9 100644
--- a/ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm
+++ b/ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm
@@ -50,6 +50,7 @@
 NSString* const kToolsMenuFindInPageId = @"kToolsMenuFindInPageId";
 NSString* const kToolsMenuReaderMode = @"kToolsMenuReaderMode";
 NSString* const kToolsMenuRequestDesktopId = @"kToolsMenuRequestDesktopId";
+NSString* const kToolsMenuRequestMobileId = @"kToolsMenuRequestMobileId";
 NSString* const kToolsMenuSettingsId = @"kToolsMenuSettingsId";
 NSString* const kToolsMenuHelpId = @"kToolsMenuHelpId";
 NSString* const kToolsMenuSuggestionsId = @"kToolsMenuSuggestionsId";
@@ -138,6 +139,9 @@ typedef NS_OPTIONS(NSUInteger, kToolbarType) {
   { IDS_IOS_TOOLS_MENU_REQUEST_DESKTOP_SITE, kToolsMenuRequestDesktopId,
     IDC_REQUEST_DESKTOP_SITE,             kToolbarTypeWebAll,
     0,                                    nil },
+  { IDS_IOS_TOOLS_MENU_REQUEST_MOBILE_SITE, kToolsMenuRequestMobileId,
+    IDC_REQUEST_MOBILE_SITE,             kToolbarTypeWebAll,
+    0,                                    nil },
   { IDS_IOS_TOOLS_MENU_READER_MODE,       kToolsMenuReaderMode,
     IDC_READER_MODE,                      kToolbarTypeWebAll,
     0,                                    nil },
@@ -266,6 +270,19 @@ @interface ToolsMenuViewController ()<UICollectionViewDelegateFlowLayout,
 
 // Get the reading list cell.
 - (ReadingListMenuViewCell*)readingListCell;
+
+// Hide a menu item by IDC value.

[Eugene But (OOO till 7-30), 2017/03/01 19:12:55]

s/Hide/Hides

Same comment for other comments

[liaoyuke, 2017/03/03 01:04:06]

Acknowledged.

+- (void)hideItemWithTag:(NSInteger)tag;
+
+// Show "Request Desktop Site" in tools menu and hide "Request Mobile Site".
+// Note that only one of |showRequestDesktopSite| and |showRequestMobileSite| is
+// expected to be called at any time.
+- (void)showRequestDesktopSite;
+
+// Show "Request Mobile Site" in tools menu and hide "Request Desktop Site".
+// Note that only one of |showRequestDesktopSite| and |showRequestMobileSite| is
+// expected to be called at any time.
+- (void)showRequestMobileSite;
 @end
 
 @implementation ToolsMenuViewController
@@ -277,6 +294,7 @@ @implementation ToolsMenuViewController
 @synthesize toolbarType = _toolbarType;
 @synthesize menuItems = _menuItems;
 @synthesize delegate = _delegate;
+@synthesize userAgentType = _userAgentType;
 
 #pragma mark Public methods
 
@@ -328,10 +346,6 @@ - (void)setCanUseReaderMode:(BOOL)enabled {
   [self setItemEnabled:enabled withTag:IDC_READER_MODE];
 }
 
-- (void)setCanUseDesktopUserAgent:(BOOL)enabled {
-  [self setItemEnabled:enabled withTag:IDC_REQUEST_DESKTOP_SITE];
-}
-
 - (void)setCanShowFindBar:(BOOL)enabled {
   [self setItemEnabled:enabled withTag:IDC_FIND];
 }
@@ -361,6 +375,49 @@ - (void)setIsTabLoading:(BOOL)isTabLoading {
   [[toolsCell reloadButton] setHidden:isTabLoading];
 }
 
+- (void)setUserAgentType:(web::UserAgentType)type {
+  _userAgentType = type;
+
+  // Decides the visibility and enability of "Request Desktop Site" and

[Eugene But (OOO till 7-30), 2017/03/01 19:12:55]

s/enability/enabled state ?

[liaoyuke, 2017/03/03 01:04:06]

Done.

+  // "Request Mobile Site" in tools menu.
+  //
+  // TODO(crbug.com/696676): Talk to UI/UX people to decide the correct behavior
+  // of "Requestion Desktop/Mobile Site" (e.g. Whether user agent flag should
+  // stick when going backward and which cell should be visible when navigating
+  // to native pages).
+  NSInteger visibleItemTag;
+  if (type == web::UserAgentType::NONE || type == web::UserAgentType::MOBILE) {
+    [self showRequestDesktopSite];
+    visibleItemTag = IDC_REQUEST_DESKTOP_SITE;
+  } else {
+    [self showRequestMobileSite];
+    visibleItemTag = IDC_REQUEST_MOBILE_SITE;
+  }
+
+  BOOL shouldItemBeEnabled = (type != web::UserAgentType::NONE);
+  [self setItemEnabled:shouldItemBeEnabled withTag:visibleItemTag];
+}
+
+- (void)showRequestDesktopSite {
+  // All items are visible by default, so hide the other one.
+  [self hideItemWithTag:IDC_REQUEST_MOBILE_SITE];
+}
+
+- (void)showRequestMobileSite {
+  // All items are visible by default, so hide the other one.
+  [self hideItemWithTag:IDC_REQUEST_DESKTOP_SITE];
+}

[kkhorimoto, 2017/03/01 18:52:52]

These methods aren't exactly showing an option, but rather removing the opposite
option, and as a result has strict expectations of how they can be called that
are only visible in the comments above in the private interface.  It would be
clearer if we renamed them to |hideRequestDesktopSite| and
|hideRequestMobileSite|.  However, since they're both one-line functions, let's
just call |-hideItemWithTag:| from within |-setUserAgentType:| and move the
comment about them initially both being visible up above.

[Eugene But (OOO till 7-30), 2017/03/01 19:12:55]

+1 that method which hides something should start with "hide".

[liaoyuke, 2017/03/03 01:04:06]

Done.

[liaoyuke, 2017/03/03 01:04:06]

Agreed. Though I would prefer positive over negatives in general, but here I
think you are right, it's better to call |hideRequestDesktopSite| directly
because |showRequestDesktopSite| doesn't match the implementations.

Thank you for explaining :)

+
+- (void)hideItemWithTag:(NSInteger)tag {

[Eugene But (OOO till 7-30), 2017/03/01 19:12:55]

This code has the same lookup logic as setItemEnabled:. Could you please factor
common code out?

[liaoyuke, 2017/03/03 01:04:06]

Acknowledged.

+  for (ToolsMenuViewItem* item in _menuItems) {
+    if ([item tag] == tag) {
+      [_menuItems removeObject:item];
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
 - (void)initializeMenu:(ToolsMenuContext*)context {
   if (context.readingListMenuNotifier) {
     _readingListMenuNotifier.reset(context.readingListMenuNotifier);