Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2659693004:
Add context menu when long press on a reading list entry (Closed)
DescriptionAdd context menu when long press on a reading list entry
This CL adds a context menu with different actions when a long press on a
reading list entry occurs.
BUG=685230
Review-Url: https://codereview.chromium.org/2659693004
Cr-Commit-Position: refs/heads/master@{#448282}
Committed: https://chromium.googlesource.com/chromium/src/+/6a13836f0b66471a08f88e02fdaba0c7671f83e6
Patch Set 1 #Patch Set 2 : Add tests #Patch Set 3 : Cleanup #
Total comments: 28
Patch Set 4 : Address comments #
Total comments: 4
Patch Set 5 : Address comment #
Total comments: 21
Patch Set 6 : Address comments #Patch Set 7 : Add metrics #
Total comments: 4
Patch Set 8 : Address comments #
Total comments: 49
Patch Set 9 : Address marq comments #
Total comments: 4
Patch Set 10 : Address comments #Dependent Patchsets: Messages
Total messages: 33 (8 generated)
gambard@chromium.org changed reviewers: + jif@chromium.org
PTAL.
stkhapugin@chromium.org changed reviewers: + stkhapugin@chromium.org
Drive-by https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/app/strings/... File ios/chrome/app/strings/ios_strings.grd (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/app/strings/... ios/chrome/app/strings/ios_strings.grd:1075: View Offline Version Please add a length in em https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:24: // toolbar, dismissing the Reading List View and opening elements. This protocol is becoming bloated, consider splitting into separate protocols for toolbar and for everything else. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:36: // Displays a context menu attach to an element of the ReadingList View. I think your delegate displays a context menu, but it's the delegate's implementation detail. Instead add a comment explaining when this gesture recognizer is recognized. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:241: hasItems:self.readingListModel->size() > 0]; nit: parenthesis around hasItems: argument https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:20: // Container for the Reading List View Controller. This "container" is a few hundreds LOC. Please document its real purpose - acting as RLVC delegate, displaying toolbars, displaying pop-ups etc. In fact, I'd argue that this class should be renamed to RLVC, and RLVC -> RLCollectionViewController. If you choose to rename this, please do it in a separate CL to preserve diffs. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:179: if (gestureRecognizer.numberOfTouches != 1 || duringEdit || Instead configure numberOfTouchesRequired when you create |gestureRecognizer|. Also, you really shouldn't be concerned with internals of this gesture recognizer here. Instead just expose a delegate API: -rlvc:(RLVC*)sender longPressOnItem:(RLCollectionViewItem*)item duringEdit:(BOOL)edit. This way you can hide everything related to the collection view in the collection view controller and not expose it at all. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:341: [self.presentingViewController dismissViewControllerAnimated:YES Ideally view controllers should not dismiss themselves, instead calling the presenting VC through a delegate. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:406: - (void)copyURL:(const GURL&)URL { Please rename to storeURLInPasteboard or something, because copy is a keyword in a method name under ARC, and also this name is not very descriptive. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:407: DCHECK(URL.is_valid()); I think this is the fourth time in our code base that this snippet is used. Consider factoring it out in a separate file and exposing an API that just puts a GURL to pasteboard.
https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:71: - (void)stopObservingReadingListModel; like we discussed offline, we can probably remove this method https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:232: [weakSelf openInNewTabEntry:entry We are supposed to use a strongSelf. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:232: [weakSelf openInNewTabEntry:entry The object pointed to by |entry| may have been deleted when this block is ran. The simplest fix is to have the blocks use (and pass) a copy of the |entry|'s URL(). https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:287: [readingListViewController reloadData]; Do we really need to call reloadData?
Thanks, PTAL. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/app/strings/... File ios/chrome/app/strings/ios_strings.grd (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/app/strings/... ios/chrome/app/strings/ios_strings.grd:1075: View Offline Version On 2017/01/30 10:34:53, stkhapugin wrote: > Please add a length in em Done. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:24: // toolbar, dismissing the Reading List View and opening elements. On 2017/01/30 10:34:53, stkhapugin wrote: > This protocol is becoming bloated, consider splitting into separate protocols > for toolbar and for everything else. For me it is still OK. But on a more general subject, yes this class should be refactored. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:36: // Displays a context menu attach to an element of the ReadingList View. On 2017/01/30 10:34:53, stkhapugin wrote: > I think your delegate displays a context menu, but it's the delegate's > implementation detail. Instead add a comment explaining when this gesture > recognizer is recognized. Done. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:241: hasItems:self.readingListModel->size() > 0]; On 2017/01/30 10:34:53, stkhapugin wrote: > nit: parenthesis around hasItems: argument Done. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:20: // Container for the Reading List View Controller. On 2017/01/30 10:34:53, stkhapugin wrote: > This "container" is a few hundreds LOC. Please document its real purpose - > acting as RLVC delegate, displaying toolbars, displaying pop-ups etc. > > In fact, I'd argue that this class should be renamed to RLVC, and RLVC -> > RLCollectionViewController. If you choose to rename this, please do it in a > separate CL to preserve diffs. I was thinking of reusing the history pattern which is a HistoryPanelViewController instead of the ReadingListViewControllerContainer and a HistoryCollectionViewController instead of the ReadingListViewController. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:20: // Container for the Reading List View Controller. On 2017/01/30 10:34:53, stkhapugin wrote: > This "container" is a few hundreds LOC. Please document its real purpose - > acting as RLVC delegate, displaying toolbars, displaying pop-ups etc. > > In fact, I'd argue that this class should be renamed to RLVC, and RLVC -> > RLCollectionViewController. If you choose to rename this, please do it in a > separate CL to preserve diffs. Done. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:179: if (gestureRecognizer.numberOfTouches != 1 || duringEdit || On 2017/01/30 10:34:53, stkhapugin wrote: > Instead configure numberOfTouchesRequired when you create |gestureRecognizer|. > Also, you really shouldn't be concerned with internals of this gesture > recognizer here. Instead just expose a delegate API: > > -rlvc:(RLVC*)sender longPressOnItem:(RLCollectionViewItem*)item > duringEdit:(BOOL)edit. > > This way you can hide everything related to the collection view in the > collection view controller and not expose it at all. The idea was to move as much intelligence as possible in the delegate. The initial idea was to use the responder chain to let this class handle the gesture directly. As it is not possible to use the responder chain with a gesture recognizer, I used the delegate pattern to forward the call. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:232: [weakSelf openInNewTabEntry:entry On 2017/01/30 17:33:27, jif wrote: > We are supposed to use a strongSelf. For one function call? stkhapugin@: as ARC expert, what do you think? :) https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:232: [weakSelf openInNewTabEntry:entry On 2017/01/30 17:33:27, jif wrote: > The object pointed to by |entry| may have been deleted when this block is ran. > The simplest fix is to have the blocks use (and pass) a copy of the |entry|'s > URL(). Done. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:287: [readingListViewController reloadData]; On 2017/01/30 17:33:27, jif wrote: > Do we really need to call reloadData? This happens when the entry does not exist in the model but is present on the UI. It should not happen this the current implementation. In that case, reloading the UI to let the user know that the entry does not exist anymore seems better than doing nothing but keeping the entry on screen. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:341: [self.presentingViewController dismissViewControllerAnimated:YES On 2017/01/30 10:34:53, stkhapugin wrote: > Ideally view controllers should not dismiss themselves, instead calling the > presenting VC through a delegate. Yes. This will change when the use of coordinators is more widespread. For now I think this is OK. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:406: - (void)copyURL:(const GURL&)URL { On 2017/01/30 10:34:53, stkhapugin wrote: > Please rename to storeURLInPasteboard or something, because copy is a keyword in > a method name under ARC, and also this name is not very descriptive. Done. https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:407: DCHECK(URL.is_valid()); On 2017/01/30 10:34:53, stkhapugin wrote: > I think this is the fourth time in our code base that this snippet is used. > Consider factoring it out in a separate file and exposing an API that just puts > a GURL to pasteboard. Done.
https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:232: [weakSelf openInNewTabEntry:entry On 2017/01/31 09:35:40, gambard wrote: > On 2017/01/30 17:33:27, jif wrote: > > The object pointed to by |entry| may have been deleted when this block is ran. > > The simplest fix is to have the blocks use (and pass) a copy of the |entry|'s > > URL(). > > Done. __weak id weakSelf = self; // No problem. When -foo is called, either weakSelf is nil or not. auto a = ^() { [weakSelf foo]; }; // Possibly problematic - self might be released between -foo and -bar calls, which _may_ be unexpected. In this case, -foo is executed, but not -bar. auto b = ^() { [weakSelf foo]; [weakSelf bar]; }; // Same as above, but -foo and -bar are now guaranteed to be called simultaneously. auto fixed_b = ^(){ id strongSelf = weakSelf; [strongSelf foo]; [strongSelf bar]; }; Technically, in this particular case, not even using a _weak at all will still yield correct code. But you should still use __weak anyway.
https://codereview.chromium.org/2659693004/diff/60001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/60001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:36: // Signals that a long press have been recognized. s/have/has/ https://codereview.chromium.org/2659693004/diff/60001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/60001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:231: [weakSelf openInNewTabURL:entryURL Objects used in blocks by reference are not copied either. So basically entryURL should be a stack-based object: const GURL entryURL = entry->URL();
Thanks, PTAL. https://codereview.chromium.org/2659693004/diff/60001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/60001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:36: // Signals that a long press have been recognized. On 2017/01/31 17:43:39, jif wrote: > s/have/has/ Done. https://codereview.chromium.org/2659693004/diff/60001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/60001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:231: [weakSelf openInNewTabURL:entryURL On 2017/01/31 17:43:39, jif wrote: > Objects used in blocks by reference are not copied either. > > So basically entryURL should be a stack-based object: > const GURL entryURL = entry->URL(); Done. I am starting to really dislike C++ object retention in block :)
lgtm
gambard@chromium.org changed reviewers: + olivierrobin@chromium.org
+ olivierrobin for owners lgtm
Adding notes from an offline discussion. I'll still be happy if you refactor this (and if necessary, provide proper standalone coordinators) while your memory of reading list is fresh :) https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/40001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:179: if (gestureRecognizer.numberOfTouches != 1 || duringEdit || On 2017/01/31 09:35:40, gambard wrote: > On 2017/01/30 10:34:53, stkhapugin wrote: > > Instead configure numberOfTouchesRequired when you create |gestureRecognizer|. > > Also, you really shouldn't be concerned with internals of this gesture > > recognizer here. Instead just expose a delegate API: > > > > -rlvc:(RLVC*)sender longPressOnItem:(RLCollectionViewItem*)item > > duringEdit:(BOOL)edit. > > > > This way you can hide everything related to the collection view in the > > collection view controller and not expose it at all. > > The idea was to move as much intelligence as possible in the delegate. > The initial idea was to use the responder chain to let this class handle the > gesture directly. As it is not possible to use the responder chain with a > gesture recognizer, I used the delegate pattern to forward the call. Per offline discussion: CollectionViewController should abstract out the gesture recognizer and instead expose a callback that passes the collection view item or index path directly.
https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/clipboard_util.mm (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/clipboard_util.mm:14: void StoreURLInPasteboard(const GURL& URL) { nit: choose pasteboard or clipboard for name of method and file. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:38: ReadingListViewController* collectionController; super optional nit: I would have viewController in the name. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:200: // Do trigger context menu on headers. do not ? https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:262: entryURL:entryURL format? https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:361: base::RecordAction(base::UserMetricsAction("MobileReadingListOpen")); Is it the same metric? Should can we have only one method with incognito parameter? https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:386: readingListViewController.readingListModel->SetReadStatus(entryURL, true); Maybe not for this CL. This code should be moved to ReadingListWebStateObserver. If pageloaded is SUCCEDD and URL is offlineURL, mark entry read. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm:109: // openItemAtIndexPath opens the correct entry. You have only one entry. What would be incorrect entry?
https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:67: fromReadingListViewController: Why do you need fromReadingListViewController? https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:204: ReadingListCollectionViewItem* readingListItem = could you pass the directly the touchedItem or the readingListItem instead of computing it here? That method could be readingListViewController:requestContextMenuOnItem: https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:279: ReadingListCollectionViewItem* readingListItem = nit: Same here, should this be openItem?
Thanks, PTAL. I have added some metrics. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/c... File ios/chrome/browser/ui/clipboard_util.mm (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/c... ios/chrome/browser/ui/clipboard_util.mm:14: void StoreURLInPasteboard(const GURL& URL) { On 2017/02/01 12:22:05, Olivier Robin wrote: > nit: > choose pasteboard or clipboard for name of method and file. Done. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:38: ReadingListViewController* collectionController; On 2017/02/01 12:22:05, Olivier Robin wrote: > super optional nit: I would have viewController in the name. Done. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:67: fromReadingListViewController: On 2017/02/01 13:03:35, Olivier Robin wrote: > Why do you need fromReadingListViewController? Done. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:200: // Do trigger context menu on headers. On 2017/02/01 12:22:05, Olivier Robin wrote: > do not ? Done. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:204: ReadingListCollectionViewItem* readingListItem = On 2017/02/01 13:03:35, Olivier Robin wrote: > could you pass the directly the touchedItem or the readingListItem instead of > computing it here? > That method could be > > readingListViewController:requestContextMenuOnItem: Done. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:262: entryURL:entryURL On 2017/02/01 12:22:05, Olivier Robin wrote: > format? This is git cl format... https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:279: ReadingListCollectionViewItem* readingListItem = On 2017/02/01 13:03:35, Olivier Robin wrote: > nit: Same here, should this be openItem? Done. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:386: readingListViewController.readingListModel->SetReadStatus(entryURL, true); On 2017/02/01 12:22:05, Olivier Robin wrote: > Maybe not for this CL. > This code should be moved to ReadingListWebStateObserver. > If pageloaded is SUCCEDD and URL is offlineURL, mark entry read. Acknowledged. https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm:109: // openItemAtIndexPath opens the correct entry. On 2017/02/01 12:22:05, Olivier Robin wrote: > You have only one entry. > What would be incorrect entry? The "correct entry" was about the reading list view controller.
Thanks for cleaning up the API! LGTM with nit: https://codereview.chromium.org/2659693004/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:40: displayContextMenuForItem:(ReadingListCollectionViewItem*)readingListItem Add a comment for |menuLocation|, specifically which coordinate system are you providing this point in. I imagine it's in readingListVC.view coordinates? This may be important if reading list is not shown full-screen, like today, but in a popover, for example, while its container is fullscreen.
LGTM two comments https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:361: base::RecordAction(base::UserMetricsAction("MobileReadingListOpen")); On 2017/02/01 12:22:05, Olivier Robin wrote: > Is it the same metric? > Should can we have only one method with incognito parameter? Same question? https://codereview.chromium.org/2659693004/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:21: @class ReadingListCollectionViewItem; nit alphabetical order.
gambard@chromium.org changed reviewers: + jwd@chromium.org, marq@chromium.org
marq: PTAL for ios/chrome/browser/ui jwd: PTAL for histograms https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/80001/ios/chrome/browser/ui/r... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:361: base::RecordAction(base::UserMetricsAction("MobileReadingListOpen")); On 2017/02/02 13:49:47, Olivier Robin wrote: > On 2017/02/01 12:22:05, Olivier Robin wrote: > > Is it the same metric? > > Should can we have only one method with incognito parameter? > > Same question? Done. https://codereview.chromium.org/2659693004/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:21: @class ReadingListCollectionViewItem; On 2017/02/02 13:49:47, Olivier Robin wrote: > nit alphabetical order. Done. https://codereview.chromium.org/2659693004/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:40: displayContextMenuForItem:(ReadingListCollectionViewItem*)readingListItem On 2017/02/02 13:41:54, stkhapugin wrote: > Add a comment for |menuLocation|, specifically which coordinate system are you > providing this point in. I imagine it's in readingListVC.view coordinates? This > may be important if reading list is not shown full-screen, like today, but in a > popover, for example, while its container is fullscreen. Done.
lgtm
In addition to my comments below, please keep CLs smaller than this. In this case, factoring the pasteboard code out should have been a separate CL, and metrics should have been added in a CL after this one. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:71: @property(nonatomic, assign) BOOL shouldMonitorModel; This seems like an implementation detail that shouldn't be exposed. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:81: ReadingListToolbar* _toolbar; I'm confused; I thought the point of the containing view controller was to decouple the toolbar and list. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:627: if (gestureRecognizer.numberOfTouches != 1 || self.editor.editing || Configure |numberOfTouchesRequired| when you create the gesture recognizer instead of checking here, unless there is a case where the number of touches can change during the lifetime of the recognizer. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_builder.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_builder.mm:19: @implementation ReadingListViewControllerBuilder This seems like it should be a class method on ReadingListViewControllerContainer. If factoring out into a separate file helps keep dependencies sane, it can be in a category. Otherwise, just make it a free function. We shouldn't use Objective-C classes just for namespaces. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:23: @interface ReadingListViewControllerContainer Please rename to 'ReadingListContainerViewController', so that it's clear this is object is itself a view controller. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:38: ReadingListViewController* readingListViewController; Why does this need to be public? https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:68: // Alert displayed when the user long press an entry. Nit: this is the coordinator for the alert, not the alert itself. Also s/long press/long presses/ https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:77: // Opens |entry| in a new tab. |entry| isn't a parameter name in the method definition. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:78: - (void)openInNewTabURL:(const GURL&)URL incognito:(BOOL)incognito; -openNewTabWithURL:incognito: https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:79: // Opens the offline version of |entry|. |entry| isn't a parameter name in the method definition. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:80: - (void)openOfflineURL:(const GURL&)URL The method name and comments don't make it clear what the difference between |URL| and |entryURL| is. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:130: // This constraints is not required. It will be activated only when the nit: s/constraints/constraint/ https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:131: // toolbar is not present, allowing the collection to take the whole page. Wording nit: Saying this constraint will only be activated when the toolbar isn't present implies that the constraint's |active| property will vary based on the toolbar being present or not. Maybe: "This constraint has a low priority so it will only take effect when the toolbar isn't present, ..." https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:134: constraintEqualToAnchor:[self view].bottomAnchor]; Please use dot notation for property access: self.readingListViewController.view self.view (Or, if you don't want to do that, please be consistent in the file and s/self.view/[self view]/ everywhere.) https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:160: @"collection" : [self.readingListViewController view] self.readingListViewController, or the readingListViewController that was passed in? https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:275: readingListViewController.readingListModel->GetEntryByURL( Why doesn't |readingListViewController| just look up and pass |entry| instead of having a delegate do it? https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:295: - (void)markPressed { Action methods should indicate the action being taken, not the UI interaction that was made. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:354: readingListViewController.shouldMonitorModel = NO; Why does this need to be done here but not in -dismiss? Why pass in |readingListViewController| when it's a property owned by this object? https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm:147: TEST_F(ReadingListViewControllerContainerTest, OpenItem) { 100+ lines of set-up for a 20-line test is a code smell. What makes this so complicated?
Thanks, PTAL. The CL grew too big patch by patch (the first one is ~350 lines)... We should have split it before it became out of control https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.h (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.h:71: @property(nonatomic, assign) BOOL shouldMonitorModel; On 2017/02/03 11:58:19, marq wrote: > This seems like an implementation detail that shouldn't be exposed. Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:81: ReadingListToolbar* _toolbar; On 2017/02/03 11:58:19, marq wrote: > I'm confused; I thought the point of the containing view controller was to > decouple the toolbar and list. The containing view only allows the toolbar to be below the collection view and not at the bottom of it. Refactoring is coming, by moving to the coordinator/VC model. The decoupling toolbar/collection will be done at that moment. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:627: if (gestureRecognizer.numberOfTouches != 1 || self.editor.editing || On 2017/02/03 11:58:19, marq wrote: > Configure |numberOfTouchesRequired| when you create the gesture recognizer > instead of checking here, unless there is a case where the number of touches can > change during the lifetime of the recognizer. Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_builder.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_builder.mm:19: @implementation ReadingListViewControllerBuilder On 2017/02/03 11:58:19, marq wrote: > This seems like it should be a class method on > ReadingListViewControllerContainer. If factoring out into a separate file helps > keep dependencies sane, it can be in a category. Otherwise, just make it a free > function. We shouldn't use Objective-C classes just for namespaces. It will be done in a future CL. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:23: @interface ReadingListViewControllerContainer On 2017/02/03 11:58:19, marq wrote: > Please rename to 'ReadingListContainerViewController', so that it's clear this > is object is itself a view controller. I a future CL. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:38: ReadingListViewController* readingListViewController; On 2017/02/03 11:58:19, marq wrote: > Why does this need to be public? Modified https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:68: // Alert displayed when the user long press an entry. On 2017/02/03 11:58:20, marq wrote: > Nit: this is the coordinator for the alert, not the alert itself. > > Also s/long press/long presses/ Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:77: // Opens |entry| in a new tab. On 2017/02/03 11:58:20, marq wrote: > |entry| isn't a parameter name in the method definition. Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:78: - (void)openInNewTabURL:(const GURL&)URL incognito:(BOOL)incognito; On 2017/02/03 11:58:20, marq wrote: > -openNewTabWithURL:incognito: Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:79: // Opens the offline version of |entry|. On 2017/02/03 11:58:20, marq wrote: > |entry| isn't a parameter name in the method definition. Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:80: - (void)openOfflineURL:(const GURL&)URL On 2017/02/03 11:58:20, marq wrote: > The method name and comments don't make it clear what the difference between > |URL| and |entryURL| is. Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:130: // This constraints is not required. It will be activated only when the On 2017/02/03 11:58:20, marq wrote: > nit: s/constraints/constraint/ Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:131: // toolbar is not present, allowing the collection to take the whole page. On 2017/02/03 11:58:20, marq wrote: > Wording nit: Saying this constraint will only be activated when the toolbar > isn't present implies that the constraint's |active| property will vary based on > the toolbar being present or not. > > Maybe: "This constraint has a low priority so it will only take effect when the > toolbar isn't present, ..." Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:134: constraintEqualToAnchor:[self view].bottomAnchor]; On 2017/02/03 11:58:20, marq wrote: > Please use dot notation for property access: > > self.readingListViewController.view > self.view > > (Or, if you don't want to do that, please be consistent in the file and > s/self.view/[self view]/ everywhere.) Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:160: @"collection" : [self.readingListViewController view] On 2017/02/03 11:58:20, marq wrote: > self.readingListViewController, or the readingListViewController that was passed > in? Done. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:275: readingListViewController.readingListModel->GetEntryByURL( On 2017/02/03 11:58:20, marq wrote: > Why doesn't |readingListViewController| just look up and pass |entry| instead of > having a delegate do it? In future refactoring I want to remove the model from the readinglistviewcontroller to move it in the coordinator. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:295: - (void)markPressed { On 2017/02/03 11:58:20, marq wrote: > Action methods should indicate the action being taken, not the UI interaction > that was made. In future CL. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:354: readingListViewController.shouldMonitorModel = NO; On 2017/02/03 11:58:20, marq wrote: > Why does this need to be done here but not in -dismiss? > > Why pass in |readingListViewController| when it's a property owned by this > object? Changed. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm:147: TEST_F(ReadingListViewControllerContainerTest, OpenItem) { On 2017/02/03 11:58:20, marq wrote: > 100+ lines of set-up for a 20-line test is a code smell. What makes this so > complicated? An objective C mock object taking a C++ object as parameter: 50 lines. C++ mock: 20 lines Fixture for the test: 30 lines. I you think I can remove thins, please tell me.
https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller.mm:81: ReadingListToolbar* _toolbar; On 2017/02/03 13:17:26, gambard wrote: > On 2017/02/03 11:58:19, marq wrote: > > I'm confused; I thought the point of the containing view controller was to > > decouple the toolbar and list. > > The containing view only allows the toolbar to be below the collection view and > not at the bottom of it. > Refactoring is coming, by moving to the coordinator/VC model. The decoupling > toolbar/collection will be done at that moment. Acknowledged. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_builder.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_builder.mm:19: @implementation ReadingListViewControllerBuilder On 2017/02/03 13:17:26, gambard wrote: > On 2017/02/03 11:58:19, marq wrote: > > This seems like it should be a class method on > > ReadingListViewControllerContainer. If factoring out into a separate file > helps > > keep dependencies sane, it can be in a category. Otherwise, just make it a > free > > function. We shouldn't use Objective-C classes just for namespaces. > > It will be done in a future CL. Acknowledged. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:23: @interface ReadingListViewControllerContainer On 2017/02/03 13:17:26, gambard wrote: > On 2017/02/03 11:58:19, marq wrote: > > Please rename to 'ReadingListContainerViewController', so that it's clear this > > is object is itself a view controller. > > I a future CL. Acknowledged. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:38: ReadingListViewController* readingListViewController; On 2017/02/03 13:17:26, gambard wrote: > On 2017/02/03 11:58:19, marq wrote: > > Why does this need to be public? > > Modified I don't see any change? https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:354: readingListViewController.shouldMonitorModel = NO; On 2017/02/03 13:17:26, gambard wrote: > On 2017/02/03 11:58:20, marq wrote: > > Why does this need to be done here but not in -dismiss? > > > > Why pass in |readingListViewController| when it's a property owned by this > > object? > > Changed. Still not clear on why |readingListViewController| is passed in here. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm:147: TEST_F(ReadingListViewControllerContainerTest, OpenItem) { On 2017/02/03 13:17:26, gambard wrote: > On 2017/02/03 11:58:20, marq wrote: > > 100+ lines of set-up for a 20-line test is a code smell. What makes this so > > complicated? > > An objective C mock object taking a C++ object as parameter: 50 lines. > C++ mock: 20 lines > Fixture for the test: 30 lines. > > I you think I can remove thins, please tell me. No, I don't have any easy answers, I'm just looking at how complicated our test set-up is. Nothing to change for this CL, I think. https://codereview.chromium.org/2659693004/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:79: // Opens the offline url |offlineURL| of the entry save in the reading list save -> saved https://codereview.chromium.org/2659693004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:246: entry->DistilledPath(), entry->URL(), entry->DistilledURL()); use entryURL here instead of entry->URL(). (and, not for this CL, but this seems like it should be a method on ReadingListEntry).
Thanks, PTAL. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h:38: ReadingListViewController* readingListViewController; On 2017/02/06 14:56:38, marq wrote: > On 2017/02/03 13:17:26, gambard wrote: > > On 2017/02/03 11:58:19, marq wrote: > > > Why does this need to be public? > > > > Modified > > I don't see any change? Was used in test. Removed. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:354: readingListViewController.shouldMonitorModel = NO; On 2017/02/06 14:56:38, marq wrote: > On 2017/02/03 13:17:26, gambard wrote: > > On 2017/02/03 11:58:20, marq wrote: > > > Why does this need to be done here but not in -dismiss? > > > > > > Why pass in |readingListViewController| when it's a property owned by this > > > object? > > > > Changed. > > Still not clear on why |readingListViewController| is passed in here. Because this is a method of the delegate. The delegate might not be owner of the readingListViewController. https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container_unittest.mm:147: TEST_F(ReadingListViewControllerContainerTest, OpenItem) { On 2017/02/06 14:56:38, marq wrote: > On 2017/02/03 13:17:26, gambard wrote: > > On 2017/02/03 11:58:20, marq wrote: > > > 100+ lines of set-up for a 20-line test is a code smell. What makes this so > > > complicated? > > > > An objective C mock object taking a C++ object as parameter: 50 lines. > > C++ mock: 20 lines > > Fixture for the test: 30 lines. > > > > I you think I can remove thins, please tell me. > > No, I don't have any easy answers, I'm just looking at how complicated our test > set-up is. Nothing to change for this CL, I think. Acknowledged. https://codereview.chromium.org/2659693004/diff/160001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:79: // Opens the offline url |offlineURL| of the entry save in the reading list On 2017/02/06 14:56:38, marq wrote: > save -> saved Done. https://codereview.chromium.org/2659693004/diff/160001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:246: entry->DistilledPath(), entry->URL(), entry->DistilledURL()); On 2017/02/06 14:56:38, marq wrote: > use entryURL here instead of entry->URL(). > > (and, not for this CL, but this seems like it should be a method on > ReadingListEntry). Done. olivierrobin was probably the one choosing the location of this method.
lgtm https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:354: readingListViewController.shouldMonitorModel = NO; On 2017/02/06 15:18:54, gambard wrote: > On 2017/02/06 14:56:38, marq wrote: > > On 2017/02/03 13:17:26, gambard wrote: > > > On 2017/02/03 11:58:20, marq wrote: > > > > Why does this need to be done here but not in -dismiss? > > > > > > > > Why pass in |readingListViewController| when it's a property owned by this > > > > object? > > > > > > Changed. > > > > Still not clear on why |readingListViewController| is passed in here. > > Because this is a method of the delegate. The delegate might not be owner of the > readingListViewController. OK, so this method and openInNewTabURL:incognito: are conceptually part of the delegate implementation, not the view controller?
Thanks! https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm (right): https://codereview.chromium.org/2659693004/diff/140001/ios/chrome/browser/ui/... ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm:354: readingListViewController.shouldMonitorModel = NO; On 2017/02/06 15:32:05, marq wrote: > On 2017/02/06 15:18:54, gambard wrote: > > On 2017/02/06 14:56:38, marq wrote: > > > On 2017/02/03 13:17:26, gambard wrote: > > > > On 2017/02/03 11:58:20, marq wrote: > > > > > Why does this need to be done here but not in -dismiss? > > > > > > > > > > Why pass in |readingListViewController| when it's a property owned by > this > > > > > object? > > > > > > > > Changed. > > > > > > Still not clear on why |readingListViewController| is passed in here. > > > > Because this is a method of the delegate. The delegate might not be owner of > the > > readingListViewController. > > OK, so this method and openInNewTabURL:incognito: are conceptually part of the > delegate implementation, not the view controller? Yes, they are only called through delegate methods.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org, olivierrobin@chromium.org, stkhapugin@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2659693004/#ps180001 (title: "Address comments")
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": 180001, "attempt_start_ts": 1486397747913930,
"parent_rev": "723d2e3a147926e82eb430db12585c2f97ca6470", "commit_rev":
"6a13836f0b66471a08f88e02fdaba0c7671f83e6"}
Message was sent while issue was closed.
Description was changed from ========== Add context menu when long press on a reading list entry This CL adds a context menu with different actions when a long press on a reading list entry occurs. BUG=685230 ========== to ========== Add context menu when long press on a reading list entry This CL adds a context menu with different actions when a long press on a reading list entry occurs. BUG=685230 Review-Url: https://codereview.chromium.org/2659693004 Cr-Commit-Position: refs/heads/master@{#448282} Committed: https://chromium.googlesource.com/chromium/src/+/6a13836f0b66471a08f88e02fdab... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/6a13836f0b66471a08f88e02fdab... |
