Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2714813002:
[iOS] Add Request Mobile Site cell to tools menu (Closed)
|
Created:
3 years, 10 months ago by liaoyuke Modified:
3 years, 8 months ago CC:
chromium-reviews, pkl (ping after 24h if needed), noyau+watch_chromium.org, asvitkine+watch_chromium.org, marq+watch_chromium.org, srahim+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[iOS] Add Request Mobile Site cell to tools menu
In the current design of Chrome for iOS, once the user pressed the
"Request Desktop Site", there is no corresponding "Request Mobile Site"
to switch back, which is counter-intuitive and not user-friendly.
This CL adds "Request Mobile Site" cell to the tools menu by placing it
right next to "Request Desktop Site" cell, and make it invisible.
BUG=678047
Review-Url: https://codereview.chromium.org/2714813002
Cr-Commit-Position: refs/heads/master@{#455238}
Committed: https://chromium.googlesource.com/chromium/src/+/ea9f3ee694c2244bdec23b85bdf9ee4a26e95e11
Patch Set 1 : Implement "Request Mobile Site" UI #
Total comments: 2
Patch Set 2 : Rebase #
Total comments: 22
Patch Set 3 : Kurt's comments #
Total comments: 14
Patch Set 4 : Addressed comments #Patch Set 5 : Inverted the sense of negative conditions to positive #Patch Set 6 : Update setUserAgentType implementation and Add TODO #
Total comments: 22
Patch Set 7 : Adopt UserAgentType and ToolsMenuConfiguration #
Total comments: 8
Patch Set 8 : Add unit test for tools menu view controller #Patch Set 9 : Add flag to hide request mobile site cell #
Total comments: 10
Patch Set 10 : Addressed unit test feedback #
Total comments: 2
Patch Set 11 : Make menuItems unmutable and copy #Patch Set 12 : Fix unit test #
Total comments: 1
Messages
Total messages: 61 (26 generated)
Description was changed from ========== [iOS] Implement Request Mobile Site In the current design of Chrome for iOS, once the user pressed the "Request Desktop Site", there is no corresponding "Request Mobile Site" to switch back, which is counter-intuitive and not user-friendly. This CL implements "Request Mobile Site" functionality by adding a "Request Mobile Site" item right next to "Request Desktop Site" item in the pop-up menu, and hide exactly one of them according to the type of user agent the current page is using. BUG=678047 ========== to ========== [iOS] Implement Request Mobile Site In the current design of Chrome for iOS, once the user pressed the "Request Desktop Site", there is no corresponding "Request Mobile Site" to switch back, which is counter-intuitive and not user-friendly. This CL implements "Request Mobile Site" functionality by adding a "Request Mobile Site" item right next to "Request Desktop Site" item in the pop-up menu, and hide exactly one of them according to the type of user agent the current page is using. BUG=678047 ==========
liaoyuke@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
Hey Eugene, Kurt, Per my offline discussion with Kurt, we agreed that it's better to implement reloading current page with new user agent in the same CL as implementing request mobile site, which is this CL. To avoid having a giant CL, my strategy would be divide my work into separate "patches" as following: "Patch" 1: Implement the UI, and in this patch, after one clicks on the "Request Desktop Site", "Request Desktop Site" cell will be replaced by "Request Mobile Site", if one clicks on "Request Mobile Site", it does nothing. "Patch": 2: Implement the functionality to reload current web view with new user agent. Please take a look at "Patch" 1 first, and if things look good to you, we will add "Patch" 2. Let me know if you don't think this strategy makes sense, and I'll re-organize my CL. Thank you very much! https://codereview.chromium.org/2714813002/diff/1/ios/chrome/browser/tabs/tab.h File ios/chrome/browser/tabs/tab.h (left): https://codereview.chromium.org/2714813002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.h:284: - (void)enableDesktopUserAgent; I found "enable" confusing because it is already used in the tools menu to represent if the "Request Desktop Site" is clickable. https://codereview.chromium.org/2714813002/diff/1/ios/chrome/browser/tabs/tab.mm File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2714813002/diff/1/ios/chrome/browser/tabs/tab... ios/chrome/browser/tabs/tab.mm:1568: } Place holder, will get rid of "[self navigationManager]->OverrideDesktopUserAgentForNextPendingItem()" in the next patch and have the current page reload with new user agent instead of adding a new pending item.
Will let Kurt to do first pass review
Let's break this CL down a little bit so that it is only updating the tools menu interface in the way I suggested. I don't think we need to be updating Tab's interface quite yet, since the updates you made here would be changed in your next CL anyway (as we talked about in our offline discussion of how we're going to actually implement the reload). Ideally, since we ultimately plan on removing Tab altogether, we can just use the public web// API from BVC to update the state and kick off the new load. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.h (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:292: - (void)reloadForUpdatedUserAgent; For this CL, let's just leave Tab's interface the same, since we aren't actually changing any implementation details yet. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1102: return [_model currentTab].usesDesktopUserAgent; After updating the tools menu interface as I suggested, these functions shouldn't be necessary anymore. When setting up the tools menu, you can just query [_model currentTab].webState->GetNavigationManager()->GetVisibleItem()->GetUserAgentType(). https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:3327: [toolsPopupController hideRequestDesktopSite]; This code doesn't seem necessary, since we're already hiding the options from the tools controller. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:4184: - (void)reloadPageWithMobileUserAgent { Please add a TODO here for the next step of feature. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:71: - (void)hideItemWithTag:(NSInteger)tag; This is only used from tools_menu_view_controller.mm, so it should be added to this class's private interface. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:83: - (void)hideRequestMobileSite; As noted below, let's remove these functions and do the hiding from the userAgentType setter. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:102: - (void)setCanUseMobileUserAgent:(BOOL)value; Instead of this and the above function, let's just expose a UserAgentType property corresponding with the UA type of the currently visible page. @property(nonatomic, assign) web::UserAgentType userAgentType; Then, in the setter for that property, hide the appropriate option. For NONE type (e.g. a native controller, or a tools menu displayed while the tab switcher is open), we should hide both options. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_popup_controller.h (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_popup_controller.h:54: - (void)setCanUseMobileUserAgent:(BOOL)value; Let's duplicate the interface I suggested for the menu view controller here: add a property, hide the unnecessary options from the setter.
On 2017/02/25 02:36:33, kkhorimoto_ wrote: > Let's break this CL down a little bit so that it is only updating the tools menu > interface in the way I suggested. I don't think we need to be updating Tab's > interface quite yet, since the updates you made here would be changed in your > next CL anyway (as we talked about in our offline discussion of how we're going > to actually implement the reload). > > Ideally, since we ultimately plan on removing Tab altogether, we can just use > the public web// API from BVC to update the state and kick off the new load. > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/tabs... > File ios/chrome/browser/tabs/tab.h (right): > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/tabs... > ios/chrome/browser/tabs/tab.h:292: - (void)reloadForUpdatedUserAgent; > For this CL, let's just leave Tab's interface the same, since we aren't actually > changing any implementation details yet. > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... > File ios/chrome/browser/ui/browser_view_controller.mm (right): > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/browser_view_controller.mm:1102: return [_model > currentTab].usesDesktopUserAgent; > After updating the tools menu interface as I suggested, these functions > shouldn't be necessary anymore. When setting up the tools menu, you can just > query [_model > currentTab].webState->GetNavigationManager()->GetVisibleItem()->GetUserAgentType(). > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/browser_view_controller.mm:3327: [toolsPopupController > hideRequestDesktopSite]; > This code doesn't seem necessary, since we're already hiding the options from > the tools controller. > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... > ios/chrome/browser/ui/browser_view_controller.mm:4184: - > (void)reloadPageWithMobileUserAgent { > Please add a TODO here for the next step of feature. > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... > File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... > ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:71: - > (void)hideItemWithTag:(NSInteger)tag; > This is only used from tools_menu_view_controller.mm, so it should be added to > this class's private interface. > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... > ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:83: - > (void)hideRequestMobileSite; > As noted below, let's remove these functions and do the hiding from the > userAgentType setter. > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... > ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:102: - > (void)setCanUseMobileUserAgent:(BOOL)value; > Instead of this and the above function, let's just expose a UserAgentType > property corresponding with the UA type of the currently visible page. > > @property(nonatomic, assign) web::UserAgentType userAgentType; > > Then, in the setter for that property, hide the appropriate option. For NONE > type (e.g. a native controller, or a tools menu displayed while the tab switcher > is open), we should hide both options. > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... > File ios/chrome/browser/ui/tools_menu/tools_popup_controller.h (right): > > https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... > ios/chrome/browser/ui/tools_menu/tools_popup_controller.h:54: - > (void)setCanUseMobileUserAgent:(BOOL)value; > Let's duplicate the interface I suggested for the menu view controller here: add > a property, hide the unnecessary options from the setter. Also, feel free to patch in my UserAgentType CL (https://codereview.chromium.org/2705293014/) and rebase this CL on top of it. I still have some unittests to write, and smaller changes to make before landing that CL, but the public interface should be the same as it is in the CL's current state. I won't be able to land it until monday, since Eugene has already left for the weekend.
Description was changed from ========== [iOS] Implement Request Mobile Site In the current design of Chrome for iOS, once the user pressed the "Request Desktop Site", there is no corresponding "Request Mobile Site" to switch back, which is counter-intuitive and not user-friendly. This CL implements "Request Mobile Site" functionality by adding a "Request Mobile Site" item right next to "Request Desktop Site" item in the pop-up menu, and hide exactly one of them according to the type of user agent the current page is using. BUG=678047 ========== to ========== [iOS] Add Request Mobile Site cell in tools menu In the current design of Chrome for iOS, once the user pressed the "Request Desktop Site", there is no corresponding "Request Mobile Site" to switch back, which is counter-intuitive and not user-friendly. This CL adds "Request Mobile Site" cell to the tools menu by placing it right next to "Request Desktop Site" cell, and make it invisible. BUG=678047 ==========
Description was changed from ========== [iOS] Add Request Mobile Site cell in tools menu In the current design of Chrome for iOS, once the user pressed the "Request Desktop Site", there is no corresponding "Request Mobile Site" to switch back, which is counter-intuitive and not user-friendly. This CL adds "Request Mobile Site" cell to the tools menu by placing it right next to "Request Desktop Site" cell, and make it invisible. BUG=678047 ========== to ========== [iOS] Add Request Mobile Site cell to tools menu In the current design of Chrome for iOS, once the user pressed the "Request Desktop Site", there is no corresponding "Request Mobile Site" to switch back, which is counter-intuitive and not user-friendly. This CL adds "Request Mobile Site" cell to the tools menu by placing it right next to "Request Desktop Site" cell, and make it invisible. BUG=678047 ==========
Replied to comments. PTAL. Thank you very much! https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.h (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:292: - (void)reloadForUpdatedUserAgent; On 2017/02/25 02:36:32, kkhorimoto_ wrote: > For this CL, let's just leave Tab's interface the same, since we aren't actually > changing any implementation details yet. Done. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1102: return [_model currentTab].usesDesktopUserAgent; On 2017/02/25 02:36:32, kkhorimoto_ wrote: > After updating the tools menu interface as I suggested, these functions > shouldn't be necessary anymore. When setting up the tools menu, you can just > query [_model > currentTab].webState->GetNavigationManager()->GetVisibleItem()->GetUserAgentType(). I personally do not like this type of function call, which violates the law of demeter: https://en.wikipedia.org/wiki/Law_of_Demeter. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:3327: [toolsPopupController hideRequestDesktopSite]; On 2017/02/25 02:36:32, kkhorimoto_ wrote: > This code doesn't seem necessary, since we're already hiding the options from > the tools controller. Done. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:4184: - (void)reloadPageWithMobileUserAgent { On 2017/02/25 02:36:32, kkhorimoto_ wrote: > Please add a TODO here for the next step of feature. I'll leave this for next CL. In this one, I'll just add the UI cell and hide it. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:71: - (void)hideItemWithTag:(NSInteger)tag; On 2017/02/25 02:36:32, kkhorimoto_ wrote: > This is only used from tools_menu_view_controller.mm, so it should be added to > this class's private interface. Acknowledged. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:83: - (void)hideRequestMobileSite; On 2017/02/25 02:36:33, kkhorimoto_ wrote: > As noted below, let's remove these functions and do the hiding from the > userAgentType setter. Acknowledged. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:102: - (void)setCanUseMobileUserAgent:(BOOL)value; On 2017/02/25 02:36:33, kkhorimoto_ wrote: > Instead of this and the above function, let's just expose a UserAgentType > property corresponding with the UA type of the currently visible page. > > @property(nonatomic, assign) web::UserAgentType userAgentType; > > Then, in the setter for that property, hide the appropriate option. For NONE > type (e.g. a native controller, or a tools menu displayed while the tab switcher > is open), we should hide both options. My only concern is that we might be creating unnecessary dependencies, I would prefer not to have Tools Menu know the existing of web::UserAgentType thing at all. +Eugene on this one. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_popup_controller.h (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_popup_controller.h:54: - (void)setCanUseMobileUserAgent:(BOOL)value; On 2017/02/25 02:36:33, kkhorimoto_ wrote: > Let's duplicate the interface I suggested for the menu view controller here: add > a property, hide the unnecessary options from the setter. Acknowledged.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Addressed Kurt's comments. I will wait until Kurt's UserAgentType CL lands before continuing this one. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1102: return [_model currentTab].usesDesktopUserAgent; On 2017/02/27 17:34:22, liaoyuke wrote: > On 2017/02/25 02:36:32, kkhorimoto_ wrote: > > After updating the tools menu interface as I suggested, these functions > > shouldn't be necessary anymore. When setting up the tools menu, you can just > > query [_model > > > currentTab].webState->GetNavigationManager()->GetVisibleItem()->GetUserAgentType(). > > I personally do not like this type of function call, which violates the law of > demeter: https://en.wikipedia.org/wiki/Law_of_Demeter. I guess this is fine in the context of BVC, as BVC already knows about everything, providing more layers of abstraction rarely makes a difference. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:71: - (void)hideItemWithTag:(NSInteger)tag; On 2017/02/27 17:34:22, liaoyuke wrote: > On 2017/02/25 02:36:32, kkhorimoto_ wrote: > > This is only used from tools_menu_view_controller.mm, so it should be added to > > this class's private interface. > > Acknowledged. Done. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:83: - (void)hideRequestMobileSite; On 2017/02/27 17:34:22, liaoyuke wrote: > On 2017/02/25 02:36:33, kkhorimoto_ wrote: > > As noted below, let's remove these functions and do the hiding from the > > userAgentType setter. > > Acknowledged. Done. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:102: - (void)setCanUseMobileUserAgent:(BOOL)value; On 2017/02/27 17:34:22, liaoyuke wrote: > On 2017/02/25 02:36:33, kkhorimoto_ wrote: > > Instead of this and the above function, let's just expose a UserAgentType > > property corresponding with the UA type of the currently visible page. > > > > @property(nonatomic, assign) web::UserAgentType userAgentType; > > > > Then, in the setter for that property, hide the appropriate option. For NONE > > type (e.g. a native controller, or a tools menu displayed while the tab > switcher > > is open), we should hide both options. > > My only concern is that we might be creating unnecessary dependencies, I would > prefer not to have Tools Menu know the existing of web::UserAgentType thing at > all. > > +Eugene on this one. Done.
https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1102: return [_model currentTab].usesDesktopUserAgent; On 2017/02/27 21:48:59, liaoyuke wrote: > On 2017/02/27 17:34:22, liaoyuke wrote: > > On 2017/02/25 02:36:32, kkhorimoto_ wrote: > > > After updating the tools menu interface as I suggested, these functions > > > shouldn't be necessary anymore. When setting up the tools menu, you can > just > > > query [_model > > > > > > currentTab].webState->GetNavigationManager()->GetVisibleItem()->GetUserAgentType(). > > > > I personally do not like this type of function call, which violates the law of > > demeter: https://en.wikipedia.org/wiki/Law_of_Demeter. > > I guess this is fine in the context of BVC, as BVC already knows about > everything, providing more layers of abstraction rarely makes a difference. I definitely agree that layering violations should be avoided. However, everything in the ios/web/public/ directory is considered in the publicly accessible layer. So whenever an object has access to a WebState, any functionality that is publicly exposed is not considered a layering violation. https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:102: - (void)setCanUseMobileUserAgent:(BOOL)value; On 2017/02/27 21:48:59, liaoyuke wrote: > On 2017/02/27 17:34:22, liaoyuke wrote: > > On 2017/02/25 02:36:33, kkhorimoto_ wrote: > > > Instead of this and the above function, let's just expose a UserAgentType > > > property corresponding with the UA type of the currently visible page. > > > > > > @property(nonatomic, assign) web::UserAgentType userAgentType; > > > > > > Then, in the setter for that property, hide the appropriate option. For > NONE > > > type (e.g. a native controller, or a tools menu displayed while the tab > > switcher > > > is open), we should hide both options. > > > > My only concern is that we might be creating unnecessary dependencies, I would > > prefer not to have Tools Menu know the existing of web::UserAgentType thing at > > all. > > > > +Eugene on this one. > > Done. As mentioned in my other comment, because UserAgentType is defined as part of the public directory, it is okay for any object outside of web// to access it. Otherwise, we'd need to store two bools depending on native vs. web pages and overriding vs. non-overriding UA. https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:3300: ->GetVisibleItem() It's a bug, but I believe there are some instances where GetVisibleItem() might return a nullptr. This can happen if you cancel a pending load when it was opened in a new Tab (or via window.open). Let's just use NONE as a default value if there is no visible item. https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:3302: [toolsPopupController setUserAgentType:userAgentType]; Let's use dot notation since this is a property now. https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:82: - (void)setUserAgentType:(web::UserAgentType)type; This isn't necessary since it's automatically created for your property. https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:398: [self setItemEnabled:YES withTag:IDC_REQUEST_MOBILE_SITE]; 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]; https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_popup_controller.h (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_popup_controller.h:37: - (void)setUserAgentType:(web::UserAgentType)type; This isn't necessary since it's created by your property declaration.
Patchset #4 (id:100001) has been deleted
PTAL. Thank you very much! https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:3300: ->GetVisibleItem() On 2017/02/27 23:44:24, kkhorimoto_ wrote: > It's a bug, but I believe there are some instances where GetVisibleItem() might > return a nullptr. This can happen if you cancel a pending load when it was > opened in a new Tab (or via window.open). Let's just use NONE as a default > value if there is no visible item. Done. https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:3302: [toolsPopupController setUserAgentType:userAgentType]; On 2017/02/27 23:44:24, kkhorimoto_ wrote: > Let's use dot notation since this is a property now. Done. https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:82: - (void)setUserAgentType:(web::UserAgentType)type; On 2017/02/27 23:44:24, kkhorimoto_ wrote: > This isn't necessary since it's automatically created for your property. Done. https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:398: [self setItemEnabled:YES withTag:IDC_REQUEST_MOBILE_SITE]; On 2017/02/27 23:44:24, kkhorimoto_ wrote: > 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]; 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. https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:398: [self setItemEnabled:YES withTag:IDC_REQUEST_MOBILE_SITE]; On 2017/02/27 23:44:24, kkhorimoto_ wrote: > 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]; 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. https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_popup_controller.h (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_popup_controller.h:37: - (void)setUserAgentType:(web::UserAgentType)type; On 2017/02/27 23:44:24, kkhorimoto_ wrote: > This isn't necessary since it's created by your property declaration. Done.
I've inverted the sense of negative conditions to positive in the Request Desktop/Mobile Site visibility and enability logic. Please let me know if it looks better. Thanks!
https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:398: [self setItemEnabled:YES withTag:IDC_REQUEST_MOBILE_SITE]; On 2017/02/28 02:01:01, liaoyuke wrote: > On 2017/02/27 23:44:24, kkhorimoto_ wrote: > > 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]; > > 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. 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.
https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:398: [self setItemEnabled:YES withTag:IDC_REQUEST_MOBILE_SITE]; On 2017/02/28 18:34:36, kkhorimoto_ wrote: > On 2017/02/28 02:01:01, liaoyuke wrote: > > On 2017/02/27 23:44:24, kkhorimoto_ wrote: > > > 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]; > > > > 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. > > 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. 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.
On 2017/02/28 18:36:08, kkhorimoto_ wrote: > https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... > File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): > > https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... > ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:398: [self > setItemEnabled:YES withTag:IDC_REQUEST_MOBILE_SITE]; > On 2017/02/28 18:34:36, kkhorimoto_ wrote: > > On 2017/02/28 02:01:01, liaoyuke wrote: > > > On 2017/02/27 23:44:24, kkhorimoto_ wrote: > > > > 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]; > > > > > > 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. > > > > 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. > > 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. whoops, I accidentally lgtm'd this CL in my comment above. not lgtm
PTAL! Thank you very much! https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/80001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:398: [self setItemEnabled:YES withTag:IDC_REQUEST_MOBILE_SITE]; On 2017/02/28 18:36:08, kkhorimoto_ wrote: > On 2017/02/28 18:34:36, kkhorimoto_ wrote: > > On 2017/02/28 02:01:01, liaoyuke wrote: > > > On 2017/02/27 23:44:24, kkhorimoto_ wrote: > > > > 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]; > > > > > > 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. > > > > 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. > > 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. 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.
lgtm with one last change! thanks for your patience with this CL :) https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:409: } 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.
Let's wait until web::UserAgentType is landed. Also could you please write unit tests for your changes. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:548: // is NONE if there is no visible page. Is this also NONE for pages where UA is not applicable? https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:694: // Sets the desktop user agent flag and reload the current page. s/reload/reloads https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:1107: - (web::UserAgentType)getUserAgentType { s/getUserAgentType/userAgentType From "Coding Guidelines for Cocoa": The use of “get” is unnecessary, unless one or more values are returned indirectly. https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Co... https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:1112: if (navigationManager && navigationManager->GetVisibleItem()) navigationManager can not be null. webState can. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:1113: userAgentType = navigationManager->GetVisibleItem()->GetUserAgentType(); I don't think there is a guarantee that VisibleItem can never be null. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:60: // visibility and enability of "Request Desktop Site" and "Request Mobile Site" "which also decides the visibility and enability " is pretty vague. Can we be specific what happens if Type is NONE, MOBILE, DESKTOP? https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:274: // Hide a menu item by IDC value. s/Hide/Hides Same comment for other comments https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:381: // Decides the visibility and enability of "Request Desktop Site" and s/enability/enabled state ? https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:409: } On 2017/03/01 18:52:52, kkhorimoto_ wrote: > 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. +1 that method which hides something should start with "hide". https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:411: - (void)hideItemWithTag:(NSInteger)tag { This code has the same lookup logic as setItemEnabled:. Could you please factor common code out?
Patchset #7 (id:180001) has been deleted
PTAL! Will add unit test in a separate patch. Thank you very much! https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:548: // is NONE if there is no visible page. On 2017/03/01 19:12:55, Eugene But wrote: > Is this also NONE for pages where UA is not applicable? Done. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:694: // Sets the desktop user agent flag and reload the current page. On 2017/03/01 19:12:54, Eugene But wrote: > s/reload/reloads Done. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:1107: - (web::UserAgentType)getUserAgentType { On 2017/03/01 19:12:55, Eugene But wrote: > s/getUserAgentType/userAgentType > > From "Coding Guidelines for Cocoa": The use of “get” is unnecessary, unless one > or more values are returned indirectly. > > https://developer.apple.com/library/content/documentation/Cocoa/Conceptual/Co... Done. Thank you for explaining with the reference! https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:1112: if (navigationManager && navigationManager->GetVisibleItem()) On 2017/03/01 19:12:55, Eugene But wrote: > navigationManager can not be null. webState can. Done. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:1113: userAgentType = navigationManager->GetVisibleItem()->GetUserAgentType(); On 2017/03/01 19:12:55, Eugene But wrote: > I don't think there is a guarantee that VisibleItem can never be null. Done. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:60: // visibility and enability of "Request Desktop Site" and "Request Mobile Site" On 2017/03/01 19:12:55, Eugene But wrote: > "which also decides the visibility and enability " is pretty vague. Can we be > specific what happens if Type is NONE, MOBILE, DESKTOP? Acknowledged. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:274: // Hide a menu item by IDC value. On 2017/03/01 19:12:55, Eugene But wrote: > s/Hide/Hides > > Same comment for other comments Acknowledged. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:381: // Decides the visibility and enability of "Request Desktop Site" and On 2017/03/01 19:12:55, Eugene But wrote: > s/enability/enabled state ? Done. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:409: } On 2017/03/01 18:52:52, kkhorimoto_ wrote: > 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. 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 :) https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:409: } On 2017/03/01 19:12:55, Eugene But wrote: > On 2017/03/01 18:52:52, kkhorimoto_ wrote: > > 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. > +1 that method which hides something should start with "hide". Done. https://codereview.chromium.org/2714813002/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:411: - (void)hideItemWithTag:(NSInteger)tag { On 2017/03/01 19:12:55, Eugene But wrote: > This code has the same lookup logic as setItemEnabled:. Could you please factor > common code out? Acknowledged.
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
On 2017/03/03 01:04:06, liaoyuke wrote: > PTAL! Will add unit test in a separate patch. Is there a reason for writing tests in a separate CL? Tests often affect API, so there are many advantages in writing them with the code.
Sorry for the confusion! But I said next patch not next CL, just wanted to make sure the new approach looks good to you at a high level first. However, I do want to let you know that I very much appreciate your efforts to push back bad code (without tests), if everyone had shared the same high standard of code quality, our code base wont end up in such a "mess" :) Thank you! On Thu, Mar 2, 2017 at 5:55 PM <eugenebut@chromium.org> wrote: On 2017/03/03 01:04:06, liaoyuke wrote: > PTAL! Will add unit test in a separate patch. Is there a reason for writing tests in a separate CL? Tests often affect API, so there are many advantages in writing them with the code. https://codereview.chromium.org/2714813002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/03/03 02:17:46, liaoyuke wrote: > Sorry for the confusion! But I said next patch not next CL, just wanted to > make sure the new approach looks good to you at a high level first. Sorry, apparently I can't read English :) code looks good https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:212: if (configuration.userAgentType == web::UserAgentType::NONE || Maybe |configuration.userAgentType ! web::UserAgentType::DESKTOP|? https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:287: // Gets the reading list cell. nit: s/Gets/Returns since you touching this :) https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:432: if (configuration.userAgentType == web::UserAgentType::NONE) Should this be a switch case?
https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:433: [self setItemEnabled:NO withTag:IDC_REQUEST_DESKTOP_SITE]; Can we add a TODO referencing your bug to decide what the correct behavior should be for NONE? I'm not sure whether it should always be a disabled desktop option, or if it should be the disabled version of whichever option would be shown for the previous non-native NavigationItem.
Patchset #8 (id:260001) has been deleted
PTAL. Thank you very much! Added unit test. Adding feature flag is nontrivial especially when we need to clean up override_desktop_user_agent_for_next_pending_item_ in navigation_manager_impl, so use #ifdef as a switch to turn off Request Mobile Site UI temporarily. Hey Kurt, Wanted to give you a heads up that we decided to hide the request mobile site UI because if I can't finish implementing the functionality in the next weeks for some reason, it will leave us with a broken UI, and we should avoid this from happening, even if it's unlikely. https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm (right): https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:212: if (configuration.userAgentType == web::UserAgentType::NONE || On 2017/03/03 02:30:25, Eugene But wrote: > Maybe |configuration.userAgentType ! web::UserAgentType::DESKTOP|? Done. https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:287: // Gets the reading list cell. On 2017/03/03 02:30:25, Eugene But wrote: > nit: s/Gets/Returns since you touching this :) Done. https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:432: if (configuration.userAgentType == web::UserAgentType::NONE) On 2017/03/03 02:30:25, Eugene But wrote: > Should this be a switch case? Done. https://codereview.chromium.org/2714813002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.mm:433: [self setItemEnabled:NO withTag:IDC_REQUEST_DESKTOP_SITE]; On 2017/03/03 18:42:43, kkhorimoto_ wrote: > Can we add a TODO referencing your bug to decide what the correct behavior > should be for NONE? I'm not sure whether it should always be a disabled desktop > option, or if it should be the disabled version of whichever option would be > shown for the previous non-native NavigationItem. I have a TODO at the 202th line in this file, and have send a document to mardini@ and UI/UX folks to ask their opinions ASAP.
PING
https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm (right): https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:5: #import "ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h" Linebreak after this https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:6: #include "base/mac/scoped_nsobject.h" import https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:14: @interface ToolsMenuViewController (ExposedForTesting) From "Unit Testing Best Practices": Test code using real APIs - don’t define back doors only for tests. go/unit-test-practices?cl=head#public-apis It's alright to make UI elements public for ToolsMenuViewController. https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:28: _configuration.reset(); Is this necessary? https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:43: base::scoped_nsobject<ToolsMenuConfiguration> _configuration; configuration_
drive-by: Hi Yuke, when pinging someone, it is best to use the format "username: ping" especially when you have multiple reviewer so that we can quickly know wether we have to look at the CL or not. Cheers,
Thank you for the tips! Sylvain :) On Mon, Mar 6, 2017 at 3:03 PM <sdefresne@chromium.org> wrote: > drive-by: Hi Yuke, when pinging someone, it is best to use the format > "username: > ping" especially when you have multiple reviewer so that we can quickly > know > wether we have to look at the CL or not. Cheers, > > https://codereview.chromium.org/2714813002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #10 (id:320001) has been deleted
PTAL. Thank you very much! https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm (right): https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:5: #import "ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h" On 2017/03/06 23:02:06, Eugene But wrote: > Linebreak after this Done. https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:6: #include "base/mac/scoped_nsobject.h" On 2017/03/06 23:02:06, Eugene But wrote: > import Done. https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:14: @interface ToolsMenuViewController (ExposedForTesting) On 2017/03/06 23:02:06, Eugene But wrote: > From "Unit Testing Best Practices": Test code using real APIs - don’t define > back doors only for tests. > > go/unit-test-practices?cl=head#public-apis > > It's alright to make UI elements public for ToolsMenuViewController. Thanks for explaining using links, they are really helpful :) https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:28: _configuration.reset(); On 2017/03/06 23:02:06, Eugene But wrote: > Is this necessary? Done. https://codereview.chromium.org/2714813002/diff/300001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller_unittest.mm:43: base::scoped_nsobject<ToolsMenuConfiguration> _configuration; On 2017/03/06 23:02:06, Eugene But wrote: > configuration_ Done.
https://codereview.chromium.org/2714813002/diff/340001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/340001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:58: @property(nonatomic, retain) NSMutableArray* menuItems; This should not be mutable and it should be a copy property
PTAL. Thanks! https://codereview.chromium.org/2714813002/diff/340001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h (right): https://codereview.chromium.org/2714813002/diff/340001/ios/chrome/browser/ui/... ios/chrome/browser/ui/tools_menu/tools_menu_view_controller.h:58: @property(nonatomic, retain) NSMutableArray* menuItems; On 2017/03/07 02:26:00, Eugene But wrote: > This should not be mutable and it should be a copy property Done.
lgtm, thank you for the patience
Thank you all for the time to review! On Tue, Mar 7, 2017 at 8:03 AM <eugenebut@chromium.org> wrote: > lgtm, thank you for the patience > > https://codereview.chromium.org/2714813002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
liaoyuke@chromium.org changed reviewers: + holte@chromium.org
+holte@, Do you mind taking a look at tools/metrics/actions/actions.xml for owner's approval? Thank you very much!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/07 16:56:28, liaoyuke wrote: > +holte@, > > Do you mind taking a look at tools/metrics/actions/actions.xml for owner's > approval? > > Thank you very much! lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2714813002/#ps380001 (title: "Fix unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 380001, "attempt_start_ts": 1488921945184820,
"parent_rev": "1c133ebd845bc02856102abda3210d26d676b0bb", "commit_rev":
"ea9f3ee694c2244bdec23b85bdf9ee4a26e95e11"}
Message was sent while issue was closed.
Description was changed from ========== [iOS] Add Request Mobile Site cell to tools menu In the current design of Chrome for iOS, once the user pressed the "Request Desktop Site", there is no corresponding "Request Mobile Site" to switch back, which is counter-intuitive and not user-friendly. This CL adds "Request Mobile Site" cell to the tools menu by placing it right next to "Request Desktop Site" cell, and make it invisible. BUG=678047 ========== to ========== [iOS] Add Request Mobile Site cell to tools menu In the current design of Chrome for iOS, once the user pressed the "Request Desktop Site", there is no corresponding "Request Mobile Site" to switch back, which is counter-intuitive and not user-friendly. This CL adds "Request Mobile Site" cell to the tools menu by placing it right next to "Request Desktop Site" cell, and make it invisible. BUG=678047 Review-Url: https://codereview.chromium.org/2714813002 Cr-Commit-Position: refs/heads/master@{#455238} Committed: https://chromium.googlesource.com/chromium/src/+/ea9f3ee694c2244bdec23b85bdf9... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:380001) as https://chromium.googlesource.com/chromium/src/+/ea9f3ee694c2244bdec23b85bdf9...
Message was sent while issue was closed.
srahim@chromium.org changed reviewers: + srahim@chromium.org
Message was sent while issue was closed.
Per b/36955197, please increase character limit to 25 characters. https://codereview.chromium.org/2714813002/diff/380001/ios/chrome/app/strings... File ios/chrome/app/strings/ios_strings.grd (right): https://codereview.chromium.org/2714813002/diff/380001/ios/chrome/app/strings... ios/chrome/app/strings/ios_strings.grd:1471: <message name="IDS_IOS_TOOLS_MENU_REQUEST_MOBILE_SITE" desc="The iOS menu item for requesting the mobile site [Length: 15em] [iOS only]"> Per localization, please increase the character limit for this message to 25 characters. |
