Let clients update icons for buttons on bottombar through service

Let clients update icons for buttons on bottombar through service

This CL enables updateVisuals() to find the button related to the given
id, and update the icon/description of it. Bottom bar button updating is
also included in this CL.

BUG=573683
Committed: https://crrev.com/0779495a7e4924c3e4a33d43e1964f06cb6b8b00
Cr-Commit-Position: refs/heads/master@{#369227}

[Ian Wen, 2016-01-08 00:21:01]

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

[Yusuf, 2016-01-08 06:34:23]

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

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:457:
if (params == null) return;
can params be null here? I think it cant anymore.

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:475:
Integer id = params.getId();
I think you can just do int here and do == below.

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:477:
int buttonNum = bottomBar.getChildCount();
Just curious, cant we use setId instead of tag and then later directly do
findViewById here?

View#setId exists with description:
Sets the identifier for this view. The identifier does not have to be unique in
this view's hierarchy. The identifier should be a positive number.
So this should technically work for the  bottom bar. I think...

Then we don't have to go through all children. Too complex for a probably no
more than 3 child hierarchy? Feel free to ignore if you think so.

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

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:245:
if (id == params.getId()) return params;
I think this doesn't address the case for the old API with no ID still. I
thought we were going to return the toolbar button params when there was no ID
and ensure the new API always calls updateVisuals with an ID on the support lib
side.

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

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:377:
final int id = actionButtonBundle.getInt(CustomTabsIntent.KEY_ID,
oh. OK now I see how the id will be assigned. Maybe let's add a comment in the
dataprovider that the id for the old API  being handled here?

[Ian Wen, 2016-01-08 08:43:59]

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

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:457:
if (params == null) return;
On 2016/01/08 06:34:22, Yusuf wrote:
> can params be null here? I think it cant anymore.

It is also called by postInflationStartup() and the params will be null if the
client does not specify any custom buttons. :P

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:475:
Integer id = params.getId();
On 2016/01/08 06:34:22, Yusuf wrote:
> I think you can just do int here and do == below.

Hmm View#getTag() returns an object and if I directly compare int with an object
it won't compile. If id is an integer and == is used in #480, java will compare
references, not value...

Another way is to cast getTag() to Integer and use == to compare with int id.
But it is slightly less elegant, lol.

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:477:
int buttonNum = bottomBar.getChildCount();
On 2016/01/08 06:34:22, Yusuf wrote:
> Just curious, cant we use setId instead of tag and then later directly do
> findViewById here?
> 
> View#setId exists with description:
> Sets the identifier for this view. The identifier does not have to be unique
in
> this view's hierarchy. The identifier should be a positive number.
> So this should technically work for the  bottom bar. I think...
> 
> Then we don't have to go through all children. Too complex for a probably no
> more than 3 child hierarchy? Feel free to ignore if you think so.

Creative idea! I tested setId() and findById() locally and it works! I would
definitely take this approach if we disallow clients to send us negative button
ids. :(

In
ViewGroup#findViewById()(http://androidxref.com/6.0.0_r1/xref/frameworks/base/core/java/android/view/ViewGroup.java#3830),
ViewGroup does not seem to care the sign of the id integer, but since the
javadoc specifically said "The identifier should be a positive number", we might
want to respect it?

Also I checked R.java and Android generates ids from the largest positive
integer 0x7fffffff and decrease it one by one, so internally they indeed avoid
using negative ids.

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

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:245:
if (id == params.getId()) return params;
On 2016/01/08 06:34:23, Yusuf wrote:
> I think this doesn't address the case for the old API with no ID still. I
> thought we were going to return the toolbar button params when there was no ID
> and ensure the new API always calls updateVisuals with an ID on the support
lib
> side.

Added some comments.

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

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabsConnection.java:377:
final int id = actionButtonBundle.getInt(CustomTabsIntent.KEY_ID,
On 2016/01/08 06:34:23, Yusuf wrote:
> oh. OK now I see how the id will be assigned. Maybe let's add a comment in the
> dataprovider that the id for the old API  being handled here?

Done.

[Yusuf, 2016-01-09 00:14:22]

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

https://codereview.chromium.org/1568813002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:477:
int buttonNum = bottomBar.getChildCount();
On 2016/01/08 08:43:58, Ian Wen wrote:
> On 2016/01/08 06:34:22, Yusuf wrote:
> > Just curious, cant we use setId instead of tag and then later directly do
> > findViewById here?
> > 
> > View#setId exists with description:
> > Sets the identifier for this view. The identifier does not have to be unique
> in
> > this view's hierarchy. The identifier should be a positive number.
> > So this should technically work for the  bottom bar. I think...
> > 
> > Then we don't have to go through all children. Too complex for a probably no
> > more than 3 child hierarchy? Feel free to ignore if you think so.
> 
> Creative idea! I tested setId() and findById() locally and it works! I would
> definitely take this approach if we disallow clients to send us negative
button
> ids. :(
> 
> In
>
ViewGroup#findViewById()(http://androidxref.com/6.0.0_r1/xref/frameworks/base/core/java/android/view/ViewGroup.java#3830),
> ViewGroup does not seem to care the sign of the id integer, but since the
> javadoc specifically said "The identifier should be a positive number", we
might
> want to respect it?
> 
> Also I checked R.java and Android generates ids from the largest positive
> integer 0x7fffffff and decrease it one by one, so internally they indeed avoid
> using negative ids.

Let's say positive then. I don't think anyone will give negative ids here
anyway.

https://codereview.chromium.org/1568813002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1568813002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:475:
Integer id = params.getId();
I think since view id is int, this won't be an issue anymore if you do the other
findViewById trick.

https://codereview.chromium.org/1568813002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/1568813002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:245:
// A custom button params will always carry an ID. If the client calls
updateVisuals()
>100 chars? codereview issue?

[Ian Wen, 2016-01-12 22:30:53]

Replaced Tag with ID in view. PTAL

https://codereview.chromium.org/1568813002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
(right):

https://codereview.chromium.org/1568813002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java:475:
Integer id = params.getId();
On 2016/01/09 00:14:22, Yusuf wrote:
> I think since view id is int, this won't be an issue anymore if you do the
other
> findViewById trick.

Done.

https://codereview.chromium.org/1568813002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
(right):

https://codereview.chromium.org/1568813002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java:245:
// A custom button params will always carry an ID. If the client calls
updateVisuals()
On 2016/01/09 00:14:22, Yusuf wrote:
> >100 chars? codereview issue?

Yeah it's a codereview issue.

[Yusuf, 2016-01-13 05:47:17]

lgtm

[Ian Wen, 2016-01-13 18:17:22]

The CQ bit was checked by ianwen@chromium.org

[commit-bot: I haz the power, 2016-01-13 18:18:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1568813002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1568813002/80001

[commit-bot: I haz the power, 2016-01-13 18:58:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-13 18:59:03]

Description was changed from

==========
Let clients update icons for buttons on bottombar through service

This CL enables updateVisuals() to find the button related to the given
id, and update the icon/description of it. Bottom bar button updating is
also included in this CL.

BUG=573683
==========

to

==========
Let clients update icons for buttons on bottombar through service

This CL enables updateVisuals() to find the button related to the given
id, and update the icon/description of it. Bottom bar button updating is
also included in this CL.

BUG=573683

Committed: https://crrev.com/0779495a7e4924c3e4a33d43e1964f06cb6b8b00
Cr-Commit-Position: refs/heads/master@{#369227}
==========

[commit-bot: I haz the power, 2016-01-13 18:59:04]

Patchset 5 (id:??) landed as
https://crrev.com/0779495a7e4924c3e4a33d43e1964f06cb6b8b00
Cr-Commit-Position: refs/heads/master@{#369227}