Add context menu for ContentSuggestions

Add context menu for ContentSuggestions

This CL adds a context menu for ContentSuggestions and a way to open the
article by tapping it.
The context menu does nothing for deletion for now.

BUG=692073
Review-Url: https://codereview.chromium.org/2691303002
Cr-Commit-Position: refs/heads/master@{#451602}
Committed: https://chromium.googlesource.com/chromium/src/+/9b6abde6cad9c609e4961e2149aafd328bfa19a5

[gambard, 2017-02-14 15:00:48]

gambard@chromium.org changed reviewers:
+ lpromero@chromium.org

[gambard, 2017-02-14 15:00:48]

PTAL.

[lpromero, 2017-02-14 16:18:18]

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/content_...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h
(right):

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/content_...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h:21:
loader:(id<UrlLoader>)loader
Can you instead make it a property?

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/content_...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/content_...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:33:
@property(nonatomic, weak, readonly) id<UrlLoader> UrlLoader;
This should be URLLoader. Consider renaming the protocol in a follow-up CL.

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_commands.h
(right):

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_commands.h:23: -
(void)openURL:(const GURL&)url;
Optional: s/url/URL

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_commands.h:24: //
Displays a context menu for opening the |url|.
There is no "url" here.

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm
(right):

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:214:
headerForSection:touchedItemIndexPath.section] ||
If you are pressing the header, how come the collection view returns an
indexPath and not nil?

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:216:
// Do not trigger context menu on headers.
This comment does not seem to talk about the second condition: !Article.

[gambard, 2017-02-15 10:37:56]

gambard@chromium.org changed reviewers:
+ stkhapugin@chromium.org

[gambard, 2017-02-15 10:37:57]

Thanks, PTAL.

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/content_...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h
(right):

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/content_...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h:21:
loader:(id<UrlLoader>)loader
On 2017/02/14 16:18:17, lpromero wrote:
> Can you instead make it a property?

Done.

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/content_...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/content_...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:33:
@property(nonatomic, weak, readonly) id<UrlLoader> UrlLoader;
On 2017/02/14 16:18:17, lpromero wrote:
> This should be URLLoader. Consider renaming the protocol in a follow-up CL.

It is widely used. I think renaming all url/URL/Url/UrL to conform one rule
might be too complicated.

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
File ios/chrome/browser/ui/content_suggestions/content_suggestions_commands.h
(right):

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_commands.h:23: -
(void)openURL:(const GURL&)url;
On 2017/02/14 16:18:17, lpromero wrote:
> Optional: s/url/URL

Done.

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_commands.h:24: //
Displays a context menu for opening the |url|.
On 2017/02/14 16:18:17, lpromero wrote:
> There is no "url" here.

Done.

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm
(right):

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:214:
headerForSection:touchedItemIndexPath.section] ||
On 2017/02/14 16:18:17, lpromero wrote:
> If you are pressing the header, how come the collection view returns an
> indexPath and not nil?

It is returning nil actually.
Should I let the test to be defensive?

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:216:
// Do not trigger context menu on headers.
On 2017/02/14 16:18:17, lpromero wrote:
> This comment does not seem to talk about the second condition: !Article.

Done.

[lpromero, 2017-02-15 10:48:29]

lgtm

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm
(right):

https://codereview.chromium.org/2691303002/diff/1/ios/chrome/browser/ui/conte...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:214:
headerForSection:touchedItemIndexPath.section] ||
On 2017/02/15 10:37:56, gambard wrote:
> On 2017/02/14 16:18:17, lpromero wrote:
> > If you are pressing the header, how come the collection view returns an
> > indexPath and not nil?
> 
> It is returning nil actually.
> Should I let the test to be defensive?

[self.collectionViewModel itemAtIndexPath:touchedItemIndexPath] can never return
a header.

[stkhapugin, 2017-02-15 16:03:36]

lgtm

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h
(right):

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h:22: //
URLLoader used to opens pages.
s/opens/open

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:60:
_suggestionsViewController = [[ContentSuggestionsViewController alloc]
here and everywhere, except init - prefer using property accessor
(self.suggestionsViewController) instead of ivar

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:147:
NSString* copyLinkTitle =
s/copyLinkTitle/deleteTitle

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm
(right):

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:205:
CollectionViewItem* touchedItem =
nit: keep all code handling the *touch* here, and move the rest (lines 208 and
below) to a method that shows a context menu for CVItem. This will be useful in
case the context menu is triggered by something else, for example a "..."
button.

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:209:
headerForSection:touchedItemIndexPath.section] ||
I'm not familiar with this API, is it possible that touched item is header?

[gambard, 2017-02-16 10:17:13]

Thanks!

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h
(right):

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.h:22: //
URLLoader used to opens pages.
On 2017/02/15 16:03:35, stkhapugin wrote:
> s/opens/open

Done.

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
File ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm
(right):

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:60:
_suggestionsViewController = [[ContentSuggestionsViewController alloc]
On 2017/02/15 16:03:36, stkhapugin wrote:
> here and everywhere, except init - prefer using property accessor
> (self.suggestionsViewController) instead of ivar 

Done.

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/cont...
ios/chrome/browser/content_suggestions/content_suggestions_coordinator.mm:147:
NSString* copyLinkTitle =
On 2017/02/15 16:03:35, stkhapugin wrote:
> s/copyLinkTitle/deleteTitle

Done.

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/ui/c...
File
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm
(right):

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:205:
CollectionViewItem* touchedItem =
On 2017/02/15 16:03:36, stkhapugin wrote:
> nit: keep all code handling the *touch* here, and move the rest (lines 208 and
> below) to a method that shows a context menu for CVItem. This will be useful
in
> case the context menu is triggered by something else, for example a "..."
> button. 

The "..." will probably be handled at the cell level. I will keep it in mind.

https://codereview.chromium.org/2691303002/diff/20001/ios/chrome/browser/ui/c...
ios/chrome/browser/ui/content_suggestions/content_suggestions_view_controller.mm:209:
headerForSection:touchedItemIndexPath.section] ||
On 2017/02/15 16:03:36, stkhapugin wrote:
> I'm not familiar with this API, is it possible that touched item is header?

Done.

[gambard, 2017-02-20 10:27:00]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-02-20 10:27:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2691303002/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2017-02-20 10:27:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 10:34:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 10:34:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[gambard, 2017-02-20 10:41:21]

gambard@chromium.org changed reviewers:
+ marq@chromium.org

[gambard, 2017-02-20 10:41:22]

+marq: PTAL at BVC.

[gambard, 2017-02-20 10:41:33]

The CQ bit was checked by gambard@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 10:41:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[marq (ping after 24h), 2017-02-20 12:07:00]

lgtm

BVC

[commit-bot: I haz the power, 2017-02-20 12:07:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 12:07:18]

Dry run: This issue passed the CQ dry run.

[gambard, 2017-02-20 12:07:32]

The CQ bit was checked by gambard@chromium.org

[gambard, 2017-02-20 12:07:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
stkhapugin@chromium.org, lpromero@chromium.org
Link to the patchset: https://codereview.chromium.org/2691303002/#ps60001
(title: "Fix gn deps")

[commit-bot: I haz the power, 2017-02-20 12:07:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-20 12:13:59]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487592452361530,
"parent_rev": "4fc5997a7496bc2b803d049ab0218c385f6d6427", "commit_rev":
"9b6abde6cad9c609e4961e2149aafd328bfa19a5"}

[commit-bot: I haz the power, 2017-02-20 12:14:33]

Description was changed from

==========
Add context menu for ContentSuggestions

This CL adds a context menu for ContentSuggestions and a way to open the
article by tapping it.
The context menu does nothing for deletion for now.

BUG=692073
==========

to

==========
Add context menu for ContentSuggestions

This CL adds a context menu for ContentSuggestions and a way to open the
article by tapping it.
The context menu does nothing for deletion for now.

BUG=692073

Review-Url: https://codereview.chromium.org/2691303002
Cr-Commit-Position: refs/heads/master@{#451602}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b6abde6cad9c609e4961e2149aa...
==========

[commit-bot: I haz the power, 2017-02-20 12:14:34]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9b6abde6cad9c609e4961e2149aa...