[Android] Update CCT/Herb menu item

[Android]Update CCT/Herb menu item

Add more items to CCT/Herb menu including:
1. Bookmark action & Download action to the top bar
2. "Add to home screen" and "Request desktop site" to the menu
3. The return of "find in page"

Udpate text message in the snackbar for Bookmark action and Download
action to display which app the action is execucted. For example, if CCT
is opened in Chrome Dev, the text in the snackbar for Bookmark should be
"Bookmarked in Chrome Dev".

BUG=671797
Review-Url: https://codereview.chromium.org/2596973002
Cr-Commit-Position: refs/heads/master@{#441743}
Committed: https://chromium.googlesource.com/chromium/src/+/5abc9695b5db94035f872aaaea6c833e70971e2d

[ltian, 2016-12-21 23:05:00]

Description was changed from

==========
[Android]Update CCT/Herb menu item

Add more items to CCT/Herb menu including:
1. Bookmark action & Download action to the top bar
2. "Add to home screen" and "Request desktop site" to the menu
3. The return of "find in page"

Udpate text message in the snackbar for Bookmark action and Download
action to display which app the action is execucted. For example, if CCT
is opened in clankium, the text in the snackbar for Bookmark should be
"Bookmarked in clankium".

BUG=671797
==========

to

==========
[Android]Update CCT/Herb menu item

Add more items to CCT/Herb menu including:
1. Bookmark action & Download action to the top bar
2. "Add to home screen" and "Request desktop site" to the menu
3. The return of "find in page"

Udpate text message in the snackbar for Bookmark action and Download
action to display which app the action is execucted. For example, if CCT
is opened in clankium, the text in the snackbar for Bookmark should be
"Bookmarked in clankium".

BUG=671797
==========

[ltian, 2016-12-21 23:05:45]

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

[commit-bot: I haz the power, 2016-12-21 23:06:18]

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, 2016-12-21 23:52:36]

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

[commit-bot: I haz the power, 2016-12-21 23:52:37]

Dry run: This issue passed the CQ dry run.

[ltian, 2016-12-22 00:01:40]

ltian@chromium.org changed reviewers:
+ yusufo@chromium.org

[ltian, 2016-12-22 00:01:41]

yusufo@chromium.org: can you take a look for the changes related to custom tabs?

Thanks!

[ltian, 2016-12-27 18:36:52]

ltian@chromium.org changed reviewers:
+ tedchoc@chromium.org

[ltian, 2016-12-27 18:36:53]

tedchoc@chromium.org: can you take a look about my changes for this CL?

Thanks!

[Ted C, 2016-12-28 00:05:13]

Description was changed from

==========
[Android]Update CCT/Herb menu item

Add more items to CCT/Herb menu including:
1. Bookmark action & Download action to the top bar
2. "Add to home screen" and "Request desktop site" to the menu
3. The return of "find in page"

Udpate text message in the snackbar for Bookmark action and Download
action to display which app the action is execucted. For example, if CCT
is opened in clankium, the text in the snackbar for Bookmark should be
"Bookmarked in clankium".

BUG=671797
==========

to

==========
[Android]Update CCT/Herb menu item

Add more items to CCT/Herb menu including:
1. Bookmark action & Download action to the top bar
2. "Add to home screen" and "Request desktop site" to the menu
3. The return of "find in page"

Udpate text message in the snackbar for Bookmark action and Download
action to display which app the action is execucted. For example, if CCT
is opened in Chrome Dev, the text in the snackbar for Bookmark should be
"Bookmarked in Chrome Dev".

BUG=671797
==========

[Ted C, 2016-12-28 00:53:47]

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1280:
snackbarText);
actually, I think you can just use isCustomTab() right here to avoid adding a
new param at all (per my later comment)

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:54:
*        <code>null</code> if bookmarking inside Chrome.
You can use the @Nullable annotation on the parameter to let others know it can
be null, but switching to a boolean will avoid that need at all.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:59:
Tab tab, SnackbarManager snackbarManager, Activity activity, String
snackbarText) {
instead of passing in the text to use, just pass in a boolean whether the
bookmark was done in Chrome.  Then you can have all the string stuff within this
class.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
(left):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:76:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
FWIW, by removing this, you are essentially blocking on the UI for all of these
calls.  This was done this way before to hopefully have it ready by the time the
menu code needed it.  Regardless, I don't think you'll need any of this change
based on my other comment in this file.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:45:
private class DefaultBrowserFetcher extends AsyncTask<String, Void, String> {
Hmm...I'm pretty sure you don't want this string logic for the bookmark
snackbar.

For bookmarks, we don't care if the default browser is not set (or if it is
different from the current Chrome), we always want to show
BuildInfo.getPackageLabel in that case.  We are educating the users where there
bookmark went, so we need to say the current version of Chrome always.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:128:
if (!FirstRunStatus.getFirstRunFlowComplete()) {
if first run hasn't been completed, can you make it so that bookmarks and
downloads are turned off in the menu.

If you clear your settings and start first run again, the privacy and TOS links
will now open in a CCT.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:150:
? mActivity.getString(R.string.menu_request_desktop_site_on)
indented too much.  should be 8 from the start of the previous line

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSnackbarController.java
(right):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSnackbarController.java:124:
public Activity getLastTrackedFocusedActivity() {
I would just call this getActivity() and I think it should be private.

And I would put in the check ApplicationStatus.hasVisibleActivities() here.  If
that is false, return null.

[ltian, 2016-12-28 20:00:35]

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

[commit-bot: I haz the power, 2016-12-28 20:00:53]

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, 2016-12-28 20:37:43]

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

[commit-bot: I haz the power, 2016-12-28 20:37:43]

Dry run: This issue passed the CQ dry run.

[ltian, 2016-12-29 16:31:56]

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1280:
snackbarText);
On 2016/12/28 00:53:46, Ted C wrote:
> actually, I think you can just use isCustomTab() right here to avoid adding a
> new param at all (per my later comment)

Done.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:54:
*        <code>null</code> if bookmarking inside Chrome.
On 2016/12/28 00:53:47, Ted C wrote:
> You can use the @Nullable annotation on the parameter to let others know it
can
> be null, but switching to a boolean will avoid that need at all.

Done.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:59:
Tab tab, SnackbarManager snackbarManager, Activity activity, String
snackbarText) {
On 2016/12/28 00:53:47, Ted C wrote:
> instead of passing in the text to use, just pass in a boolean whether the
> bookmark was done in Chrome.  Then you can have all the string stuff within
this
> class.

Done.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
(left):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:76:
}.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
On 2016/12/28 00:53:47, Ted C wrote:
> FWIW, by removing this, you are essentially blocking on the UI for all of
these
> calls.  This was done this way before to hopefully have it ready by the time
the
> menu code needed it.  Regardless, I don't think you'll need any of this change
> based on my other comment in this file.

Done.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:45:
private class DefaultBrowserFetcher extends AsyncTask<String, Void, String> {
On 2016/12/28 00:53:47, Ted C wrote:
> Hmm...I'm pretty sure you don't want this string logic for the bookmark
> snackbar.
> 
> For bookmarks, we don't care if the default browser is not set (or if it is
> different from the current Chrome), we always want to show
> BuildInfo.getPackageLabel in that case.  We are educating the users where
there
> bookmark went, so we need to say the current version of Chrome always.

Done.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:128:
if (!FirstRunStatus.getFirstRunFlowComplete()) {
On 2016/12/28 00:53:47, Ted C wrote:
> if first run hasn't been completed, can you make it so that bookmarks and
> downloads are turned off in the menu.
> 
> If you clear your settings and start first run again, the privacy and TOS
links
> will now open in a CCT.

Done.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:150:
? mActivity.getString(R.string.menu_request_desktop_site_on)
On 2016/12/28 00:53:47, Ted C wrote:
> indented too much.  should be 8 from the start of the previous line

Done.

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSnackbarController.java
(right):

https://codereview.chromium.org/2596973002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSnackbarController.java:124:
public Activity getLastTrackedFocusedActivity() {
On 2016/12/28 00:53:47, Ted C wrote:
> I would just call this getActivity() and I think it should be private.
> 
> And I would put in the check ApplicationStatus.hasVisibleActivities() here. 
If
> that is false, return null.

Done.

[Ted C, 2017-01-05 19:13:59]

lgtm w/ a few comments

https://codereview.chromium.org/2596973002/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/2596973002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1250: *
@param snackbarText Text shown in the snackbar for third-party app. Could be
remove as this is now unused

https://codereview.chromium.org/2596973002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2596973002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabAppMenuPropertiesDelegate.java:136:
MenuItem requestItem = menu.findItem(R.id.request_desktop_site_id);
Can we make this a helper method on the base AppMenuPropertiesDelegate?  Would
be nice to not have to duplicate this (like updateBookmarkMenuItem).

https://codereview.chromium.org/2596973002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSnackbarController.java
(right):

https://codereview.chromium.org/2596973002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/download/DownloadSnackbarController.java:83:
downloadInfo.getFileName(), packageLabel),
looks like git cl format tried to be too helpful.  this should be aligned with
the row below.

Should be 8 from the start of the previous row.

Same with the section below.

[ltian, 2017-01-05 20:06:22]

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

[commit-bot: I haz the power, 2017-01-05 20:06:38]

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-01-05 20:37:12]

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

[commit-bot: I haz the power, 2017-01-05 20:37:13]

Dry run: This issue passed the CQ dry run.

[ltian, 2017-01-05 20:37:34]

The CQ bit was checked by ltian@chromium.org

[ltian, 2017-01-05 20:37:35]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2596973002/#ps40001
(title: "Update based on Ted's comments.")

[commit-bot: I haz the power, 2017-01-05 20:37:59]

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-01-05 20:43:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1483648654717360,
"parent_rev": "453b8869061af7fb18fc773dc4e585229bad6bb9", "commit_rev":
"5abc9695b5db94035f872aaaea6c833e70971e2d"}

[commit-bot: I haz the power, 2017-01-05 20:43:46]

Description was changed from

==========
[Android]Update CCT/Herb menu item

Add more items to CCT/Herb menu including:
1. Bookmark action & Download action to the top bar
2. "Add to home screen" and "Request desktop site" to the menu
3. The return of "find in page"

Udpate text message in the snackbar for Bookmark action and Download
action to display which app the action is execucted. For example, if CCT
is opened in Chrome Dev, the text in the snackbar for Bookmark should be
"Bookmarked in Chrome Dev".

BUG=671797
==========

to

==========
[Android]Update CCT/Herb menu item

Add more items to CCT/Herb menu including:
1. Bookmark action & Download action to the top bar
2. "Add to home screen" and "Request desktop site" to the menu
3. The return of "find in page"

Udpate text message in the snackbar for Bookmark action and Download
action to display which app the action is execucted. For example, if CCT
is opened in Chrome Dev, the text in the snackbar for Bookmark should be
"Bookmarked in Chrome Dev".

BUG=671797

Review-Url: https://codereview.chromium.org/2596973002
Cr-Commit-Position: refs/heads/master@{#441743}
Committed:
https://chromium.googlesource.com/chromium/src/+/5abc9695b5db94035f872aaaea6c...
==========

[commit-bot: I haz the power, 2017-01-05 20:43:47]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/5abc9695b5db94035f872aaaea6c...

[Ted C, 2017-01-05 21:30:20]

Couple small things based on the last change.

FWIW: it is best to reply to all comments left on a CL (even to say Done). 
Otherwise, the reviewer needs to look at the change after submitted to verify.

https://codereview.chromium.org/2596973002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java
(right):

https://codereview.chromium.org/2596973002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:301:
MenuItem requstMenuItem, Tab currentTab, boolean isChromeScheme) {
Also, I would recalculate isChromeSheme within this method based on the tab
passed in.

https://codereview.chromium.org/2596973002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenuPropertiesDelegate.java:305:
? mActivity.getString(R.string.menu_request_desktop_site_on)
nit: indented too much (should be 8 from the start of the line)