Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706293008:
[ios] Creates ToolsMenuModel Class (Closed)
Description[ios] Creates ToolsMenuModel Class
Extracts the Model related code from ToolsMenuViewController to a new
class named ToolsMenuModel.
This CL is just the first step on refactoring and decoupling the Model
logic, so it can be used by the new ToolsMenuVC and the old one.
BUG=682880
Review-Url: https://codereview.chromium.org/2706293008
Cr-Commit-Position: refs/heads/master@{#452585}
Committed: https://chromium.googlesource.com/chromium/src/+/8a9ed5b3b71d3915fb734b99a5e6e0db550d79db
Patch Set 1 #Patch Set 2 : Includes the C class instead of importing. #
Total comments: 19
Patch Set 3 : CL Feedback #
Total comments: 4
Patch Set 4 : Format #
Messages
Total messages: 19 (7 generated)
Description was changed from ========== [ios] Creates ToolsMenuModel Class Extracts the Model related code from ToolsMenuViewController to a new class named ToolsMenuModel. This CL is just the first step on refactoring and decoupling the Model logic, so it can be used by the new ToolsMenuVC and the old one. BUG=682880 ========== to ========== [ios] Creates ToolsMenuModel Class Extracts the Model related code from ToolsMenuViewController to a new class named ToolsMenuModel. This CL is just the first step on refactoring and decoupling the Model logic, so it can be used by the new ToolsMenuVC and the old one. BUG=682880 ==========
sczs@chromium.org changed reviewers: + edchin@chromium.org, lpromero@chromium.org, marq@chromium.org
PTAL I will improve the existing model in further CL's and I might still decouple certain things if needed. But this was a simple clear cut to get us started.
lgtm. Good first step to breaking apart the massive view controller. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.h (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:39: extern NSString* const kToolsMenuSuggestionsId; Not for this CL: Consider moving all these accessibility constants to separate file. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:60: typedef NS_OPTIONS(NSUInteger, kToolbarType) { Not for this CL (especially since this is legacy code): The naming for this options enum should probably not start with "k". https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.mm (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:91: bool ToolsMenuModel::ItemShouldBeVisible(const MenuItemInfo& item, Just out of curiosity, why did you omit NS_INLINE as well as change BOOL to bool?
marq@chromium.org changed reviewers: + rohitrao@chromium.org
+rohitrao as FYI and to comment on NS_INLINE This is a good and useful refactor! However, I don't know how useful it will be for the new architecture. A significant amount of the configuration here is the association of chrome command IDs with menu items, which so far isn't a pattern we're using in the new architecture. That may change. IDS_IOS_TOOLBAR_SHOW_TABS also depends on IsIpadIdiom(), which we prefer to determine with size classes in the new architecture. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.h (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:60: typedef NS_OPTIONS(NSUInteger, kToolbarType) { On 2017/02/22 07:22:29, edchin wrote: > Not for this CL (especially since this is legacy code): The naming for this > options enum should probably not start with "k". Correct, chromium style is MACRO. But no need to fix that here. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:77: // Class for handling all ToolsMenuVC model related methods or configuration. Don't use classes (in either ObjC or C++) just to contain utility methods. Just declare a namespace and make it a free function. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.mm (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:91: bool ToolsMenuModel::ItemShouldBeVisible(const MenuItemInfo& item, On 2017/02/22 07:22:29, edchin wrote: > Just out of curiosity, why did you omit NS_INLINE as well as change BOOL to > bool? Either is OK, but if you change to bool you need to return bools (true/false) and not BOOLs (YES/NO). https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:103: if (item.title_id == IDS_IOS_TOOLBAR_SHOW_TABS) { I realize this is a direct lift from the old code, but from here on down this can just be a switch: switch (item.title_id) { case IDS_IOS_TOOLBAR_SHOW_TABS: return IsIPadIdiom(); case IDS_IOS_TOOLS_MENU_READER_MODE: return experimental_flags::IsReaderModeEnabled(); case IDS_IOS_TOOLS_MENU_READING_LIST: return reading_list::switches::IsReadingListEnabled(); case IDS_IOS_TOOLS_MENU_SUGGESTIONS: return experimental_flags::IsSuggestionsUIEnabled(); case IDS_IOS_OPTIONS_REPORT_AN_ISSUE: return ios::GetChromeBrowserProvider()->GetUserFeedbackProvider() ->IsUserFeedbackEnabled(); default: return YES; } Even if you don't do that, you should collapse this pattern: if (!Condition()) { return NO; } into return Condition();
https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.h (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:60: typedef NS_OPTIONS(NSUInteger, kToolbarType) { On 2017/02/22 11:15:57, marq wrote: > On 2017/02/22 07:22:29, edchin wrote: > > Not for this CL (especially since this is legacy code): The naming for this > > options enum should probably not start with "k". > > Correct, chromium style is MACRO. But no need to fix that here. I don't think we use MACRO style for NS_OPTIONS or NS_ENUM. I would expect to name constants with a prefix: typedef NS_OPTIONS(NSUInteger, ToolsMenuToolbarType) { ToolsMenuToolbarTypeNone = 0, ... }; https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:77: // Class for handling all ToolsMenuVC model related methods or configuration. On 2017/02/22 11:15:57, marq wrote: > Don't use classes (in either ObjC or C++) just to contain utility methods. Just > declare a namespace and make it a free function. Chromium doesn't use namespaces for this either =) A free function whose name starts with "ToolsMenu" would be fine. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.mm (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:43: IDC_NEW_TAB, kToolbarTypeAll, Can you put selectors into structs? We could expand this to include both IDC constants and selectors: { IDS_IOS_TOOLS_MENU_NEW_INCOGNITO_TAB, kToolsMenuNewIncognitoTabId, @selector(newIncognitoTab), IDC_NEW_INCOGNITO_TAB, kToolbarTypeAll, 0, nil }, If we end up using selectors to pass action messages, this could act as a model for both the old and new architectures. Eventually we'd just drop the IDC constants. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:91: bool ToolsMenuModel::ItemShouldBeVisible(const MenuItemInfo& item, On 2017/02/22 11:15:57, marq wrote: > On 2017/02/22 07:22:29, edchin wrote: > > Just out of curiosity, why did you omit NS_INLINE as well as change BOOL to > > bool? I have no idea why the NS_INLINE was in here in the first place. > Either is OK, but if you change to bool you need to return bools (true/false) > and not BOOLs (YES/NO). Doesn't matter much, but bool/true/false is probably a better fit for a C++ function.
https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.h (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:60: typedef NS_OPTIONS(NSUInteger, kToolbarType) { On 2017/02/22 12:17:33, rohitrao wrote: > On 2017/02/22 11:15:57, marq wrote: > > On 2017/02/22 07:22:29, edchin wrote: > > > Not for this CL (especially since this is legacy code): The naming for this > > > options enum should probably not start with "k". > > > > Correct, chromium style is MACRO. But no need to fix that here. > > I don't think we use MACRO style for NS_OPTIONS or NS_ENUM. I would expect to > name constants with a prefix: > > typedef NS_OPTIONS(NSUInteger, ToolsMenuToolbarType) { > ToolsMenuToolbarTypeNone = 0, > ... > }; Our style guides are unclear on this point, then. Regardless of this, the enum *type* should absolutely not begin with a 'k'. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.mm (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:43: IDC_NEW_TAB, kToolbarTypeAll, On 2017/02/22 12:17:33, rohitrao wrote: > Can you put selectors into structs? We could expand this to include both IDC > constants and selectors: > > { IDS_IOS_TOOLS_MENU_NEW_INCOGNITO_TAB, kToolsMenuNewIncognitoTabId, > @selector(newIncognitoTab), > IDC_NEW_INCOGNITO_TAB, kToolbarTypeAll, > 0, nil }, > > If we end up using selectors to pass action messages, this could act as a model > for both the old and new architectures. Eventually we'd just drop the IDC > constants. Yes, that would work. We would need to think about how this would be shared in that case, since so far our rationale for shared/ has been that it wouldn't have chrome/ or clean/ dependencies. This class would use chrome command_ids and clean action selectors. We could move both of those into shared/of course. Selectors in the structs can be added in a future CL.
PTAL Thanks for the feedback, it will be useful moving forward. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.h (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:39: extern NSString* const kToolsMenuSuggestionsId; On 2017/02/22 07:22:29, edchin wrote: > Not for this CL: Consider moving all these accessibility constants to separate > file. Acknowledged. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:60: typedef NS_OPTIONS(NSUInteger, kToolbarType) { On 2017/02/22 12:29:06, marq wrote: > On 2017/02/22 12:17:33, rohitrao wrote: > > On 2017/02/22 11:15:57, marq wrote: > > > On 2017/02/22 07:22:29, edchin wrote: > > > > Not for this CL (especially since this is legacy code): The naming for > this > > > > options enum should probably not start with "k". > > > > > > Correct, chromium style is MACRO. But no need to fix that here. > > > > I don't think we use MACRO style for NS_OPTIONS or NS_ENUM. I would expect to > > name constants with a prefix: > > > > typedef NS_OPTIONS(NSUInteger, ToolsMenuToolbarType) { > > ToolsMenuToolbarTypeNone = 0, > > ... > > }; > > Our style guides are unclear on this point, then. > > Regardless of this, the enum *type* should absolutely not begin with a 'k'. Acknowledged. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:77: // Class for handling all ToolsMenuVC model related methods or configuration. On 2017/02/22 12:17:33, rohitrao wrote: > On 2017/02/22 11:15:57, marq wrote: > > Don't use classes (in either ObjC or C++) just to contain utility methods. > Just > > declare a namespace and make it a free function. > > Chromium doesn't use namespaces for this either =) A free function whose name > starts with "ToolsMenu" would be fine. Done. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.mm (right): https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:43: IDC_NEW_TAB, kToolbarTypeAll, On 2017/02/22 12:29:07, marq wrote: > On 2017/02/22 12:17:33, rohitrao wrote: > > Can you put selectors into structs? We could expand this to include both IDC > > constants and selectors: > > > > { IDS_IOS_TOOLS_MENU_NEW_INCOGNITO_TAB, kToolsMenuNewIncognitoTabId, > > @selector(newIncognitoTab), > > IDC_NEW_INCOGNITO_TAB, kToolbarTypeAll, > > 0, nil }, > > > > If we end up using selectors to pass action messages, this could act as a > model > > for both the old and new architectures. Eventually we'd just drop the IDC > > constants. > > Yes, that would work. We would need to think about how this would be shared in > that case, since so far our rationale for shared/ has been that it wouldn't have > chrome/ or clean/ dependencies. This class would use chrome command_ids and > clean action selectors. We could move both of those into shared/of course. > > Selectors in the structs can be added in a future CL. That's exactly what I want to try doing in a next CL, I might even sub-class this from a shared model class so we can support the old VC and still have a clean model class for the new VC. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:91: bool ToolsMenuModel::ItemShouldBeVisible(const MenuItemInfo& item, On 2017/02/22 12:17:33, rohitrao wrote: > On 2017/02/22 11:15:57, marq wrote: > > On 2017/02/22 07:22:29, edchin wrote: > > > Just out of curiosity, why did you omit NS_INLINE as well as change BOOL to > > > bool? > > I have no idea why the NS_INLINE was in here in the first place. > > > Either is OK, but if you change to bool you need to return bools (true/false) > > and not BOOLs (YES/NO). > > Doesn't matter much, but bool/true/false is probably a better fit for a C++ > function. Done. https://codereview.chromium.org/2706293008/diff/20001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.mm:103: if (item.title_id == IDS_IOS_TOOLBAR_SHOW_TABS) { On 2017/02/22 11:15:57, marq wrote: > I realize this is a direct lift from the old code, but from here on down this > can just be a switch: > > switch (item.title_id) { > case IDS_IOS_TOOLBAR_SHOW_TABS: > return IsIPadIdiom(); > case IDS_IOS_TOOLS_MENU_READER_MODE: > return experimental_flags::IsReaderModeEnabled(); > case IDS_IOS_TOOLS_MENU_READING_LIST: > return reading_list::switches::IsReadingListEnabled(); > case IDS_IOS_TOOLS_MENU_SUGGESTIONS: > return experimental_flags::IsSuggestionsUIEnabled(); > case IDS_IOS_OPTIONS_REPORT_AN_ISSUE: > return ios::GetChromeBrowserProvider()->GetUserFeedbackProvider() > ->IsUserFeedbackEnabled(); > default: > return YES; > } > > Even if you don't do that, you should collapse this pattern: > > if (!Condition()) { > return NO; > } > > into > > return Condition(); Done.
LGTM after type name and formatting changes. https://codereview.chromium.org/2706293008/diff/40001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.h (right): https://codereview.chromium.org/2706293008/diff/40001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:60: typedef NS_OPTIONS(NSUInteger, kToolbarType) { Please, in this CL, change kToolbarType to ToolbarType. We can fix the enum name style later, but we can't have a type name beginning with 'k'. https://codereview.chromium.org/2706293008/diff/40001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:69: kToolbarTypeSwitcheriPhone | Align the 'k's on this and the next line with the 'k' after the equals sign on the line above.
Rohit, please let me know if there's something else you would like me to change, if not I'll send to the CQ later today. https://codereview.chromium.org/2706293008/diff/40001/ios/chrome/browser/ui/t... File ios/chrome/browser/ui/tools_menu/tools_menu_model.h (right): https://codereview.chromium.org/2706293008/diff/40001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:60: typedef NS_OPTIONS(NSUInteger, kToolbarType) { On 2017/02/22 18:39:05, marq wrote: > Please, in this CL, change kToolbarType to ToolbarType. We can fix the enum name > style later, but we can't have a type name beginning with 'k'. Done. https://codereview.chromium.org/2706293008/diff/40001/ios/chrome/browser/ui/t... ios/chrome/browser/ui/tools_menu/tools_menu_model.h:69: kToolbarTypeSwitcheriPhone | On 2017/02/22 18:39:06, marq wrote: > Align the 'k's on this and the next line with the 'k' after the equals sign on > the line above. Done.
The CQ bit was checked by sczs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from edchin@chromium.org, marq@chromium.org Link to the patchset: https://codereview.chromium.org/2706293008/#ps60001 (title: "Format")
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": 60001, "attempt_start_ts": 1487873300912300,
"parent_rev": "ba526877fb310ec4391bd67580eb46b2dbb8652d", "commit_rev":
"8a9ed5b3b71d3915fb734b99a5e6e0db550d79db"}
Message was sent while issue was closed.
Description was changed from ========== [ios] Creates ToolsMenuModel Class Extracts the Model related code from ToolsMenuViewController to a new class named ToolsMenuModel. This CL is just the first step on refactoring and decoupling the Model logic, so it can be used by the new ToolsMenuVC and the old one. BUG=682880 ========== to ========== [ios] Creates ToolsMenuModel Class Extracts the Model related code from ToolsMenuViewController to a new class named ToolsMenuModel. This CL is just the first step on refactoring and decoupling the Model logic, so it can be used by the new ToolsMenuVC and the old one. BUG=682880 Review-Url: https://codereview.chromium.org/2706293008 Cr-Commit-Position: refs/heads/master@{#452585} Committed: https://chromium.googlesource.com/chromium/src/+/8a9ed5b3b71d3915fb734b99a5e6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8a9ed5b3b71d3915fb734b99a5e6...
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/2711263002/ by sczs@chromium.org. The reason for reverting is: Broke EG test.
Message was sent while issue was closed.
On 2017/02/23 23:13:09, sczs wrote: > A revert of this CL (patchset #4 id:60001) has been created in > https://codereview.chromium.org/2711263002/ by mailto:sczs@chromium.org. > > The reason for reverting is: Broke EG test. The CL broke an internal EG test that I hadn't run. I will create a new CL with the Fix |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
