Android - Introduce "keyboard accessory" for Autofill suggestions.

Android - Introduce "keyboard accessory" for Autofill suggestions.

The new UI is exposed behind a flag, --enable-autofill-keyboard-accessory-view. The intent is that there should be no behavior change when the flag is not passed, but eventually the flag will become the default and AutofillPopup[Bridge] and related code will be removed.

This maintains the same appearance for the list items, and matching the mocks (switching to white on blue, etc.) is a TODO. See go/wzhwq for goal state.

There are also TODOs surrounding behavior:
- we likely want to hide the keyboard accessory if the keyboard hide
- add animations
- somehow deal with very small amounts of available space (comes up in landscape mode)

BUG=428087
TBR=isherman@chromium.org

Committed: https://crrev.com/b5efbab6f71b0e2660e3d5231d99df698c0e5b38
Cr-Commit-Position: refs/heads/master@{#327324}

[Evan Stade, 2015-04-17 19:08:16]

estade@chromium.org changed reviewers:
+ dtrainor@chromium.org, newt@chromium.org, rouslan@chromium.org

[Evan Stade, 2015-04-17 19:08:59]

+dtrainor, newt for android review
+rouslan as review/FYI for src/components/autofill changes

[please use gerrit instead, 2015-04-17 19:17:51]

components/autofill lgtm

[Evan Stade, 2015-04-20 22:47:23]

On 2015/04/17 19:17:51, Rouslan Solomakhin wrote:
> components/autofill lgtm

ping newt and/or dtrainor

[David Trainor- moved to gerrit, 2015-04-20 23:29:44]

https://codereview.chromium.org/1082183002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1082183002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:32:
mAccessoryView = null;
Do you need this line?

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc
(right):

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
2015?

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:26:
AutofillKeyboardAccessoryView::~AutofillKeyboardAccessoryView() {
Should we be clearing Java's native pointer of this object in Hide() (or here)? 
We don't want something trying to call back into this object when it's gone.  It
could happen if the handler's runnable runs after we destroy this object.

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:42:
controller_ = NULL;
nullptr

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
2015

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:32:
* Has to be kept in sync with enum in WebAutofillClient.h
Should we add a gyp target to build the Java enum from the C++ one?  That's been
the general rule lately AFAIK.  If not, this is fine :).

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:63:
WindowAndroid windowAndroid, AutofillKeyboardAccessoryDelegate autofillCallback)
{
assert autofillCallback != null?

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:104:
listItem.measure(0, 0);
Should we make a proper widthMeasureSpec and heightMeasureSpec here?  Look at
View#makeMeasureSpec().  It will end up at 0 anyway, but it seems like a future
proof/safe idea IMO.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:114:
container.setVisibility(View.VISIBLE);
Do we need to set visibility?

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:119:
public void dismiss() {
Javadoc

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:122:
container.setVisibility(View.GONE);
Do we need to set visibility?

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:334: * overlaying
it. Currently used as a container for Autofill suggestions.
add @param view and describe the viewgroup there imo.  We probably should have
done it for the placeholder view above earlier.  I missed that one!

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:340: public
ViewGroup getKeyboardAccessoryView() {
Javadoc

[Evan Stade, 2015-04-21 00:48:50]

estade@chromium.org changed reviewers:
+ aruslan@chromium.org

[Evan Stade, 2015-04-21 00:48:52]

+aruslan for OWNERS coverage of chrome/browser/ui/android/autofill/

https://codereview.chromium.org/1082183002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1082183002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:32:
mAccessoryView = null;
On 2015/04/20 23:29:43, David Trainor wrote:
> Do you need this line?

code removed

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc
(right):

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2015/04/20 23:29:43, David Trainor wrote:
> 2015?

Done.

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:26:
AutofillKeyboardAccessoryView::~AutofillKeyboardAccessoryView() {
On 2015/04/20 23:29:43, David Trainor wrote:
> Should we be clearing Java's native pointer of this object in Hide() (or
here)? 
> We don't want something trying to call back into this object when it's gone. 
It
> could happen if the handler's runnable runs after we destroy this object.

I copied this pattern from AutofillPopupBridge/AutofillPopupViewAndroid. I
changed to create/init to avoid this problem.

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:42:
controller_ = NULL;
On 2015/04/20 23:29:43, David Trainor wrote:
> nullptr

Done.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:1:
// Copyright 2013 The Chromium Authors. All rights reserved.
On 2015/04/20 23:29:43, David Trainor wrote:
> 2015

Done.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:32:
* Has to be kept in sync with enum in WebAutofillClient.h
On 2015/04/20 23:29:43, David Trainor wrote:
> Should we add a gyp target to build the Java enum from the C++ one?  That's
been
> the general rule lately AFAIK.  If not, this is fine :).

mehhh

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:63:
WindowAndroid windowAndroid, AutofillKeyboardAccessoryDelegate autofillCallback)
{
On 2015/04/20 23:29:43, David Trainor wrote:
> assert autofillCallback != null?

Done.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:104:
listItem.measure(0, 0);
On 2015/04/20 23:29:43, David Trainor wrote:
> Should we make a proper widthMeasureSpec and heightMeasureSpec here?  Look at
> View#makeMeasureSpec().  It will end up at 0 anyway, but it seems like a
future
> proof/safe idea IMO.

I prefer to avoid adding complexity for the sake of speculative future-proofing,
but maybe I don't fully understand the implications of what you're saying. Is
this not a "safe" way to determine a view's preferred/natural size?

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:114:
container.setVisibility(View.VISIBLE);
On 2015/04/20 23:29:43, David Trainor wrote:
> Do we need to set visibility?

it's possible that we could just leave it visible all the time. Would it be
allocated 0 space if it's WRAP_CONTENT and has no content?

It does seem like setting the visibility is likely to be a better idea in terms
of the performance optimizations it allows and for the readability of the code
and xml files.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:122:
container.setVisibility(View.GONE);
On 2015/04/20 23:29:43, David Trainor wrote:
> Do we need to set visibility?

Done.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:334: * overlaying
it. Currently used as a container for Autofill suggestions.
On 2015/04/20 23:29:44, David Trainor wrote:
> add @param view and describe the viewgroup there imo.  We probably should have
> done it for the placeholder view above earlier.  I missed that one!

Done.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:340: public
ViewGroup getKeyboardAccessoryView() {
On 2015/04/20 23:29:43, David Trainor wrote:
> Javadoc

I can't think of a doc that makes this any easier to understand than it is
already. That is, most things I could write would be redundant with the docs for
setKeyboardAccessoryView. Do you have a suggestion for what you'd like to see
here?

[David Trainor- moved to gerrit, 2015-04-21 06:55:47]

moar commentz.

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc
(right):

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:26:
AutofillKeyboardAccessoryView::~AutofillKeyboardAccessoryView() {
On 2015/04/21 00:48:51, Evan Stade wrote:
> On 2015/04/20 23:29:43, David Trainor wrote:
> > Should we be clearing Java's native pointer of this object in Hide() (or
> here)? 
> > We don't want something trying to call back into this object when it's gone.

> It
> > could happen if the handler's runnable runs after we destroy this object.
> 
> I copied this pattern from AutofillPopupBridge/AutofillPopupViewAndroid. I
> changed to create/init to avoid this problem.

I might be misunderstanding the lifecycle of this class.  Does this class get
killed/recreated during runtime or is it a singleton that's created only once? 
If it gets killed/recreated during runtime then I think my concern still
applies.  Just because this comes from an existing pattern doesn't make it safe.
 If the Java component lives around long enough to try to poke this native
object after it's destroyed it'll be a bug.  The easiest way to fix that is to
clear the native pointer in Java when this class goes away.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:32:
* Has to be kept in sync with enum in WebAutofillClient.h
On 2015/04/21 00:48:51, Evan Stade wrote:
> On 2015/04/20 23:29:43, David Trainor wrote:
> > Should we add a gyp target to build the Java enum from the C++ one?  That's
> been
> > the general rule lately AFAIK.  If not, this is fine :).
> 
> mehhh

Haha I'm not sure your argument convinced me!  I'll let Newton chime in here,
but I will say I don't think anyone changing that file in WebKit will ever find
this file in Java and keep it up to date...

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:104:
listItem.measure(0, 0);
On 2015/04/21 00:48:51, Evan Stade wrote:
> On 2015/04/20 23:29:43, David Trainor wrote:
> > Should we make a proper widthMeasureSpec and heightMeasureSpec here?  Look
at
> > View#makeMeasureSpec().  It will end up at 0 anyway, but it seems like a
> future
> > proof/safe idea IMO.
> 
> I prefer to avoid adding complexity for the sake of speculative
future-proofing,
> but maybe I don't fully understand the implications of what you're saying. Is
> this not a "safe" way to determine a view's preferred/natural size?

I don't think changing it adds complexity.  I think changing it uses the API
correctly.  From a code search, it looks like we use
MeasureSpec.makeMeasureSpec() pretty much everywhere else in the codebase.  If
we have places where we don't, we should probably fix those too!

The issue is the int you're passing in is a union of a MeasureSpec mode and a
size.  It just happens that 0 has the correct behavior you want.  But passing 0
exposes the internals of how Android wants to format their MeasureSpec.  If you
want the same behavior, you should probably do the following for both the width
and height:

MeasureSpec.makeMeasureSpec(0, MeasureSpec.UNSPECIFIED)

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:114:
container.setVisibility(View.VISIBLE);
On 2015/04/21 00:48:51, Evan Stade wrote:
> On 2015/04/20 23:29:43, David Trainor wrote:
> > Do we need to set visibility?
> 
> it's possible that we could just leave it visible all the time. Would it be
> allocated 0 space if it's WRAP_CONTENT and has no content?
> 
> It does seem like setting the visibility is likely to be a better idea in
terms
> of the performance optimizations it allows and for the readability of the code
> and xml files.

Oh sorry I misread what you were setting visibility on.  I thought it was
setting visibility on the child that was being added/removed, not the container.
 My bad.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:340: public
ViewGroup getKeyboardAccessoryView() {
On 2015/04/21 00:48:52, Evan Stade wrote:
> On 2015/04/20 23:29:43, David Trainor wrote:
> > Javadoc
> 
> I can't think of a doc that makes this any easier to understand than it is
> already. That is, most things I could write would be redundant with the docs
for
> setKeyboardAccessoryView. Do you have a suggestion for what you'd like to see
> here?

I'd be fine with an {@see #setKeyboardAccessoryView(ViewGroup)}, but yeah I see
your point. :)

[Evan Stade, 2015-04-21 18:10:30]

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc
(right):

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:26:
AutofillKeyboardAccessoryView::~AutofillKeyboardAccessoryView() {
On 2015/04/21 06:55:47, David Trainor wrote:
> On 2015/04/21 00:48:51, Evan Stade wrote:
> > On 2015/04/20 23:29:43, David Trainor wrote:
> > > Should we be clearing Java's native pointer of this object in Hide() (or
> > here)? 
> > > We don't want something trying to call back into this object when it's
gone.
> 
> > It
> > > could happen if the handler's runnable runs after we destroy this object.
> > 
> > I copied this pattern from AutofillPopupBridge/AutofillPopupViewAndroid. I
> > changed to create/init to avoid this problem.
> 
> I might be misunderstanding the lifecycle of this class.  Does this class get
> killed/recreated during runtime or is it a singleton that's created only once?

> If it gets killed/recreated during runtime then I think my concern still
> applies.  Just because this comes from an existing pattern doesn't make it
safe.

No, but if it comes from a pattern that's existed for years without any crash
reports, that's an indication of something. Regardless, I have changed the way
it works so there's no handler.

I don't see the pattern you're talking about used in any of the other C++/java
jni class pairs, but maybe you can point me to one. AFAIU, java_object_
maintains a java reference to the bridge class. Nothing else holds a reference
to that class, so when this C++ object goes down the java side should also go
down.

>  If the Java component lives around long enough to try to poke this native
> object after it's destroyed it'll be a bug.  The easiest way to fix that is to
> clear the native pointer in Java when this class goes away.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:32:
* Has to be kept in sync with enum in WebAutofillClient.h
On 2015/04/21 06:55:47, David Trainor wrote:
> On 2015/04/21 00:48:51, Evan Stade wrote:
> > On 2015/04/20 23:29:43, David Trainor wrote:
> > > Should we add a gyp target to build the Java enum from the C++ one? 
That's
> > been
> > > the general rule lately AFAIK.  If not, this is fine :).
> > 
> > mehhh
> 
> Haha I'm not sure your argument convinced me!  I'll let Newton chime in here,
> but I will say I don't think anyone changing that file in WebKit will ever
find
> this file in Java and keep it up to date...

OK, I looked at doing this. This file is in ui/ (because that's where
AutofillPopup lives, which is used by AW, and eventually this class will be used
by AW, I guess? I'm not sure if it will be easy to use this class in AW but I
don't like the idea of maintaining a separate UI just for AW) and the enum is in
components/. I believe ui/ can't depend on components/.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:104:
listItem.measure(0, 0);
On 2015/04/21 06:55:47, David Trainor wrote:
> On 2015/04/21 00:48:51, Evan Stade wrote:
> > On 2015/04/20 23:29:43, David Trainor wrote:
> > > Should we make a proper widthMeasureSpec and heightMeasureSpec here?  Look
> at
> > > View#makeMeasureSpec().  It will end up at 0 anyway, but it seems like a
> > future
> > > proof/safe idea IMO.
> > 
> > I prefer to avoid adding complexity for the sake of speculative
> future-proofing,
> > but maybe I don't fully understand the implications of what you're saying.
Is
> > this not a "safe" way to determine a view's preferred/natural size?
> 
> I don't think changing it adds complexity.  I think changing it uses the API
> correctly.  From a code search, it looks like we use
> MeasureSpec.makeMeasureSpec() pretty much everywhere else in the codebase.  If
> we have places where we don't, we should probably fix those too!
> 
> The issue is the int you're passing in is a union of a MeasureSpec mode and a
> size.  It just happens that 0 has the correct behavior you want.  But passing
0
> exposes the internals of how Android wants to format their MeasureSpec.  If
you
> want the same behavior, you should probably do the following for both the
width
> and height:
> 
> MeasureSpec.makeMeasureSpec(0, MeasureSpec.UNSPECIFIED)

Done.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:119:
public void dismiss() {
On 2015/04/20 23:29:43, David Trainor wrote:
> Javadoc

Done.

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:340: public
ViewGroup getKeyboardAccessoryView() {
On 2015/04/21 06:55:47, David Trainor wrote:
> On 2015/04/21 00:48:52, Evan Stade wrote:
> > On 2015/04/20 23:29:43, David Trainor wrote:
> > > Javadoc
> > 
> > I can't think of a doc that makes this any easier to understand than it is
> > already. That is, most things I could write would be redundant with the docs
> for
> > setKeyboardAccessoryView. Do you have a suggestion for what you'd like to
see
> > here?
> 
> I'd be fine with an {@see #setKeyboardAccessoryView(ViewGroup)}, but yeah I
see
> your point. :)

Done.

[David Trainor- moved to gerrit, 2015-04-21 21:02:31]

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc
(right):

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:26:
AutofillKeyboardAccessoryView::~AutofillKeyboardAccessoryView() {
On 2015/04/21 18:10:30, Evan Stade wrote:
> On 2015/04/21 06:55:47, David Trainor wrote:
> > On 2015/04/21 00:48:51, Evan Stade wrote:
> > > On 2015/04/20 23:29:43, David Trainor wrote:
> > > > Should we be clearing Java's native pointer of this object in Hide() (or
> > > here)? 
> > > > We don't want something trying to call back into this object when it's
> gone.
> > 
> > > It
> > > > could happen if the handler's runnable runs after we destroy this
object.
> > > 
> > > I copied this pattern from AutofillPopupBridge/AutofillPopupViewAndroid. I
> > > changed to create/init to avoid this problem.
> > 
> > I might be misunderstanding the lifecycle of this class.  Does this class
get
> > killed/recreated during runtime or is it a singleton that's created only
once?
> 
> > If it gets killed/recreated during runtime then I think my concern still
> > applies.  Just because this comes from an existing pattern doesn't make it
> safe.
> 
> No, but if it comes from a pattern that's existed for years without any crash
> reports, that's an indication of something. Regardless, I have changed the way
> it works so there's no handler.

I don't think that's a valid argument for proving that a piece of code is safe.

> 
> I don't see the pattern you're talking about used in any of the other C++/java
> jni class pairs, but maybe you can point me to one. AFAIU, java_object_
> maintains a java reference to the bridge class. Nothing else holds a reference
> to that class, so when this C++ object goes down the java side should also go
> down.

Sorry I wasn't very clear earlier :(.  We're actually talking about the same
pattern!  The issue is the complete pattern destroys the java -> native
connection when the native object dies.  The code you copied just didn't do this
(and should probably be fixed too!).  Here are some examples of classes that
follow this pattern and do this (I did a quick code search for upstream code and
grabbed a few):
ContentViewCore.java
WebContents.java
NavigationController.java
MediaDrmBridge.java
MediaPlayerBridge.java
AwContentsClientBridge.java

The issue is, if you don't destroy the Java -> native reference you're 100%
relying on nobody holding onto a Java reference when native dies.  This is
fragile - someone touching your AutofillKeyboardAccessory View and adding some
animation/handler might never think to check the delegate/bridge.  It's also
unsafe if you interact with the Android framework, like this code does.

As for this code in particular:
AutofillKeyboardAccessory holds a reference to AutofillKeyboardAccessoryBridge. 
The Android framework holds a reference to the AutofillKeyboardAccessory View. 
The Android framework can do whatever it wants.  In fact a lot of the click
handling gets posted on their own internal handlers... View.java is actually
unsafe in that regard (posts a task to notify the click listener).  AbsListView
looks somewhat safe for the current version of L (still can post a task, but
looks like it might check if the View was detached from the window), but might
be unsafe for older Android versions or custom OEM versions.

These are incredibly unlikely timing bugs, but just because it's unlikely
doesn't mean we should ignore it.  Especially when the fix is as simple as one
of:

1. Inside AutofillKeyboardAccessoryBridge#dismiss() we just clear our native
pointer and do != 0 checks everywhere we use it.
2. Add a AutofillKeyboardAccessoryBridge#clearNativePtr() that gets called from
the destructor of native AutofillKeyboardAccessoryView and do != 0 checks
everywhere we use the native pointer.

Sorry for the mix up on language around the "pattern."  Let me know if this make
sense!

> 
> >  If the Java component lives around long enough to try to poke this native
> > object after it's destroyed it'll be a bug.  The easiest way to fix that is
to
> > clear the native pointer in Java when this class goes away.
>

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1082183002/diff/80001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:32:
* Has to be kept in sync with enum in WebAutofillClient.h
On 2015/04/21 18:10:30, Evan Stade wrote:
> On 2015/04/21 06:55:47, David Trainor wrote:
> > On 2015/04/21 00:48:51, Evan Stade wrote:
> > > On 2015/04/20 23:29:43, David Trainor wrote:
> > > > Should we add a gyp target to build the Java enum from the C++ one? 
> That's
> > > been
> > > > the general rule lately AFAIK.  If not, this is fine :).
> > > 
> > > mehhh
> > 
> > Haha I'm not sure your argument convinced me!  I'll let Newton chime in
here,
> > but I will say I don't think anyone changing that file in WebKit will ever
> find
> > this file in Java and keep it up to date...
> 
> OK, I looked at doing this. This file is in ui/ (because that's where
> AutofillPopup lives, which is used by AW, and eventually this class will be
used
> by AW, I guess? I'm not sure if it will be easy to use this class in AW but I
> don't like the idea of maintaining a separate UI just for AW) and the enum is
in
> components/. I believe ui/ can't depend on components/.

Okay in that case I'm fine with this for now.  Thanks for looking into it!

[Evan Stade, 2015-04-23 01:21:09]

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc
(right):

https://codereview.chromium.org/1082183002/diff/80001/chrome/browser/ui/andro...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc:26:
AutofillKeyboardAccessoryView::~AutofillKeyboardAccessoryView() {
On 2015/04/21 21:02:31, David Trainor wrote:
> On 2015/04/21 18:10:30, Evan Stade wrote:
> > On 2015/04/21 06:55:47, David Trainor wrote:
> > > On 2015/04/21 00:48:51, Evan Stade wrote:
> > > > On 2015/04/20 23:29:43, David Trainor wrote:
> > > > > Should we be clearing Java's native pointer of this object in Hide()
(or
> > > > here)? 
> > > > > We don't want something trying to call back into this object when it's
> > gone.
> > > 
> > > > It
> > > > > could happen if the handler's runnable runs after we destroy this
> object.
> > > > 
> > > > I copied this pattern from AutofillPopupBridge/AutofillPopupViewAndroid.
I
> > > > changed to create/init to avoid this problem.
> > > 
> > > I might be misunderstanding the lifecycle of this class.  Does this class
> get
> > > killed/recreated during runtime or is it a singleton that's created only
> once?
> > 
> > > If it gets killed/recreated during runtime then I think my concern still
> > > applies.  Just because this comes from an existing pattern doesn't make it
> > safe.
> > 
> > No, but if it comes from a pattern that's existed for years without any
crash
> > reports, that's an indication of something. Regardless, I have changed the
way
> > it works so there's no handler.
> 
> I don't think that's a valid argument for proving that a piece of code is
safe.
> 
> > 
> > I don't see the pattern you're talking about used in any of the other
C++/java
> > jni class pairs, but maybe you can point me to one. AFAIU, java_object_
> > maintains a java reference to the bridge class. Nothing else holds a
reference
> > to that class, so when this C++ object goes down the java side should also
go
> > down.
> 
> Sorry I wasn't very clear earlier :(.  We're actually talking about the same
> pattern!  The issue is the complete pattern destroys the java -> native
> connection when the native object dies.  The code you copied just didn't do
this
> (and should probably be fixed too!).  Here are some examples of classes that
> follow this pattern and do this (I did a quick code search for upstream code
and
> grabbed a few):
> ContentViewCore.java
> WebContents.java
> NavigationController.java
> MediaDrmBridge.java
> MediaPlayerBridge.java
> AwContentsClientBridge.java
> 
> The issue is, if you don't destroy the Java -> native reference you're 100%
> relying on nobody holding onto a Java reference when native dies.  This is
> fragile - someone touching your AutofillKeyboardAccessory View and adding some
> animation/handler might never think to check the delegate/bridge.  It's also
> unsafe if you interact with the Android framework, like this code does.
> 
> As for this code in particular:
> AutofillKeyboardAccessory holds a reference to
AutofillKeyboardAccessoryBridge. 
> The Android framework holds a reference to the AutofillKeyboardAccessory View.

> The Android framework can do whatever it wants.  In fact a lot of the click
> handling gets posted on their own internal handlers... View.java is actually
> unsafe in that regard (posts a task to notify the click listener). 
AbsListView
> looks somewhat safe for the current version of L (still can post a task, but
> looks like it might check if the View was detached from the window), but might
> be unsafe for older Android versions or custom OEM versions.
> 
> These are incredibly unlikely timing bugs, but just because it's unlikely
> doesn't mean we should ignore it.  Especially when the fix is as simple as one
> of:
> 
> 1. Inside AutofillKeyboardAccessoryBridge#dismiss() we just clear our native
> pointer and do != 0 checks everywhere we use it.
> 2. Add a AutofillKeyboardAccessoryBridge#clearNativePtr() that gets called
from
> the destructor of native AutofillKeyboardAccessoryView and do != 0 checks
> everywhere we use the native pointer.
> 
> Sorry for the mix up on language around the "pattern."  Let me know if this
make
> sense!
> 
> > 
> > >  If the Java component lives around long enough to try to poke this native
> > > object after it's destroyed it'll be a bug.  The easiest way to fix that
is
> to
> > > clear the native pointer in Java when this class goes away.
> > 

Done. Thanks for explanation.

[David Trainor- moved to gerrit, 2015-04-23 17:47:14]

lgtm

[Evan Stade, 2015-04-24 02:20:32]

ping newt

[newt (away), 2015-04-24 17:45:12]

A few small comments.

Two bigger picture comments:
 - Can filtering out separator items be dealt with inside components/autofill to
prevent the duplicated constant from getting out of sync.
 - Is WindowAndroid a reasonable means for passing around the accessory view
parent? I'm not very familiar with WindowAndroid or its intended purpose, but I
think it's worth checking with someone who is.

https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:26:
public AutofillKeyboardAccessoryBridge() {
It appears this should be private.

https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:53:
public void init(long nativeAutofillKeyboardAccessory, WindowAndroid
windowAndroid) {
can this be private?

https://codereview.chromium.org/1082183002/diff/140001/chrome/browser/ui/andr...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h
(right):

https://codereview.chromium.org/1082183002/diff/140001/chrome/browser/ui/andr...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:22: // A
suggestion view that acts as an alternative to the field-attached popup
How does this look on tablets?

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/res/dr...
File ui/android/java/res/drawable/autofill_accessory_view_border.xml (right):

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/res/dr...
ui/android/java/res/drawable/autofill_accessory_view_border.xml:7: <layer-list
xmlns:android="http://schemas.android.com/apk/res/android">
Android doesn't have a great way to draw backgrounds with borders on only
certain sides. Here's another way to do it:

  <inset xmlns:android="http://schemas.android.com/apk/res/android"
      android:insetBottom="-1dp"
      android:insetLeft="-1dp"
      android:insetRight="-1dp">
  
      <shape android:shape="rectangle">
          <stroke android:width="1dp" android:color="@android:color/black" />
          <solid android:color="@android:color/white" />
      </shape>
  
  </inset>

What you have is fine too (just see my comment below).

Also, consider using 1dp instead of 1px. 1px is almost indiscernible on an
xxxhdpi device. (We should probably create @dimen/hairline for something that's
usually 1px, but on super high res devices is 2px. This comes up now and then.)

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/res/dr...
ui/android/java/res/drawable/autofill_accessory_view_border.xml:10: <stroke
android:width="1px" android:color="@android:color/black" />
Defining a <shape> without a <solid> causes trouble on certain devices. They'll
draw the background color as black if you don't specify anything. You can fix
this by adding a <solid> element with the color set to transparent.

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:27:
* The Autofill suggestion view that lists relevant suggestions.
I'd mention where this view appears ("above the keyboard") here.

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:34:
private static final int ITEM_ID_SEPARATOR_ENTRY = -3;
Could you filter out separators in code inside components/autofill when that
constant is accessible? This seems prone to breakage.

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:85:
for (int i = 0; i < suggestions.length; i++) {
you can use an enhanced for loop here:

    for (AutofillSuggestion suggestion : suggestions) {
        ...
    }

[David Trainor- moved to gerrit, 2015-04-24 20:05:15]

On 2015/04/24 17:45:12, newt wrote:
> A few small comments.
> 
> Two bigger picture comments:
>  - Can filtering out separator items be dealt with inside components/autofill
to
> prevent the duplicated constant from getting out of sync.
>  - Is WindowAndroid a reasonable means for passing around the accessory view
> parent? I'm not very familiar with WindowAndroid or its intended purpose, but
I
> think it's worth checking with someone who is.

My 2 cents since I helped come up with this class :).  Yeah it's a reasonable
place.  That's what we originally made it for (exposing hooks for ui/ and
component/ packages to add UX to Android).  Generally they needed the Activity
to show popup dialogs, but they're growing to adding views now, so this makes
sense.

> 
>
https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
> (right):
> 
>
https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:26:
> public AutofillKeyboardAccessoryBridge() {
> It appears this should be private.
> 
>
https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:53:
> public void init(long nativeAutofillKeyboardAccessory, WindowAndroid
> windowAndroid) {
> can this be private?
> 
>
https://codereview.chromium.org/1082183002/diff/140001/chrome/browser/ui/andr...
> File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h
> (right):
> 
>
https://codereview.chromium.org/1082183002/diff/140001/chrome/browser/ui/andr...
> chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:22: // A
> suggestion view that acts as an alternative to the field-attached popup
> How does this look on tablets?
> 
>
https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/res/dr...
> File ui/android/java/res/drawable/autofill_accessory_view_border.xml (right):
> 
>
https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/res/dr...
> ui/android/java/res/drawable/autofill_accessory_view_border.xml:7: <layer-list
> xmlns:android="http://schemas.android.com/apk/res/android">
> Android doesn't have a great way to draw backgrounds with borders on only
> certain sides. Here's another way to do it:
> 
>   <inset xmlns:android="http://schemas.android.com/apk/res/android"
>       android:insetBottom="-1dp"
>       android:insetLeft="-1dp"
>       android:insetRight="-1dp">
>   
>       <shape android:shape="rectangle">
>           <stroke android:width="1dp" android:color="@android:color/black" />
>           <solid android:color="@android:color/white" />
>       </shape>
>   
>   </inset>
> 
> What you have is fine too (just see my comment below).
> 
> Also, consider using 1dp instead of 1px. 1px is almost indiscernible on an
> xxxhdpi device. (We should probably create @dimen/hairline for something
that's
> usually 1px, but on super high res devices is 2px. This comes up now and
then.)
> 
>
https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/res/dr...
> ui/android/java/res/drawable/autofill_accessory_view_border.xml:10: <stroke
> android:width="1px" android:color="@android:color/black" />
> Defining a <shape> without a <solid> causes trouble on certain devices.
They'll
> draw the background color as black if you don't specify anything. You can fix
> this by adding a <solid> element with the color set to transparent.
> 
>
https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
> File
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
> (right):
> 
>
https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
>
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:27:
> * The Autofill suggestion view that lists relevant suggestions.
> I'd mention where this view appears ("above the keyboard") here.
> 
>
https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
>
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:34:
> private static final int ITEM_ID_SEPARATOR_ENTRY = -3;
> Could you filter out separators in code inside components/autofill when that
> constant is accessible? This seems prone to breakage.
> 
>
https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
>
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:85:
> for (int i = 0; i < suggestions.length; i++) {
> you can use an enhanced for loop here:
> 
>     for (AutofillSuggestion suggestion : suggestions) {
>         ...
>     }

[Evan Stade, 2015-04-24 21:19:18]

https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java
(right):

https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:26:
public AutofillKeyboardAccessoryBridge() {
On 2015/04/24 17:45:11, newt wrote:
> It appears this should be private.

Done.

https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:53:
public void init(long nativeAutofillKeyboardAccessory, WindowAndroid
windowAndroid) {
On 2015/04/24 17:45:12, newt wrote:
> can this be private?

Done.

https://codereview.chromium.org/1082183002/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillKeyboardAccessoryBridge.java:68:
private void nativeViewDestroyed() {
also changed the name of this because native* is used for calls into native

https://codereview.chromium.org/1082183002/diff/140001/chrome/browser/ui/andr...
File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h
(right):

https://codereview.chromium.org/1082183002/diff/140001/chrome/browser/ui/andr...
chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:22: // A
suggestion view that acts as an alternative to the field-attached popup
On 2015/04/24 17:45:12, newt wrote:
> How does this look on tablets?

I don't have a tablet handy to test it on, but I expect making it look good
there (i.e. utilizing all the available space) is one of the things we'll
iterate on, design-wise.

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/res/dr...
File ui/android/java/res/drawable/autofill_accessory_view_border.xml (right):

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/res/dr...
ui/android/java/res/drawable/autofill_accessory_view_border.xml:7: <layer-list
xmlns:android="http://schemas.android.com/apk/res/android">
On 2015/04/24 17:45:12, newt wrote:
> Android doesn't have a great way to draw backgrounds with borders on only
> certain sides. Here's another way to do it:
> 
>   <inset xmlns:android="http://schemas.android.com/apk/res/android"
>       android:insetBottom="-1dp"
>       android:insetLeft="-1dp"
>       android:insetRight="-1dp">
>   
>       <shape android:shape="rectangle">
>           <stroke android:width="1dp" android:color="@android:color/black" />
>           <solid android:color="@android:color/white" />
>       </shape>
>   
>   </inset>
> 
> What you have is fine too (just see my comment below).
> 
> Also, consider using 1dp instead of 1px. 1px is almost indiscernible on an
> xxxhdpi device. (We should probably create @dimen/hairline for something
that's
> usually 1px, but on super high res devices is 2px. This comes up now and
then.)

I've gone with your code because it's shorter.

This background is temporary. It doesn't match the mocks, I just added it
because I didn't want to attempt to make everything match the mocks in one go
and this is the most reasonable looking background for now.

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java
(right):

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:27:
* The Autofill suggestion view that lists relevant suggestions.
On 2015/04/24 17:45:12, newt wrote:
> I'd mention where this view appears ("above the keyboard") here.

Done.

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:34:
private static final int ITEM_ID_SEPARATOR_ENTRY = -3;
On 2015/04/24 17:45:12, newt wrote:
> Could you filter out separators in code inside components/autofill when that
> constant is accessible? This seems prone to breakage.

this leads to some ugly ifdefs but I guess it makes this code simpler. done.

https://codereview.chromium.org/1082183002/diff/140001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/autofill/AutofillKeyboardAccessory.java:85:
for (int i = 0; i < suggestions.length; i++) {
On 2015/04/24 17:45:12, newt wrote:
> you can use an enhanced for loop here:
> 
>     for (AutofillSuggestion suggestion : suggestions) {
>         ...
>     }

this code is removed

[newt (away), 2015-04-24 21:48:11]

lgtm

[Evan Stade, 2015-04-27 18:35:07]

On 2015/04/24 21:48:11, newt wrote:
> lgtm

ping aruslan for chrome/browser/ui/android/autofill/

[Evan Stade, 2015-04-27 19:46:47]

On 2015/04/27 18:35:07, Evan Stade wrote:
> On 2015/04/24 21:48:11, newt wrote:
> > lgtm
> 
> ping aruslan for chrome/browser/ui/android/autofill/

how do I make this warning go away:
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...

[newt (away), 2015-04-27 20:06:00]

findbugs doesn't seem to care about the asserts, but it is concerned that
WindowAndroid.getActivity().get() is always null in the default WindowAndroid
implementation.

I think you could fix the findbugs error and probably improve the code a bit by
explicitly passing a Context as an additional argument to
AutofillKeyboardAccessory's constructor. AutofillKeyboardAccessoryBridge.init(),
which constructs the AutofillKeyboardAccessory, already checks to ensure that it
has a non-null activity. This would make the correctness easier to verify for
humans as well.

[David Trainor- moved to gerrit, 2015-04-27 20:07:56]

Huh weird!

Maybe try grabbing a strong reference to the Activity (Activity activity =
windowAndroid.getActivity().get()) then use that later instead of re-querying
for it?

If it ends up being an invalid warning you can add
@SuppressFBWarnings("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE") on the method to
suppress it.

[aruslan, 2015-04-27 22:55:30]

lgtm

[Evan Stade, 2015-04-28 00:10:44]

On 2015/04/27 22:55:30, aruslan wrote:
> lgtm

thanks.

I went with dtrainor's suggestion re: findbugs because it seems redundant to me
to pass both an object and a derivative of that object.

[Evan Stade, 2015-04-28 00:10:50]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-04-28 00:10:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
rouslan@chromium.org, dtrainor@chromium.org, newt@chromium.org,
aruslan@chromium.org
Link to the patchset: https://codereview.chromium.org/1082183002/#ps280001
(title: "fix test")

[commit-bot: I haz the power, 2015-04-28 00:11:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1082183002/280001

[newt (away), 2015-04-28 00:18:47]

On 2015/04/28 00:10:44, Evan Stade wrote:
> On 2015/04/27 22:55:30, aruslan wrote:
> > lgtm
> 
> thanks.
> 
> I went with dtrainor's suggestion re: findbugs because it seems redundant to
me
> to pass both an object and a derivative of that object.

I think it's OK either way, though I'd argue it's not redundant.
WindowAndroid.getActivity() is null in many cases, whereas a Context passed as
an extra parameter would be guaranteed non-null. As it is, you're assuming that
WindowAndroid is really an ActivityWindowAndroid, which is a hidden assumption
that other developers probably won't notice.

[commit-bot: I haz the power, 2015-04-28 00:20:44]

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

[commit-bot: I haz the power, 2015-04-28 00:20:45]

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

[please use gerrit instead, 2015-04-28 16:58:05]

Missing LGTM from an OWNER for these files:
    tools/metrics/histograms/histograms.xml

[Evan Stade, 2015-04-28 17:39:00]

estade@chromium.org changed reviewers:
+ isherman@chromium.org

[Evan Stade, 2015-04-28 17:39:13]

TBR isherman for histograms.xml

[Evan Stade, 2015-04-28 17:39:17]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-04-28 17:39:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1082183002/280001

[commit-bot: I haz the power, 2015-04-28 18:08:05]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-04-28 18:09:06]

Patchset 15 (id:??) landed as
https://crrev.com/b5efbab6f71b0e2660e3d5231d99df698c0e5b38
Cr-Commit-Position: refs/heads/master@{#327324}

[Ilya Sherman, 2015-04-28 19:54:11]

histograms.xml lgtm