Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972733002:
Creates common super class for bookmark handset and tablet view controllers (Closed)
|
Created:
3 years, 5 months ago by ramyasharma Modified:
3 years, 5 months ago CC:
chromium-reviews, marq+watch_chromium.org, tfarina, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org, Marti Wong Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreates common super class for bookmark handset and tablet view controllers
This CL just pulls out the basic views, and models into the super class. Next
CLs will pull out delegate logic, and view layouts.
BUG=705339
Review-Url: https://codereview.chromium.org/2972733002
Cr-Commit-Position: refs/heads/master@{#485209}
Committed: https://chromium.googlesource.com/chromium/src/+/51dec2f185939a05ef01d21eccaf578b9d2ed7ea
Patch Set 1 #
Total comments: 44
Patch Set 2 #Messages
Total messages: 33 (25 generated)
Description was changed from ========== Common super class BUG= ========== to ========== Creates common super class for bookmark handset and tablet view controllers This CL just pulls out the basic views, and models into the super class. Next CLs will pull out delegate logic, and view layouts. BUG=705339 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by ramyasharma@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...
Patchset #1 (id:40001) has been deleted
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...)
ramyasharma@chromium.org changed reviewers: + noyau@chromium.org
Hi Eric, This is my first cl for the super class refactor, sending out the low lying fruits first to make sure they are small changes. Please let me know if this approach looks okay, and if I should be adding anyone else to the reviewers list / discuss the approach? Thanks, Ramya
noyau@chromium.org changed reviewers: + lpromero@chromium.org
Looks good. Most of my notes are for tidying up comments and a couple of TODOs for later. I'm adding Louis as CC, who is familiar with this code and can be a second pair of eye to catch issues. Feel free to use the both of us for reviews. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:31: @interface BookmarkHomeViewController : UIViewController Please add a class comment. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:33: // Designated initializer. Replace this comment by NS_DESIGNATED_INITIALIZER. Ideally also add the superclass initializers marked with NS_UNAVAILABLE https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:37: @property(nonatomic, assign, readonly) bookmarks::BookmarkModel* bookmarks; Need a quick comment. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:42: // View shown from the menu. This comment is net very informative. "The main view showing all the bookmarks" maybe? https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:45: @property(nonatomic, weak, readonly) id<UrlLoader> loader; Add comment https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:47: // The menu with all the folders and special entries. I don't think there are any special entries left :) https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:56: // in the BookmarkMenuView. I think this comment is misleading a bit. I believe there is only one collection view, period. What is visible in the collection view is determined by this attribute. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:59: @property(nonatomic, strong, readonly) BookmarkPanelView* panelView; Can you add a comment? https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:29: @interface BookmarkHomeViewController () Please add a comment explaining that those are r/w redeclaration of ro properties. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:59: DCHECK(browserState); Move the DCHECK to the beginning of the method. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:71: _panelView.delegate = nil; Now that we use ARC this should be replaced by weak delegates (maybe not in this CL, but maybe add a TODO? https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:89: // Therefore, it must be created here. This comment seems specific to iPhone. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:116: [[self primaryView] removeFromSuperview]; The primary view is now always the folder view. I'm pretty sure there is no need to remove it anymore. The code to add back the view has not made it to this class yet, when it does this should be simplified (again, maybe a TODO?) https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm:10: @interface MockBookmarkHomeViewController : BookmarkHomeViewController I don't understand why you don't use BookmarkHomeViewController directly?
lgtm with nits https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h:50: browserState:(ios::ChromeBrowserState*)browserState; Remove as it is the parent's case already. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.h (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.h:21: - (instancetype)initWithLoader:(id<UrlLoader>)loader Remove as it is the parent's case already. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm:251: - (void)loadView { Not needed anymore as this is no longer a specific ContentView. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm:436: // add the panelView to the view hierarchy. Nit: s/add/Add https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:54: // of the view hierarchy. This property determine which collection view is *determines https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm:5: #import "ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h" Add a new line below, or else sort-headers will mix it among the others. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm:28: EXPECT_TRUE(controller.menuView == nil); Optional nit: EXPECT_EQ(nil, controller.menuView); Using EXPECT_EQ/EXPECT_NE gives usually a better log when the expectation fails. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm:35: EXPECT_TRUE(controller != nil); Related optional nit: EXPECT_NE(nil, controller);
The CQ bit was checked by ramyasharma@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...
Patchset #2 (id:80001) has been deleted
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ramyasharma@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...
Thanks Eric and Louis https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_handset_view_controller.h:50: browserState:(ios::ChromeBrowserState*)browserState; On 2017/07/07 13:30:09, lpromero wrote: > Remove as it is the parent's case already. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.h (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.h:21: - (instancetype)initWithLoader:(id<UrlLoader>)loader On 2017/07/07 13:30:09, lpromero wrote: > Remove as it is the parent's case already. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm:251: - (void)loadView { On 2017/07/07 13:30:09, lpromero wrote: > Not needed anymore as this is no longer a specific ContentView. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_tablet_ntp_controller.mm:436: // add the panelView to the view hierarchy. On 2017/07/07 13:30:09, lpromero wrote: > Nit: s/add/Add Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:31: @interface BookmarkHomeViewController : UIViewController On 2017/07/07 09:37:18, noyau (Ping after 24h) wrote: > Please add a class comment. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:33: // Designated initializer. On 2017/07/07 09:37:19, noyau (Ping after 24h) wrote: > Replace this comment by NS_DESIGNATED_INITIALIZER. > > Ideally also add the superclass initializers marked with NS_UNAVAILABLE Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:37: @property(nonatomic, assign, readonly) bookmarks::BookmarkModel* bookmarks; On 2017/07/07 09:37:20, noyau (Ping after 24h) wrote: > Need a quick comment. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:42: // View shown from the menu. On 2017/07/07 09:37:22, noyau (Ping after 24h) wrote: > This comment is net very informative. "The main view showing all the bookmarks" > maybe? Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:45: @property(nonatomic, weak, readonly) id<UrlLoader> loader; On 2017/07/07 09:37:22, noyau (Ping after 24h) wrote: > Add comment Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:47: // The menu with all the folders and special entries. On 2017/07/07 09:37:18, noyau (Ping after 24h) wrote: > I don't think there are any special entries left :) thanks, just copied over the comment from before :). Changed now. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:54: // of the view hierarchy. This property determine which collection view is On 2017/07/07 13:30:09, lpromero wrote: > *determines Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:56: // in the BookmarkMenuView. On 2017/07/07 09:37:21, noyau (Ping after 24h) wrote: > I think this comment is misleading a bit. I believe there is only one collection > view, period. What is visible in the collection view is determined by this > attribute. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h:59: @property(nonatomic, strong, readonly) BookmarkPanelView* panelView; On 2017/07/07 09:37:18, noyau (Ping after 24h) wrote: > Can you add a comment? Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:29: @interface BookmarkHomeViewController () On 2017/07/07 09:37:23, noyau (Ping after 24h) wrote: > Please add a comment explaining that those are r/w redeclaration of ro > properties. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:59: DCHECK(browserState); On 2017/07/07 09:37:22, noyau (Ping after 24h) wrote: > Move the DCHECK to the beginning of the method. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:71: _panelView.delegate = nil; On 2017/07/07 09:37:23, noyau (Ping after 24h) wrote: > Now that we use ARC this should be replaced by weak delegates (maybe not in this > CL, but maybe add a TODO? Just checked the delegates, they have already been converted to weak delegates. So I don't need to set these to nil in dealloc. Deleted this code. Thanks https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:89: // Therefore, it must be created here. On 2017/07/07 09:37:24, noyau (Ping after 24h) wrote: > This comment seems specific to iPhone. Acknowledged. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.mm:116: [[self primaryView] removeFromSuperview]; On 2017/07/07 09:37:24, noyau (Ping after 24h) wrote: > The primary view is now always the folder view. I'm pretty sure there is no need > to remove it anymore. The code to add back the view has not made it to this > class yet, when it does this should be simplified (again, maybe a TODO?) Thanks, I have added a TODO to take care of this. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm (right): https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm:5: #import "ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller.h" On 2017/07/07 13:30:09, lpromero wrote: > Add a new line below, or else sort-headers will mix it among the others. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm:10: @interface MockBookmarkHomeViewController : BookmarkHomeViewController On 2017/07/07 09:37:24, noyau (Ping after 24h) wrote: > I don't understand why you don't use BookmarkHomeViewController directly? This was just copied from the other test class. Going with the real class now. thanks https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm:28: EXPECT_TRUE(controller.menuView == nil); On 2017/07/07 13:30:10, lpromero wrote: > Optional nit: > > EXPECT_EQ(nil, controller.menuView); > > Using EXPECT_EQ/EXPECT_NE gives usually a better log when the expectation fails. Done. https://codereview.chromium.org/2972733002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/bookmarks/bookmark_home_view_controller_unittest.mm:35: EXPECT_TRUE(controller != nil); On 2017/07/07 13:30:09, lpromero wrote: > Related optional nit: > > EXPECT_NE(nil, controller); Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:100001) has been deleted
The CQ bit was checked by ramyasharma@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lpromero@chromium.org Link to the patchset: https://codereview.chromium.org/2972733002/#ps120001 (title: "")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by ramyasharma@chromium.org
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": 120001, "attempt_start_ts": 1499673402353320,
"parent_rev": "7c708332548f834d469be0af9659ceecb3ab3fa9", "commit_rev":
"51dec2f185939a05ef01d21eccaf578b9d2ed7ea"}
Message was sent while issue was closed.
Description was changed from ========== Creates common super class for bookmark handset and tablet view controllers This CL just pulls out the basic views, and models into the super class. Next CLs will pull out delegate logic, and view layouts. BUG=705339 ========== to ========== Creates common super class for bookmark handset and tablet view controllers This CL just pulls out the basic views, and models into the super class. Next CLs will pull out delegate logic, and view layouts. BUG=705339 Review-Url: https://codereview.chromium.org/2972733002 Cr-Commit-Position: refs/heads/master@{#485209} Committed: https://chromium.googlesource.com/chromium/src/+/51dec2f185939a05ef01d21eccaf... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:120001) as https://chromium.googlesource.com/chromium/src/+/51dec2f185939a05ef01d21eccaf... |
