Add a Share Icon to Tabular Context Menu

Add a Share Icon to Tabular Context Menu

For one of the last changes on the context menu, I'll be adding a direct
share Icon so if a person clicks on it, it will share with any app. It's
pretty slick. Feel free to check out the video of the work in action.

This is activated when someone shares an item within the page.

--Screenshot--
https://drive.google.com/open?id=0B6D5A57VLDpeWEl1SXZrRGk1eXc

--Video--
https://drive.google.com/open?id=0B6D5A57VLDpecDRDNWZsb2toMjQ

BUG=655359
Review-Url: https://codereview.chromium.org/2775373002
Cr-Commit-Position: refs/heads/master@{#461269}
Committed: https://chromium.googlesource.com/chromium/src/+/b91d264cd53075aabecd1bce709b9c7146c978e9

[JJ, 2017-03-27 20:50:18]

jwanda@chromium.org changed reviewers:
+ twellington@chromium.org

[JJ, 2017-03-27 20:50:19]

Thanks again for taking a look at this. This handles a lot of share.

[Theresa, 2017-03-27 21:17:43]

Looks good overall

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:173:
// Reset the component name so it can share otherwise.
This comment isn't very clear.

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:177:
public void shareImageDirectly(ComponentName name) {
nit: this needs a JavaDoc.

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java
(right):

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:35:
* @param contextMenuHelper Used to directly share an image through the icon.
nit: ... through an icon.

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:108:
viewHolder.mShareIcon.setVisibility(View.GONE);
You should just be able to set visibility to GONE without setting the image
drawable, on click listener and content description.

[JJ, 2017-03-27 23:23:10]

Thanks for the review!

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:173:
// Reset the component name so it can share otherwise.
On 2017/03/27 21:17:43, Theresa wrote:
> This comment isn't very clear.

Done.

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:177:
public void shareImageDirectly(ComponentName name) {
On 2017/03/27 21:17:43, Theresa wrote:
> nit: this needs a JavaDoc.

Done.

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java
(right):

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:35:
* @param contextMenuHelper Used to directly share an image through the icon.
On 2017/03/27 21:17:43, Theresa wrote:
> nit: ... through an icon.

Done.

https://codereview.chromium.org/2775373002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/TabularContextMenuListAdapter.java:108:
viewHolder.mShareIcon.setVisibility(View.GONE);
On 2017/03/27 21:17:43, Theresa wrote:
> You should just be able to set visibility to GONE without setting the image
> drawable, on click listener and content description.

True. I'll change it so!

[JJ, 2017-03-28 17:12:56]

jwanda@chromium.org changed reviewers:
+ dtrainor@chromium.org

[JJ, 2017-03-28 17:12:56]

dtrainor@chromium.org: Please review changes in 

the entire cl for owners approval.

[JJ, 2017-03-29 21:37:17]

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

[commit-bot: I haz the power, 2017-03-29 21:37:55]

Dry run: 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-03-29 21:42:25]

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

[commit-bot: I haz the power, 2017-03-29 21:42:26]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[JJ, 2017-03-29 23:05:31]

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

[commit-bot: I haz the power, 2017-03-29 23:06:36]

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

[JJ, 2017-03-29 23:12:02]

Super friendly ping as it's been 24 hours c:

[commit-bot: I haz the power, 2017-03-30 04:15:31]

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

[commit-bot: I haz the power, 2017-03-30 04:15:31]

Dry run: This issue passed the CQ dry run.

[Theresa, 2017-03-30 15:12:45]

lgtm % nits

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:38:
private Activity mActivity;
Typically we recommend against caching the activity since a Tab's activity may
change if it's is reparented in Android N+ multiwindow mode. It looks like
mActivity gets reset every time showContextMenu() is called and the activity,
but it would be good to do a quick test:

1) Open Chrome, trigger the context menu
2) Put Chrome in multi-window mode
3) Move the tab that previously triggered the context menu to a new window (menu
-> "move to other window")
4) Trigger the context menu again and share item.

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:185:
// image it won't share with the last shared item unless explicitly told.
nit: ...won't share with the last shared app...?

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:191:
* @param name The app to share the image directly with.
nit: The {@link ComponentName} of the app to share...

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:318:
*             component name is.
nit: ...will share the image directly with the app associated with the {@link
ComponentName}.

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:552:
CharSequence directShareTitle;
nit: these can both defined when they are set below:

Pair<Drawable, CharSequence> directShare = getShareableIconAndName(activity);
Drawable directShareIcon = directShare.first;
CharSequence directShareTitle = directShare.second;

[JJ, 2017-03-30 16:20:17]

c: thanks!

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:38:
private Activity mActivity;
On 2017/03/30 15:12:45, Theresa wrote:
> Typically we recommend against caching the activity since a Tab's activity may
> change if it's is reparented in Android N+ multiwindow mode. It looks like
> mActivity gets reset every time showContextMenu() is called and the activity,
> but it would be good to do a quick test:
> 
> 1) Open Chrome, trigger the context menu
> 2) Put Chrome in multi-window mode
> 3) Move the tab that previously triggered the context menu to a new window
(menu
> -> "move to other window")
> 4) Trigger the context menu again and share item.

It works!

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:185:
// image it won't share with the last shared item unless explicitly told.
On 2017/03/30 15:12:45, Theresa wrote:
> nit: ...won't share with the last shared app...?

Done.

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:191:
* @param name The app to share the image directly with.
On 2017/03/30 15:12:45, Theresa wrote:
> nit: The {@link ComponentName} of the app to share...

Done.

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:318:
*             component name is.
On 2017/03/30 15:12:45, Theresa wrote:
> nit: ...will share the image directly with the app associated with the {@link
> ComponentName}.

Whoops. You just got a glimpse into what my javadocs look like before I clean
them up.

https://codereview.chromium.org/2775373002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:552:
CharSequence directShareTitle;
On 2017/03/30 15:12:45, Theresa wrote:
> nit: these can both defined when they are set below:
> 
> Pair<Drawable, CharSequence> directShare = getShareableIconAndName(activity);
> Drawable directShareIcon = directShare.first;
> CharSequence directShareTitle = directShare.second;

Done.

[JJ, 2017-03-30 17:40:57]

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

[commit-bot: I haz the power, 2017-03-30 17:41:34]

Dry run: 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-03-30 18:21:44]

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

[commit-bot: I haz the power, 2017-03-30 18:21:45]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[JJ, 2017-03-30 21:31:52]

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

[commit-bot: I haz the power, 2017-03-30 21:33:07]

Dry run: 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-03-30 22:38:22]

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

[commit-bot: I haz the power, 2017-03-30 22:38:23]

Dry run: This issue passed the CQ dry run.

[David Trainor- moved to gerrit, 2017-03-30 23:50:23]

lgtm % nits

https://codereview.chromium.org/2775373002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:174:
// image it won't share with the last shared item unless explicitly told.
item -> app?

https://codereview.chromium.org/2775373002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:194:
mComponentName = name;
This would be cleaner if we used final and magically put it in the callback
method as well :D.  But either way!

https://codereview.chromium.org/2775373002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:698:
public static ComponentName getLastShareComponentName() {
Javadoc?

[JJ, 2017-03-31 20:58:54]

Yaaaaaaaaaaaay it feels liek a real app now

https://codereview.chromium.org/2775373002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:174:
// image it won't share with the last shared item unless explicitly told.
On 2017/03/30 23:50:22, David Trainor-ping if over 24h wrote:
> item -> app?

Done.

https://codereview.chromium.org/2775373002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/contextmenu/ContextMenuHelper.java:194:
mComponentName = name;
On 2017/03/30 23:50:22, David Trainor-ping if over 24h wrote:
> This would be cleaner if we used final and magically put it in the callback
> method as well :D.  But either way!

Acknowledged.

https://codereview.chromium.org/2775373002/diff/100001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java
(right):

https://codereview.chromium.org/2775373002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/share/ShareHelper.java:698:
public static ComponentName getLastShareComponentName() {
On 2017/03/30 23:50:22, David Trainor-ping if over 24h wrote:
> Javadoc?

Done.

[JJ, 2017-03-31 22:32:47]

The CQ bit was checked by jwanda@chromium.org

[JJ, 2017-03-31 22:32:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2775373002/#ps160001
(title: "Fixed more tests!")

[commit-bot: I haz the power, 2017-03-31 22:33:34]

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-03-31 23:22:31]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1490999567271680,
"parent_rev": "4e3a8e8773e17842197072f1add49917adbaca16", "commit_rev":
"b91d264cd53075aabecd1bce709b9c7146c978e9"}

[commit-bot: I haz the power, 2017-03-31 23:23:43]

Description was changed from

==========
Add a Share Icon to Tabular Context Menu

For one of the last changes on the context menu, I'll be adding a direct
share Icon so if a person clicks on it, it will share with any app. It's
pretty slick. Feel free to check out the video of the work in action.

This is activated when someone shares an item within the page.

--Screenshot--
https://drive.google.com/open?id=0B6D5A57VLDpeWEl1SXZrRGk1eXc

--Video--
https://drive.google.com/open?id=0B6D5A57VLDpecDRDNWZsb2toMjQ

BUG=655359
==========

to

==========
Add a Share Icon to Tabular Context Menu

For one of the last changes on the context menu, I'll be adding a direct
share Icon so if a person clicks on it, it will share with any app. It's
pretty slick. Feel free to check out the video of the work in action.

This is activated when someone shares an item within the page.

--Screenshot--
https://drive.google.com/open?id=0B6D5A57VLDpeWEl1SXZrRGk1eXc

--Video--
https://drive.google.com/open?id=0B6D5A57VLDpecDRDNWZsb2toMjQ

BUG=655359

Review-Url: https://codereview.chromium.org/2775373002
Cr-Commit-Position: refs/heads/master@{#461269}
Committed:
https://chromium.googlesource.com/chromium/src/+/b91d264cd53075aabecd1bce709b...
==========

[commit-bot: I haz the power, 2017-03-31 23:23:44]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/b91d264cd53075aabecd1bce709b...