[iOS] Add Request Mobile Site cell to tools menu

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

 // Copyright 2014 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/tools_menu/tools_menu_view_controller.h"
 
 #include <stdint.h>
 
 #include "base/ios/ios_util.h"
 #import "base/ios/weak_nsobject.h"
@@ skipping 32 matching lines @@
 NSString* const kToolsMenuCloseAllIncognitoTabsId =
     @"kToolsMenuCloseAllIncognitoTabsId";
 NSString* const kToolsMenuBookmarksId = @"kToolsMenuBookmarksId";
 NSString* const kToolsMenuReadingListId = @"kToolsMenuReadingListId";
 NSString* const kToolsMenuOtherDevicesId = @"kToolsMenuOtherDevicesId";
 NSString* const kToolsMenuHistoryId = @"kToolsMenuHistoryId";
 NSString* const kToolsMenuReportAnIssueId = @"kToolsMenuReportAnIssueId";
 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";
 
 namespace {
 
 // Time for ink to fade into view.
 static const NSTimeInterval kMDCInkTouchDelayInterval = 0.15;
 
 static const CGFloat kMenuItemHeight = 48;
@@ skipping 68 matching lines @@
     0,                                    nil },
   { IDS_IOS_OPTIONS_REPORT_AN_ISSUE,      kToolsMenuReportAnIssueId,
     IDC_REPORT_AN_ISSUE,                  kToolbarTypeAll,
     0,                                    nil },
   { IDS_IOS_TOOLS_MENU_FIND_IN_PAGE,      kToolsMenuFindInPageId,
     IDC_FIND,                             kToolbarTypeWebAll,
     0,                                    nil },
   { 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 },
   { IDS_IOS_TOOLS_MENU_SETTINGS,          kToolsMenuSettingsId,
     IDC_OPTIONS,                          kToolbarTypeAll,
     0,                                    nil },
   { IDS_IOS_TOOLS_MENU_HELP_MOBILE,           kToolsMenuHelpId,
     IDC_HELP_PAGE_VIA_MENU,               kToolbarTypeWebAll,
     0,                                    nil },
     // clang-format on
@@ skipping 108 matching lines @@
   // for the reading list badge.
   base::WeakNSObject<ReadingListMenuNotifier> _readingListMenuNotifier;
 }
 @property(nonatomic, retain) ToolsMenuCollectionView* menuView;
 @property(nonatomic, retain) MDCInkView* touchFeedbackView;
 @property(nonatomic, retain) NSMutableArray* menuItems;
 @property(nonatomic, assign) kToolbarType toolbarType;
 
 // Get the reading list cell.
 - (ReadingListMenuViewCell*)readingListCell;
+
+// Hide a menu item by IDC value.
+- (void)hideItemWithTag:(NSInteger)tag;
 @end
 
 @implementation ToolsMenuViewController
 
 @synthesize menuView = _menuView;
 @synthesize isCurrentPageBookmarked = _isCurrentPageBookmarked;
 @synthesize touchFeedbackView = _touchFeedbackView;
 @synthesize isTabLoading = _isTabLoading;
 @synthesize toolbarType = _toolbarType;
 @synthesize menuItems = _menuItems;
 @synthesize delegate = _delegate;
+@synthesize userAgentType = _userAgentType;
 
 #pragma mark Public methods
 
 - (CGFloat)optimalHeight:(CGFloat)suggestedHeight {
   NSInteger numberOfItems = [self.menuItems count];
   if (_toolbarType == kToolbarTypeWebiPhone) {
     // Account for the height of the first row, not included in |menuItems|.
     numberOfItems++;
   }
   CGFloat maxItems = suggestedHeight / kMenuItemHeight;
   if (maxItems >= numberOfItems) {
     return numberOfItems * kMenuItemHeight;
   } else {
     const CGFloat halfHeight = kMenuItemHeight / 2;
     return round(maxItems) * kMenuItemHeight - halfHeight;
   }
 }
 
+- (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) {
       [item setActive:enabled];
       break;
     }
   }
 
   for (ToolsMenuViewCell* cell in [_menuView visibleCells]) {
     if (![cell isKindOfClass:[ToolsMenuViewCell class]])
@@ skipping 13 matching lines @@
 
   ToolsMenuViewToolsCell* toolsCell = [self toolsCell];
   [[toolsCell starButton] setHidden:_isCurrentPageBookmarked];
   [[toolsCell starredButton] setHidden:!_isCurrentPageBookmarked];
 }
 
 - (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];
 }
 
 - (void)setCanShowShareMenu:(BOOL)enabled {
   ToolsMenuViewToolsCell* toolsCell = [self toolsCell];
   [[toolsCell shareButton] setEnabled:enabled];
   [self setItemEnabled:enabled withTag:IDC_SHARE_PAGE];
 }
 
 - (UIButton*)toolsButton {
   UIButton* toolsButton = [[self toolsCell] toolsButton];
   [toolsButton addTarget:self
                   action:@selector(buttonPressed:)
         forControlEvents:UIControlEventTouchUpInside];
   [toolsButton setTranslatesAutoresizingMaskIntoConstraints:YES];
   [toolsButton setOpaque:NO];
   [toolsButton setBackgroundColor:[UIColor clearColor]];
   return toolsButton;
 }
 
 - (void)setIsTabLoading:(BOOL)isTabLoading {
   _isTabLoading = isTabLoading;
 
   ToolsMenuViewToolsCell* toolsCell = [self toolsCell];
   [[toolsCell stopButton] setHidden:!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);
     [context.readingListMenuNotifier setDelegate:self];
   }
 
   if (IsIPadIdiom()) {
     _toolbarType = context.hasNoOpenedTabs
                        ? kToolbarTypeNoTabsiPad
                        : (!IsCompactTablet() ? kToolbarTypeWebiPad
@@ skipping 373 matching lines @@
 
 - (void)unreadCountChanged:(NSInteger)unreadCount {
   [[self readingListCell] updateBadgeCount:unreadCount animated:YES];
 }
 
 - (void)unseenStateChanged:(BOOL)unseenItemsExist {
   [[self readingListCell] updateSeenState:unseenItemsExist animated:YES];
 }
 
 @end