[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.

[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
 
 @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;
@@ skipping 31 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;
+
+  // 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);
     [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