[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..6709eb3db5d449946f0f51e049d7465c2ee86186 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,9 @@ @interface ToolsMenuViewController ()<UICollectionViewDelegateFlowLayout,
 
 // Get the reading list cell.
 - (ReadingListMenuViewCell*)readingListCell;
+
+// Hide a menu item by IDC value.
+- (void)hideItemWithTag:(NSInteger)tag;
 @end
 
 @implementation ToolsMenuViewController
@@ -277,6 +284,7 @@ @implementation ToolsMenuViewController
 @synthesize toolbarType = _toolbarType;
 @synthesize menuItems = _menuItems;
 @synthesize delegate = _delegate;
+@synthesize userAgentType = _userAgentType;
 
 #pragma mark Public methods
 
@@ -295,6 +303,16 @@ - (CGFloat)optimalHeight:(CGFloat)suggestedHeight {
   }
 }
 
+- (void)hideItemWithTag:(NSInteger)tag {
+  for (ToolsMenuViewItem* item in _menuItems) {
+    if ([item tag] == tag) {
+      [_menuItems removeObject:item];
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
 - (void)setItemEnabled:(BOOL)enabled withTag:(NSInteger)tag {
   for (ToolsMenuViewItem* item in _menuItems) {
     if ([item tag] == tag) {
@@ -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,29 @@ - (void)setIsTabLoading:(BOOL)isTabLoading {
   [[toolsCell reloadButton] setHidden:isTabLoading];
 }
 
+- (void)setUserAgentType:(web::UserAgentType)type {
+  _userAgentType = type;
+
+  // Hide exactly one of "Request Desktop Site" and "Request Mobile Site".
+  // 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).
+  if (type == web::UserAgentType::NONE || type == web::UserAgentType::MOBILE)
+    [self hideItemWithTag:IDC_REQUEST_MOBILE_SITE];
+  else
+    [self hideItemWithTag:IDC_REQUEST_DESKTOP_SITE];
+
+  // Decides the enability of "Request Desktop Site" and "Request Mobile Site".
+  [self setItemEnabled:NO withTag:IDC_REQUEST_DESKTOP_SITE];
+  [self setItemEnabled:NO withTag:IDC_REQUEST_MOBILE_SITE];
+
+  if (type == web::UserAgentType::Mobile)
+    [self setItemEnabled:YES withTag:IDC_REQUEST_DESKTOP_SITE];
+  else if (type == web::UserAgentType::DESKTOP)
+    [self setItemEnabled:YES withTag:IDC_REQUEST_MOBILE_SITE];

[kkhorimoto, 2017/02/27 23:44:24]

Let's just combine these into a single condition, as it seems more natural to
not disable both and selectively re-enable later:

int enabledItemTag;
if (type == web::UserAgentType::None || type == web::UserAgentType::MOBILE) {
  enabledItemTag = IDC_REQUEST_DESKTOP_SITE;
  [self hideItemWithTag:IDC_REQUEST_MOBILE_SITE;
} else {
  enabledItemTag == IDC_REQUEST_MOBILE_SITE;
  [self hideItemWithTag:IDC_REQUEST_DESKTOP_SITE;
}
[self setItemEnabled:(type != web::UserAgentType::NONE) withTag:enabledItemTag];

[liaoyuke, 2017/02/28 02:01:00]

The condition for hiding and enabling is actually different:

Hide "Request Mobile Site" iff userAgentType is NONE or MOBILE. Enable "Request
Mobile Site" iff userAgentType is MOBILE.

And by the way, all items are enabled by default, and that's why I disable them
first. If we don't disable them first, we will end up with 3 conditions: if
(MOBILE) else if (DESKTOP) else

But I get your point, and it is unnatural, so I've updated the comments, please
let me know if it looks better now.

[liaoyuke, 2017/02/28 02:01:01]

Also note that I will change this logic to always hide Request Mobile Site once
this CL is good to go and add it back in the next CL.

[kkhorimoto, 2017/02/28 18:34:36]

I just double checked, and it seems to me that the flow that I suggested is
logically equivalent to the one you've written.  The condition for hiding one
option is the same as the one in your code.  Since one of the items is hidden,
we don't need to worry about enabling/disabling that item; only the visible one.
 And the combination of the condition you're using to hide and the condition
you're using to enable means that the visible item is enabled iff type != NONE. 
Maybe it would have been clearer if I had named the variable |visibleItemTag|
instead of |enabledItemTag|.  I think that enabling/disabling the visible item
by directly comparing type to NONE more clearly denotes the intention of this
code.

[kkhorimoto, 2017/02/28 18:36:08]

Also, you shouldn't make functional changes to a CL after it's lgtm'd.  Just
land the code with the appropriate item being visible/hidden and
enabled/disabled with a TODO to actually finish the implementation in a follow
up CL.

[liaoyuke, 2017/03/01 01:09:38]

Sorry, I missed that you have "(type != web::UserAgentType::NONE)" in your last
line, I think they are logically equivalent, but I don't like the name
enabledItemTag, when I look at this name, I would just assume that this tag will
always be enabled later, which isn't the case. Let's change it to
visibleItemTag.

Thank you for explaining :)

I totally agree that I shouldn't care about the enability of the hidden item,
I'll remove them right away.

+}
+
 - (void)initializeMenu:(ToolsMenuContext*)context {
   if (context.readingListMenuNotifier) {
     _readingListMenuNotifier.reset(context.readingListMenuNotifier);