IPH : Highlight for the menu icons in the chrome home footer

IPH : Highlight for the menu icons in the chrome home footer

This CL fixes :
1 - Highlighting of menu icons in the footer in chrome home mode
2 - Highlighting of data saver footer
3 - Y inset of the text bubble in the chrome home mode

BUG=721088
Review-Url: https://codereview.chromium.org/2872023004
Cr-Commit-Position: refs/heads/master@{#471038}
Committed: https://chromium.googlesource.com/chromium/src/+/60e4c6d33956a93ca6dcb7c78c913e84445dcfce

[shaktisahu, 2017-05-10 22:21:20]

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

[shaktisahu, 2017-05-10 22:21:21]

PTAL

[David Trainor- moved to gerrit, 2017-05-10 22:41:45]

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

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:737:
0, FeatureUtilities.isChromeHomeEnabled() ? -yInsetPx : yInsetPx, 0, 0);
MathUtils.flipSignIf(yInsetPx, FeatureUtilities.isChromeHomeEnabled())

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:278: *
Highlights the given |footerView| or one of its child.
{@code footerView}?

Add a comment about highlighting the whole view if it's the whole footerView
otherwise will use a circle pulse on the individual view.

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:280: * 
                          be {@code null} if no item should be highlighted.
@param footerView.  Remove extra spaces after highlightedItemId.

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:288:
Drawable background = view.getBackground();
This may be null.  Pull this later.  IMO the flow should look like:

1. Build PulseDrawable.
2. If View has a background:
  a. Build a new background from
background.getConstantState().newDrawable(resources);
  b. Build a LayerDrawable around that and the pulsedrawable
3. Set the background to one of those two.

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:293:
PulseDrawable pulse =
Don't need a new pulseDrawable.  You're not re-using it so just use the one
above.

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:550:
0, FeatureUtilities.isChromeHomeEnabled() ? -yInsetPx : yInsetPx, 0, 0);
Same as above

[shaktisahu, 2017-05-10 23:21:38]

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

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:737:
0, FeatureUtilities.isChromeHomeEnabled() ? -yInsetPx : yInsetPx, 0, 0);
On 2017/05/10 22:41:44, David Trainor-ping if over 24h wrote:
> MathUtils.flipSignIf(yInsetPx, FeatureUtilities.isChromeHomeEnabled())

Done.

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:278: *
Highlights the given |footerView| or one of its child.
On 2017/05/10 22:41:44, David Trainor-ping if over 24h wrote:
> {@code footerView}?
> 
> Add a comment about highlighting the whole view if it's the whole footerView
> otherwise will use a circle pulse on the individual view.

Done.

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:280: * 
                          be {@code null} if no item should be highlighted.
On 2017/05/10 22:41:45, David Trainor-ping if over 24h wrote:
> @param footerView.  Remove extra spaces after highlightedItemId.

Done.

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:288:
Drawable background = view.getBackground();
On 2017/05/10 22:41:44, David Trainor-ping if over 24h wrote:
> This may be null.  Pull this later.  IMO the flow should look like:
> 
> 1. Build PulseDrawable.
> 2. If View has a background:
>   a. Build a new background from
> background.getConstantState().newDrawable(resources);
>   b. Build a LayerDrawable around that and the pulsedrawable
> 3. Set the background to one of those two.

Done.

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:293:
PulseDrawable pulse =
On 2017/05/10 22:41:45, David Trainor-ping if over 24h wrote:
> Don't need a new pulseDrawable.  You're not re-using it so just use the one
> above.

Done.

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/2872023004/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:550:
0, FeatureUtilities.isChromeHomeEnabled() ? -yInsetPx : yInsetPx, 0, 0);
On 2017/05/10 22:41:45, David Trainor-ping if over 24h wrote:
> Same as above

Done.

[David Trainor- moved to gerrit, 2017-05-11 00:36:54]

lgtm % code structure idea

https://codereview.chromium.org/2872023004/diff/20001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java
(right):

https://codereview.chromium.org/2872023004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:282: *
{@code null} if no item should be highlighted.
Sorry add spaces here to align it properly.  Just meant only have one space
after highlightedItemId.

Do you want to add the @param footerView?

https://codereview.chromium.org/2872023004/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/appmenu/AppMenu.java:293:
Drawable backgroundDrawable = pulse;
This looks like the right logic.  Might be able to clean this up a bit though? 
What do you think about:

Drawable newBackground = view == footerView ? PulseDrawable.createHighlight() :
PulseDrawable.createCircle();

Drawable currentBackground = view.getBackground();
if (currentBackground != null && currentBackground.getConstantState() != null) {
  Drawable backgroundClone =
currentBackground.getConstantState().newDrawable(footerView.getResources());

  newBackground = new LayerDrawable(new Drawable[] { newBackground,
backgroundClone });
}

view.setBackground(newBackground);

[shaktisahu, 2017-05-11 16:58:56]

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

[commit-bot: I haz the power, 2017-05-11 17:00:04]

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-05-11 18:48:59]

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

[commit-bot: I haz the power, 2017-05-11 18:49:00]

Dry run: This issue passed the CQ dry run.

[shaktisahu, 2017-05-11 19:06:13]

The CQ bit was checked by shaktisahu@chromium.org

[shaktisahu, 2017-05-11 19:06:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2872023004/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2017-05-11 19:07:47]

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-05-11 19:18:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1494529573342420,
"parent_rev": "2791788c6454704a4d6f8f6a917a3017b20f966a", "commit_rev":
"60e4c6d33956a93ca6dcb7c78c913e84445dcfce"}

[commit-bot: I haz the power, 2017-05-11 19:18:46]

Description was changed from

==========
IPH : Highlight for the menu icons in the chrome home footer

This CL fixes :
1 - Highlighting of menu icons in the footer in chrome home mode
2 - Highlighting of data saver footer
3 - Y inset of the text bubble in the chrome home mode

BUG=721088
==========

to

==========
IPH : Highlight for the menu icons in the chrome home footer

This CL fixes :
1 - Highlighting of menu icons in the footer in chrome home mode
2 - Highlighting of data saver footer
3 - Y inset of the text bubble in the chrome home mode

BUG=721088

Review-Url: https://codereview.chromium.org/2872023004
Cr-Commit-Position: refs/heads/master@{#471038}
Committed:
https://chromium.googlesource.com/chromium/src/+/60e4c6d33956a93ca6dcb7c78c91...
==========

[commit-bot: I haz the power, 2017-05-11 19:18:47]

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