Add context menu when long press on a reading list entry

ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm

Index: ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm
diff --git a/ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm b/ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm
index 5da64ccf4d6905a3260a8ef48439e1b92413bfbe..02f93f2d2179cf9508f719fa8d2898f62cb9b34c 100644
--- a/ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm
+++ b/ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.mm
@@ -4,10 +4,28 @@
 
 #import "ios/chrome/browser/ui/reading_list/reading_list_view_controller_container.h"
 
-#import "ios/chrome/browser/ui/uikit_ui_util.h"
+#import <MobileCoreServices/MobileCoreServices.h>
+
+#import "base/mac/foundation_util.h"
+#include "base/metrics/user_metrics.h"
+#include "base/metrics/user_metrics_action.h"
+#include "base/strings/sys_string_conversions.h"
+#include "components/reading_list/ios/reading_list_model.h"
+#include "ios/chrome/browser/reading_list/offline_url_utils.h"
+#import "ios/chrome/browser/ui/alert_coordinator/action_sheet_coordinator.h"
+#import "ios/chrome/browser/ui/collection_view/cells/collection_view_item.h"
+#import "ios/chrome/browser/ui/collection_view/collection_view_model.h"
 #import "ios/chrome/browser/ui/keyboard/UIKeyCommand+Chrome.h"
+#import "ios/chrome/browser/ui/reading_list/reading_list_collection_view_item.h"
 #import "ios/chrome/browser/ui/reading_list/reading_list_toolbar.h"
 #import "ios/chrome/browser/ui/reading_list/reading_list_view_controller.h"
+#import "ios/chrome/browser/ui/uikit_ui_util.h"
+#import "ios/chrome/browser/ui/url_loader.h"
+#include "ios/chrome/grit/ios_strings.h"
+#include "ios/web/public/navigation_manager.h"
+#import "net/base/mac/url_conversions.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/strings/grit/ui_strings.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
 #error "This file requires ARC support."
@@ -30,31 +48,55 @@ typedef NS_ENUM(NSInteger, LayoutPriority) {
     ReadingListToolbarHeightDelegate> {
   // Toolbar with the actions.
   ReadingListToolbar* _toolbar;
-  ReadingListViewController* _collectionController;
   // This constraint control the expanded mode of the toolbar.
   NSLayoutConstraint* _expandedToolbarConstraint;
+  // Alert displayed when the user long press an entry.
+  AlertCoordinator* _alertCoordinator;
 }
 
+// UrlLoader for navigating to entries.
+@property(nonatomic, weak, readonly) id<UrlLoader> URLLoader;
+
+// Closes the ReadingList view.
+- (void)dismiss;
+// Opens |entry| in a new tab.
+- (void)openInNewTabEntry:(const ReadingListEntry*)entry
+    fromReadingListViewController:
+        (ReadingListViewController*)readingListViewController;
+// Opens |entry| in a new incognito tab.
+- (void)openInNewIncognitoTabEntry:(const ReadingListEntry*)entry
+     fromReadingListViewController:
+         (ReadingListViewController*)readingListViewController;
+// Opens the offline version of |entry|.
+- (void)openOfflineEntry:(const ReadingListEntry*)entry
+    fromReadingListViewController:
+        (ReadingListViewController*)readingListViewController;
+// Copies |URL| in the pasteboard.
+- (void)copyURL:(const GURL&)URL;
+
 @end
 
 @implementation ReadingListViewControllerContainer
 
+@synthesize collectionController = _collectionController;
+@synthesize URLLoader = _URLLoader;
+
 - (instancetype)initWithModel:(ReadingListModel*)model
-                      tabModel:(TabModel*)tabModel
+                        loader:(id<UrlLoader>)loader
               largeIconService:(favicon::LargeIconService*)largeIconService
     readingListDownloadService:
         (ReadingListDownloadService*)readingListDownloadService {
   self = [super initWithNibName:nil bundle:nil];
   if (self) {
+    _URLLoader = loader;
     _toolbar = [[ReadingListToolbar alloc] initWithFrame:CGRectZero];
     _toolbar.heightDelegate = self;
     _collectionController = [[ReadingListViewController alloc]
                      initWithModel:model
-                          tabModel:tabModel
                   largeIconService:largeIconService
         readingListDownloadService:readingListDownloadService
                            toolbar:_toolbar];
-    _collectionController.audience = self;
+    _collectionController.delegate = self;
 
     // Configure modal presentation.
     [self setModalPresentationStyle:UIModalPresentationFormSheet];
@@ -64,22 +106,23 @@ typedef NS_ENUM(NSInteger, LayoutPriority) {
 }
 
 - (void)viewDidLoad {
-  [self addChildViewController:_collectionController];
-  [self.view addSubview:[_collectionController view]];
-  [_collectionController didMoveToParentViewController:self];
+  [self addChildViewController:self.collectionController];
+  [self.view addSubview:[self.collectionController view]];
+  [self.collectionController didMoveToParentViewController:self];
 
   [_toolbar setTranslatesAutoresizingMaskIntoConstraints:NO];
-  [[_collectionController view]
+  [[self.collectionController view]
       setTranslatesAutoresizingMaskIntoConstraints:NO];
 
-  NSDictionary* views = @{ @"collection" : [_collectionController view] };
+  NSDictionary* views = @{ @"collection" : [self.collectionController view] };
   NSArray* constraints = @[ @"V:|[collection]", @"H:|[collection]|" ];
   ApplyVisualConstraints(constraints, views);
 
   // This constraints is not required. It will be activated only when the
   // toolbar is not present, allowing the collection to take the whole page.
-  NSLayoutConstraint* constraint = [[_collectionController view].bottomAnchor
-      constraintEqualToAnchor:[self view].bottomAnchor];
+  NSLayoutConstraint* constraint =
+      [[self.collectionController view].bottomAnchor
+          constraintEqualToAnchor:[self view].bottomAnchor];
   constraint.priority = LayoutPriorityLow;
   constraint.active = YES;
 }
@@ -95,15 +138,17 @@ typedef NS_ENUM(NSInteger, LayoutPriority) {
   return YES;
 }
 
-#pragma mark - ReadingListViewControllerAudience
+#pragma mark - ReadingListViewControllerDelegate
 
-- (void)setCollectionHasItems:(BOOL)hasItems {
+- (void)readingListViewController:
+            (ReadingListViewController*)readingListViewController
+                         hasItems:(BOOL)hasItems {
   if (hasItems) {
     // If there are items, add the toolbar.
     [self.view addSubview:_toolbar];
     NSDictionary* views = @{
       @"toolbar" : _toolbar,
-      @"collection" : [_collectionController view]
+      @"collection" : [self.collectionController view]
     };
     NSArray* constraints = @[ @"V:[collection][toolbar]|", @"H:|[toolbar]|" ];
     ApplyVisualConstraints(constraints, views);
@@ -122,27 +167,156 @@ typedef NS_ENUM(NSInteger, LayoutPriority) {
   }
 }
 
-- (void)dismiss {
-  [self.presentingViewController dismissViewControllerAnimated:YES
-                                                    completion:nil];
+- (void)dismissReadingListViewController:
+    (ReadingListViewController*)readingListViewController {
+  [self dismiss];
+}
+
+- (void)
+readingListViewController:(ReadingListViewController*)readingListViewController
+    didRecognizeLongPress:(UILongPressGestureRecognizer*)gestureRecognizer
+               duringEdit:(BOOL)duringEdit {
+  if (gestureRecognizer.numberOfTouches != 1 || duringEdit ||

[stkhapugin, 2017/01/30 10:34:53]

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. 

[gambard, 2017/01/31 09:35:40]

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.

[stkhapugin, 2017/02/01 10:42:44]

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. 

+      gestureRecognizer.state != UIGestureRecognizerStateBegan) {
+    return;
+  }
+
+  CGPoint touchLocation = [gestureRecognizer
+      locationOfTouch:0
+               inView:readingListViewController.collectionView];
+  NSIndexPath* touchedItemIndexPath = [readingListViewController.collectionView
+      indexPathForItemAtPoint:touchLocation];
+  if (!touchedItemIndexPath ||
+      ![readingListViewController.collectionViewModel
+          hasItemAtIndexPath:touchedItemIndexPath]) {
+    // Make sure there is an item at this position.
+    return;
+  }
+  CollectionViewItem* touchedItem =
+      [readingListViewController.collectionViewModel
+          itemAtIndexPath:touchedItemIndexPath];
+
+  if (touchedItem == [readingListViewController.collectionViewModel
+                         headerForSection:touchedItemIndexPath.section] ||
+      ![touchedItem isKindOfClass:[ReadingListCollectionViewItem class]]) {
+    // Do trigger context menu on headers.
+    return;
+  }
+
+  ReadingListCollectionViewItem* readingListItem =
+      base::mac::ObjCCastStrict<ReadingListCollectionViewItem>(touchedItem);
+  const ReadingListEntry* entry =
+      readingListViewController.readingListModel->GetEntryByURL(
+          readingListItem.url);
+
+  if (!entry) {
+    [readingListViewController reloadData];
+    return;
+  }
+
+  __weak ReadingListViewControllerContainer* weakSelf = self;
+
+  _alertCoordinator = [[ActionSheetCoordinator alloc]
+      initWithBaseViewController:self
+                           title:readingListItem.text
+                         message:readingListItem.detailText
+                            rect:CGRectMake(touchLocation.x, touchLocation.y, 0,
+                                            0)
+                            view:readingListViewController.collectionView];
+
+  NSString* openInNewTabTitle =
+      l10n_util::GetNSString(IDS_IOS_CONTENT_CONTEXT_OPENLINKNEWTAB);
+  [_alertCoordinator
+      addItemWithTitle:openInNewTabTitle
+                action:^{
+                  [weakSelf openInNewTabEntry:entry

[jif, 2017/01/30 17:33:27]

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().

[jif, 2017/01/30 17:33:27]

We are supposed to use a strongSelf.

[gambard, 2017/01/31 09:35:40]

For one function call?
stkhapugin@: as ARC expert, what do you think? :)

[gambard, 2017/01/31 09:35:40]

Done.

[stkhapugin, 2017/01/31 10:18:26]

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

+                      fromReadingListViewController:readingListViewController];
+                }
+                 style:UIAlertActionStyleDefault];
+
+  NSString* openInNewTabIncognitoTitle =
+      l10n_util::GetNSString(IDS_IOS_CONTENT_CONTEXT_OPENLINKNEWINCOGNITOTAB);
+  [_alertCoordinator
+      addItemWithTitle:openInNewTabIncognitoTitle
+                action:^{
+                  [weakSelf
+                         openInNewIncognitoTabEntry:entry
+                      fromReadingListViewController:readingListViewController];
+                }
+                 style:UIAlertActionStyleDefault];
+
+  NSString* copyLinkTitle =
+      l10n_util::GetNSString(IDS_IOS_CONTENT_CONTEXT_COPY);
+  [_alertCoordinator addItemWithTitle:copyLinkTitle
+                               action:^{
+                                 [weakSelf copyURL:readingListItem.url];
+                               }
+                                style:UIAlertActionStyleDefault];
+
+  if (entry->DistilledState() == ReadingListEntry::PROCESSED) {
+    NSString* viewOfflineVersionTitle =
+        l10n_util::GetNSString(IDS_IOS_READING_LIST_CONTENT_CONTEXT_OFFLINE);
+    [_alertCoordinator addItemWithTitle:viewOfflineVersionTitle
+                                 action:^{
+                                   [weakSelf openOfflineEntry:entry
+                                       fromReadingListViewController:
+                                           readingListViewController];
+                                 }
+                                  style:UIAlertActionStyleDefault];
+  }
+
+  [_alertCoordinator addItemWithTitle:l10n_util::GetNSString(IDS_APP_CANCEL)
+                               action:nil
+                                style:UIAlertActionStyleCancel];
+
+  [_alertCoordinator start];
+}
+
+- (void)readingListViewController:
+            (ReadingListViewController*)readingListViewController
+              openItemAtIndexPath:(NSIndexPath*)indexPath {
+  ReadingListCollectionViewItem* readingListItem =
+      base::mac::ObjCCastStrict<ReadingListCollectionViewItem>(
+          [readingListViewController.collectionViewModel
+              itemAtIndexPath:indexPath]);
+  const ReadingListEntry* entry =
+      readingListViewController.readingListModel->GetEntryByURL(
+          readingListItem.url);
+
+  if (!entry) {
+    [readingListViewController reloadData];

[jif, 2017/01/30 17:33:27]

Do we really need to call reloadData?

[gambard, 2017/01/31 09:35:40]

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.

+    return;
+  }
+
+  // Reset observer to prevent further model update notifications.
+  [readingListViewController stopObservingReadingListModel];
+
+  base::RecordAction(base::UserMetricsAction("MobileReadingListOpen"));
+
+  [self.URLLoader loadURL:entry->URL()
+                 referrer:web::Referrer()
+               transition:ui::PAGE_TRANSITION_AUTO_BOOKMARK
+        rendererInitiated:NO];
+
+  [self dismiss];
 }
 
 #pragma mark - ReadingListToolbarActionTarget
 
 - (void)markPressed {
-  [_collectionController markPressed];
+  [self.collectionController markPressed];
 }
 
 - (void)deletePressed {
-  [_collectionController deletePressed];
+  [self.collectionController deletePressed];
 }
 
 - (void)enterEditingModePressed {
-  [_collectionController enterEditingModePressed];
+  [self.collectionController enterEditingModePressed];
 }
 
 - (void)exitEditingModePressed {
-  [_collectionController exitEditingModePressed];
+  [self.collectionController exitEditingModePressed];
 }
 
 #pragma mark - ReadingListToolbarHeightDelegate
@@ -161,6 +335,85 @@ typedef NS_ENUM(NSInteger, LayoutPriority) {
   });
 }
 
+#pragma mark - Private
+
+- (void)dismiss {
+  [self.presentingViewController dismissViewControllerAnimated:YES

[stkhapugin, 2017/01/30 10:34:53]

Ideally view controllers should not dismiss themselves, instead calling the
presenting VC through a delegate. 

[gambard, 2017/01/31 09:35:40]

Yes. This will change when the use of coordinators is more widespread. For now I
think this is OK.

+                                                    completion:nil];
+}
+
+- (void)openInNewTabEntry:(const ReadingListEntry*)entry
+    fromReadingListViewController:
+        (ReadingListViewController*)readingListViewController {
+  // Reset observer to prevent further model update notifications.
+  [readingListViewController stopObservingReadingListModel];
+
+  base::RecordAction(base::UserMetricsAction("MobileReadingListOpen"));
+
+  [self.URLLoader webPageOrderedOpen:entry->URL()
+                            referrer:web::Referrer()
+                          windowName:nil
+                         inIncognito:NO
+                        inBackground:NO
+                            appendTo:kLastTab];
+
+  [self dismiss];
+}
+
+- (void)openInNewIncognitoTabEntry:(const ReadingListEntry*)entry
+     fromReadingListViewController:
+         (ReadingListViewController*)readingListViewController {
+  // Reset observer to prevent further model update notifications.
+  [readingListViewController stopObservingReadingListModel];
+
+  base::RecordAction(base::UserMetricsAction("MobileReadingListOpen"));
+
+  [self.URLLoader webPageOrderedOpen:entry->URL()
+                            referrer:web::Referrer()
+                          windowName:nil
+                         inIncognito:YES
+                        inBackground:NO
+                            appendTo:kLastTab];
+
+  [self dismiss];
+}
+
+- (void)openOfflineEntry:(const ReadingListEntry*)entry
+    fromReadingListViewController:
+        (ReadingListViewController*)readingListViewController {
+  if (entry->DistilledState() != ReadingListEntry::PROCESSED) {
+    return;
+  }
+
+  // Reset observer to prevent further model update notifications.
+  [readingListViewController stopObservingReadingListModel];
+
+  base::RecordAction(base::UserMetricsAction("MobileReadingListOpen"));
+
+  GURL url = reading_list::OfflineURLForPath(
+      entry->DistilledPath(), entry->URL(), entry->DistilledURL());
+
+  [self.URLLoader loadURL:url
+                 referrer:web::Referrer()
+               transition:ui::PAGE_TRANSITION_AUTO_BOOKMARK
+        rendererInitiated:NO];
+
+  readingListViewController.readingListModel->SetReadStatus(entry->URL(), true);
+
+  [self dismiss];
+}
+
+- (void)copyURL:(const GURL&)URL {

[stkhapugin, 2017/01/30 10:34:53]

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.

[gambard, 2017/01/31 09:35:40]

Done.

+  DCHECK(URL.is_valid());

[stkhapugin, 2017/01/30 10:34:53]

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.

[gambard, 2017/01/31 09:35:40]

Done.

+  NSData* plainText = [base::SysUTF8ToNSString(URL.spec())
+      dataUsingEncoding:NSUTF8StringEncoding];
+  NSDictionary* copiedItem = @{
+    (NSString*)kUTTypeURL : net::NSURLWithGURL(URL),
+    (NSString*)kUTTypeUTF8PlainText : plainText,
+  };
+  [[UIPasteboard generalPasteboard] setItems:@[ copiedItem ]];
+}
+
 #pragma mark - UIResponder
 
 - (NSArray*)keyCommands {