Expanded Autofill Credit Card Popup Layout Experiment in Android.

Expanded Autofill Credit Card Popup Layout Experiment in Android.
Assets are yet to be added to this change.
BUG=664367
Committed: https://crrev.com/9ecb43870649edbe295031612d18d074536c9ada
Cr-Commit-Position: refs/heads/master@{#437960}

[csashi, 2016-11-29 02:17:17]

Description was changed from

==========
Expanded Autofill Credit Card Popup Layout Experiment in Android.
Assets are yet to be added to this change.
BUG=664367
==========

to

==========
Expanded Autofill Credit Card Popup Layout Experiment in Android.
Assets are yet to be added to this change.
BUG=664367
==========

[csashi, 2016-11-29 02:17:17]

csashi@google.com changed reviewers:
+ mathp@chromium.org

[csashi, 2016-11-29 02:17:43]

Hi Mathieu,
Can you take a look? Thanks.
-sashi.

[csashi, 2016-11-30 20:16:00]

On 2016/11/29 02:17:43, csashi wrote:
> Hi Mathieu,
> Can you take a look? Thanks.
> -sashi.

Hi Mathieu,
Turns out we don't need new card icons for the experiment.
I separated out the experiment flag to combine card number and expiration date
in a single line.
Can you take a look? Thanks,
-sashi.

[Mathieu, 2016-11-30 21:27:36]

I had started the review before your patchset. Here are some comments and I'll
start again :)

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:89: int
background_color;
lines 89-94 could be an anonymous function to be reused in three of your new
functions here.

GetCreditCardPopupParameterIntValue(kAutofillCreditCardPopupBackgroundColorKey)

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:114: return
param_value.compare("true") == 0;
what about 

return param_value == "true"?

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:134: void
ModifyAutofillCreditCardSuggestion(Suggestion* suggestion) {
Add a comment detailing the final result (it's a bit obscure to read now :))

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:135: if
(IsAutofillCreditCardPopupLayoutExperimentEnabled()) {
DCHECK(IsAutofillCreditCardPopupLayoutExperimentEnabled())?

It should be a precondition that this is true, so let's not check again in
release builds

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_experiments.h (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.h:81: // Illegal include:
"third_party/skia/include/core/SkColor.h"
Can you try adding a DEPS rule in components/autofill/DEPS?

Example
https://cs.chromium.org/chromium/src/components/infobars/DEPS?q=DEPS+third_pa...

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:400: if
(!suggestion_value.empty())
why would it be empty?

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:296: // TODO(csashi):
Should we check GetStorableType()?
I believe this is fine as is.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:100: // Whether the |field|
is for a credit card number.
nit: Current implementation also applies to credit card name, expiration, so
please adjust the comment.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill_str...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill_str...
components/autofill_strings.grdp:156: Chrome Autofill settings...
Make sure to update these :)

https://codereview.chromium.org/2531223003/diff/20001/components/autofill_str...
components/autofill_strings.grdp:166: exp
should we use a format string that includes the actual expiration date? I'm
worried the ordering between exp and the date could be different in other
locales.

https://codereview.chromium.org/2531223003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2531223003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:34: //
|AutofillPopupLayoutModel|. 0 == SK_ColorTRANSPARENT.
nit: let's not mention Autofill

[csashi, 2016-12-01 01:13:44]

Hi Mathieu,
Please take a look. Thanks,
-sashi.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_experiments.cc (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:89: int
background_color;
On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> lines 89-94 could be an anonymous function to be reused in three of your new
> functions here.
> 
>
GetCreditCardPopupParameterIntValue(kAutofillCreditCardPopupBackgroundColorKey)

Done.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:114: return
param_value.compare("true") == 0;
On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> what about 
> 
> return param_value == "true"?

Done.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:134: void
ModifyAutofillCreditCardSuggestion(Suggestion* suggestion) {
On 2016/11/30 21:27:34, Mathieu Perreault wrote:
> Add a comment detailing the final result (it's a bit obscure to read now :))

Done.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.cc:135: if
(IsAutofillCreditCardPopupLayoutExperimentEnabled()) {
On 2016/11/30 21:27:34, Mathieu Perreault wrote:
> DCHECK(IsAutofillCreditCardPopupLayoutExperimentEnabled())?
> 
> It should be a precondition that this is true, so let's not check again in
> release builds

Done.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_experiments.h (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_experiments.h:81: // Illegal include:
"third_party/skia/include/core/SkColor.h"
On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> Can you try adding a DEPS rule in components/autofill/DEPS?
> 
> Example
>
https://cs.chromium.org/chromium/src/components/infobars/DEPS?q=DEPS+third_pa...

Done.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:400: if
(!suggestion_value.empty())
On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> why would it be empty?

I wasn't sure how the resource strings get populated in different locales. I was
guarding against a case where the string was not yet translated. If that is not
possible, I can remove this. I changed code to always use
IDS_AUTOFILL_CREDIT_CARD_OPTIONS_POPUP for the settings text that we can later
override in an experiment.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.cc (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.cc:296: // TODO(csashi):
Should we check GetStorableType()?
On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> I believe this is fine as is.

Done.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:100: // Whether the |field|
is for a credit card number.
On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> nit: Current implementation also applies to credit card name, expiration, so
> please adjust the comment.

Done.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill_str...
File components/autofill_strings.grdp (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill_str...
components/autofill_strings.grdp:156: Chrome Autofill settings...
On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> Make sure to update these :)

This was intentional. That way we can change to "Add or Remove Cards..." in an
experiment. If experiment works we can change the default - i.e. this text.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill_str...
components/autofill_strings.grdp:166: exp
On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> should we use a format string that includes the actual expiration date? I'm
> worried the ordering between exp and the date could be different in other
> locales.

Done.

https://codereview.chromium.org/2531223003/diff/20001/ui/android/java/src/org...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2531223003/diff/20001/ui/android/java/src/org...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:34: //
|AutofillPopupLayoutModel|. 0 == SK_ColorTRANSPARENT.
On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> nit: let's not mention Autofill

Done.

[Mathieu, 2016-12-01 21:59:47]

Thanks, this is looking great. Some more comments

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:400: if
(!suggestion_value.empty())
On 2016/12/01 01:13:43, csashi wrote:
> On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> > why would it be empty?
> 
> I wasn't sure how the resource strings get populated in different locales. I
was
> guarding against a case where the string was not yet translated. If that is
not
> possible, I can remove this. I changed code to always use
> IDS_AUTOFILL_CREDIT_CARD_OPTIONS_POPUP for the settings text that we can later
> override in an experiment.

yes assume it is translated

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:73:
layout_model_(this, delegate->IsCreditCardField()),
as mentioned, would prefer IsCreditCardPopup to make clear it's about the full
popup

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right):

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_layout_model.cc:250: return
is_credit_card_field_ ?
return is_credit_card_field_ && IsAutofill...

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_layout_model.cc:269: return
is_credit_card_field_ ? IsCreditCardIconInPopupAtLeft() : false;
same here

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right):

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_layout_model.h:34: bool
is_credit_card_field);
nit: the name makes it seem like there is one layout model per field. How about
a different name to avoid confusion? |is_credit_card_popup|, for example.

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/DEPS
File components/autofill/DEPS (right):

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/DEP...
components/autofill/DEPS:7: "+third_party/skia",  # For SkColor in
autofill_experiments
nit: wouldn't necessarily say why here

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/and...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java
(right):

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java:31:
* Has to be kept in sync with {@code POPUP_ITEM_ID_SEPARATOR} enum in
Add a similar TODO to
https://cs.chromium.org/chromium/src/components/autofill/android/java/src/org...

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.h:151: // FIXME
unclear why this is here?

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:100: // Whether the |field|
belongs to |CREDIT_CARD| |FieldTypeGroup|.
// Whether the |field| belongs to the CREDIT_CARD FieldTypeGroup.

https://codereview.chromium.org/2531223003/diff/60001/ui/android/java/res/lay...
File ui/android/java/res/layout/dropdown_item_icon_at_left.xml (right):

https://codereview.chromium.org/2531223003/diff/60001/ui/android/java/res/lay...
ui/android/java/res/layout/dropdown_item_icon_at_left.xml:1: <?xml version="1.0"
encoding="utf-8"?>
Can you look at the discussion here [1] and see whether it applies, about
creating a separate XML file just for the left icon?

https://codereview.chromium.org/2510283002/#msg15

[csashi, 2016-12-02 05:15:29]

Hi Mathieu,
Can you take a look? Thanks,
-sashi.

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.cc (right):

https://codereview.chromium.org/2531223003/diff/20001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.cc:400: if
(!suggestion_value.empty())
On 2016/12/01 21:59:46, Mathieu Perreault wrote:
> On 2016/12/01 01:13:43, csashi wrote:
> > On 2016/11/30 21:27:35, Mathieu Perreault wrote:
> > > why would it be empty?
> > 
> > I wasn't sure how the resource strings get populated in different locales. I
> was
> > guarding against a case where the string was not yet translated. If that is
> not
> > possible, I can remove this. I changed code to always use
> > IDS_AUTOFILL_CREDIT_CARD_OPTIONS_POPUP for the settings text that we can
later
> > override in an experiment.
> 
> yes assume it is translated

Done.

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right):

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:73:
layout_model_(this, delegate->IsCreditCardField()),
On 2016/12/01 21:59:46, Mathieu Perreault wrote:
> as mentioned, would prefer IsCreditCardPopup to make clear it's about the full
> popup

Done.

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right):

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_layout_model.cc:250: return
is_credit_card_field_ ?
On 2016/12/01 21:59:46, Mathieu Perreault wrote:
> return is_credit_card_field_ && IsAutofill...

Done.

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_layout_model.cc:269: return
is_credit_card_field_ ? IsCreditCardIconInPopupAtLeft() : false;
On 2016/12/01 21:59:46, Mathieu Perreault wrote:
> same here

Done.

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right):

https://codereview.chromium.org/2531223003/diff/60001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_popup_layout_model.h:34: bool
is_credit_card_field);
On 2016/12/01 21:59:47, Mathieu Perreault wrote:
> nit: the name makes it seem like there is one layout model per field. How
about
> a different name to avoid confusion? |is_credit_card_popup|, for example.

Done.

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/DEPS
File components/autofill/DEPS (right):

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/DEP...
components/autofill/DEPS:7: "+third_party/skia",  # For SkColor in
autofill_experiments
On 2016/12/01 21:59:47, Mathieu Perreault wrote:
> nit: wouldn't necessarily say why here

Done.

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/and...
File
components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java
(right):

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/and...
components/autofill/android/java/src/org/chromium/components/autofill/AutofillPopup.java:31:
* Has to be kept in sync with {@code POPUP_ITEM_ID_SEPARATOR} enum in
On 2016/12/01 21:59:47, Mathieu Perreault wrote:
> Add a similar TODO to
>
https://cs.chromium.org/chromium/src/components/autofill/android/java/src/org...

Done.

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_external_delegate.h (right):

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_external_delegate.h:151: // FIXME
On 2016/12/01 21:59:47, Mathieu Perreault wrote:
> unclear why this is here?

By "this", do you mean the "FIXME" or the "is_credit_card_field_"? The
"is_credit_card_field" is here so we can show the current settings text in
ApplyAutofillOptions. By default we will show "Chrome Autofill Settings". In an
experiment, we can try replacing with "Add or Remove Cards.

I have removed the FIXME.

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/cor...
File components/autofill/core/browser/autofill_manager.h (right):

https://codereview.chromium.org/2531223003/diff/60001/components/autofill/cor...
components/autofill/core/browser/autofill_manager.h:100: // Whether the |field|
belongs to |CREDIT_CARD| |FieldTypeGroup|.
On 2016/12/01 21:59:47, Mathieu Perreault wrote:
> // Whether the |field| belongs to the CREDIT_CARD FieldTypeGroup.

Done.

https://codereview.chromium.org/2531223003/diff/60001/ui/android/java/res/lay...
File ui/android/java/res/layout/dropdown_item_icon_at_left.xml (right):

https://codereview.chromium.org/2531223003/diff/60001/ui/android/java/res/lay...
ui/android/java/res/layout/dropdown_item_icon_at_left.xml:1: <?xml version="1.0"
encoding="utf-8"?>
On 2016/12/01 21:59:47, Mathieu Perreault wrote:
> Can you look at the discussion here [1] and see whether it applies, about
> creating a separate XML file just for the left icon?
> 
> https://codereview.chromium.org/2510283002/#msg15

Yes, it applies. I have modified my CL to be similar to this change.

[Mathieu, 2016-12-02 17:57:34]

lgtm, you will need some more owners on the Android side.

[csashi, 2016-12-02 18:17:05]

csashi@google.com changed reviewers:
+ aelias@chromium.org, bsalomon@google.com, isherman@chromium.org,
michaelbai@chromium.org, miguelg@chromium.org

[csashi, 2016-12-02 18:18:15]

On 2016/12/02 17:57:34, Mathieu Perreault wrote:
> lgtm, you will need some more owners on the Android side.

Hi,
Can you please review for owners approval? Thanks,
-sashi.

[aelias_OOO_until_Jul13, 2016-12-02 20:24:59]

content/public/android lgtm

[michaelbai, 2016-12-02 21:28:07]

android_webview LGTM

[bsalomon, 2016-12-02 22:21:15]

skia lgtm

[Ilya Sherman, 2016-12-03 00:02:18]

I'm assuming you wanted me to review chrome/browser/ui/autofill* and
components/password_manager/*?  In the future, please clarify when requesting a
review =)

https://codereview.chromium.org/2531223003/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right):

https://codereview.chromium.org/2531223003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_layout_model.cc:256:
GetCreditCardPopupBackgroundColor() : SK_ColorTRANSPARENT;
Should this be checking for IsPopupLayoutExperimentEnabled() rather than
is_credit_card_popup_?  I think there's currently an implicit assumption that
the 0 returned from GetCreditCardPopupBackgroundColor() is equivalent to
SK_ColorTRANSPARENT... which might be true, but is not obvious to, say, me.

[csashi, 2016-12-03 00:40:07]

Hi Ilya,

I used the list that "git cl OWNERS" showed me. Next time I will check who
should be assigned which sub-directory.

Please take a look at the updated comments. Thanks.
-sashi.

https://codereview.chromium.org/2531223003/diff/100001/chrome/browser/ui/auto...
File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right):

https://codereview.chromium.org/2531223003/diff/100001/chrome/browser/ui/auto...
chrome/browser/ui/autofill/autofill_popup_layout_model.cc:256:
GetCreditCardPopupBackgroundColor() : SK_ColorTRANSPARENT;
On 2016/12/03 00:02:18, Ilya Sherman wrote:
> Should this be checking for IsPopupLayoutExperimentEnabled() rather than
> is_credit_card_popup_?  I think there's currently an implicit assumption that
> the 0 returned from GetCreditCardPopupBackgroundColor() is equivalent to
> SK_ColorTRANSPARENT... which might be true, but is not obvious to, say, me.

GetCreditCardPopupBackgroundColor checks if the popup experiment is enabled.

I have clarified that the 0 returned from the functions is chosen because it
maps to SK_ColorTRANSPARENT.

[Ilya Sherman, 2016-12-03 00:59:19]

On 2016/12/03 00:40:07, csashi wrote:
> Hi Ilya,
> 
> I used the list that "git cl OWNERS" showed me. Next time I will check who
> should be assigned which sub-directory.
> 
> Please take a look at the updated comments. Thanks.
> -sashi.

Thanks.  chrome/browser/ui/autofill* and components/password_manager/* lgtm.

[csashi, 2016-12-05 19:23:04]

The CQ bit was checked by csashi@google.com

[csashi, 2016-12-05 19:23:04]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
mathp@chromium.org, michaelbai@chromium.org, bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/2531223003/#ps120001
(title: "Clarify returned SkColor value is 0 because it maps to
SK_ColorTRANSPARENT")

[commit-bot: I haz the power, 2016-12-05 19:23:35]

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-05 19:27:26]

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

[commit-bot: I haz the power, 2016-12-05 19:27:31]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[csashi, 2016-12-05 19:53:04]

The CQ bit was checked by csashi@google.com

[csashi, 2016-12-05 19:53:04]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
mathp@chromium.org, michaelbai@chromium.org, bsalomon@google.com,
isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2531223003/#ps140001
(title: "Clarify returned SkColor value is 0 because it maps to
SK_ColorTRANSPARENT")

[commit-bot: I haz the power, 2016-12-05 19:53:51]

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-05 20:07:11]

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

[commit-bot: I haz the power, 2016-12-05 20:07:13]

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

[csashi, 2016-12-05 20:11:30]

csashi@google.com changed reviewers:
+ dtrainor@chromium.org, rouslan@chromium.org

[csashi, 2016-12-05 20:14:24]

On 2016/12/05 20:07:13, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Hi Rouslan,
Can you approve for chrome/browser/ui/android/autofill?

Hi Miguel,
Can you approve for chrome/android/javatests and ui/android?

Thanks,
-sashi.

[please use gerrit instead, 2016-12-05 20:37:13]

On 2016/12/05 20:14:24, csashi wrote:
> On 2016/12/05 20:07:13, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Hi Rouslan,
> Can you approve for chrome/browser/ui/android/autofill?
> 
> Hi Miguel,
> Can you approve for chrome/android/javatests and ui/android?
> 
> Thanks,
> -sashi.

I don't own chrome/browser/ui/android/autofill, but Miguel does.

Miguel, ptal chrome/browser/ui/android/autofill as well.

[please use gerrit instead, 2016-12-05 20:37:33]

rouslan@chromium.org changed reviewers:
- rouslan@chromium.org

[csashi, 2016-12-07 18:25:25]

On 2016/12/05 20:37:13, rouslan wrote:
> On 2016/12/05 20:14:24, csashi wrote:
> > On 2016/12/05 20:07:13, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > 
> > Hi Rouslan,
> > Can you approve for chrome/browser/ui/android/autofill?
> > 
> > Hi Miguel,
> > Can you approve for chrome/android/javatests and ui/android?
> > 
> > Thanks,
> > -sashi.
> 
> I don't own chrome/browser/ui/android/autofill, but Miguel does.
> 
> Miguel, ptal chrome/browser/ui/android/autofill as well.

Hi Mathieu,
Per your comment and UI review, we now use experiment drop down item height to
all popups.
Secondly, IDS_AUTOFILL_SCAN_CREDIT_CARD already displays the camera icon at the
start.
Please take a look.

Hi Miguel,
I still need your approval.

Thanks,
-sashi.

[csashi, 2016-12-08 16:58:59]

csashi@google.com changed reviewers:
+ msw@chromium.org, twellington@chromium.org
- miguelg@chromium.org

[csashi, 2016-12-08 17:03:28]

On 2016/12/07 18:25:25, csashi wrote:
> On 2016/12/05 20:37:13, rouslan wrote:
> > On 2016/12/05 20:14:24, csashi wrote:
> > > On 2016/12/05 20:07:13, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > > 
> > > Hi Rouslan,
> > > Can you approve for chrome/browser/ui/android/autofill?
> > > 
> > > Hi Miguel,
> > > Can you approve for chrome/android/javatests and ui/android?
> > > 
> > > Thanks,
> > > -sashi.
> > 
> > I don't own chrome/browser/ui/android/autofill, but Miguel does.
> > 
> > Miguel, ptal chrome/browser/ui/android/autofill as well.
> 
> Hi Mathieu,
> Per your comment and UI review, we now use experiment drop down item height to
> all popups.
> Secondly, IDS_AUTOFILL_SCAN_CREDIT_CARD already displays the camera icon at
the
> start.
> Please take a look.
> 
> Hi Miguel,
> I still need your approval.
> 
> Thanks,
> -sashi.

Hi,
I am adding new reviewers because I haven't heard from Miguel.

Hi Theresa,
Can you approve for ui/android?

Hi Mike,
Can you approve for chrome/browser/ui/android/autofill/ ? The other OWNERS under
chrome/browser/ui/android are OOO.

Hi David,
Can you approve for
chrome/android/javatests/src/org/chromium/chrome/browser/autofill/ ?

Thanks, -sashi.

[Theresa, 2016-12-08 18:26:20]

https://codereview.chromium.org/2531223003/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java
(right):

https://codereview.chromium.org/2531223003/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java:48:
mContext, items, null /* separators */, 0 /* backgroundColor */,
Should the background color passed in here be Color.Transparent?

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/res/va...
File ui/android/java/res/values/colors.xml (left):

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/res/va...
ui/android/java/res/values/colors.xml:8: <color
name="dropdown_divider_color">#E5E5E5</color>
This XML file's indentation was correct (it should be 4 spaces).

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:34: // native code. 0
== SK_ColorTRANSPARENT is default value if not overriden by experiment.
I'm a little confused by this comment. It looks like if these are set to 0, the
values in colors.xml will be used for the divider line and the background will
be transparent?

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:37: // 0 => use the
default value for |dropdown_item_height| specified in XML resource file.
This comment should go in the JavaDoc for the constructor -- something like: The
height of the drop down items in dp(?). If this is 0, the
@dimens/dropdown_item_height will be used.


Alternatively, instead of using 0 to indicate "use the default" you could
introduce a second constructor. See comment below.

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:92: } else if
(mDividerColor == 0) {
Since 0 is a valid color (transparent), would it make sense to use -1 to
indicate that the default dropdown_divider_color should be used? Or (probably
better) there could be two constructors, one that takes a dividerColor and one
that passes R.color.dropdown_divider_color as the dividerColor to the other
constructor.

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownDividerDrawable.java (right):

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownDividerDrawable.java:18: // Paints
canvas with |mBackgroundColor|, if |mBackgroundColor| is not 0.
This should go as a JavaDoc on the constructor.

[Mathieu, 2016-12-08 18:35:10]

still lgtm with nit

https://codereview.chromium.org/2531223003/diff/180001/chrome/browser/ui/andr...
File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right):

https://codereview.chromium.org/2531223003/diff/180001/chrome/browser/ui/andr...
chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:105:
controller_->layout_model().IsIconAtStart(),
Can you pass |suggestion_id| to ..LayoutModel::IsIconAtStart, that way we can
remove the check in AutofillSuggestion.isIconAtStart that checks about the HTTP
ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE and rather have it in the layout model.

[David Trainor- moved to gerrit, 2016-12-08 18:40:32]

chrome/android/javatests/src/org/chromium/chrome/browser/autofill/ lgtm

[csashi, 2016-12-08 20:09:01]

Hi Theresa,
Please take a look. Thanks.
-sashi.

https://codereview.chromium.org/2531223003/diff/180001/chrome/browser/ui/andr...
File chrome/browser/ui/android/autofill/autofill_popup_view_android.cc (right):

https://codereview.chromium.org/2531223003/diff/180001/chrome/browser/ui/andr...
chrome/browser/ui/android/autofill/autofill_popup_view_android.cc:105:
controller_->layout_model().IsIconAtStart(),
On 2016/12/08 18:35:10, Mathieu Perreault wrote:
> Can you pass |suggestion_id| to ..LayoutModel::IsIconAtStart, that way we can
> remove the check in AutofillSuggestion.isIconAtStart that checks about the
HTTP
> ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE and rather have it in the layout
model.
> 

Done.

https://codereview.chromium.org/2531223003/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java
(right):

https://codereview.chromium.org/2531223003/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/input/SelectPopupDropdown.java:48:
mContext, items, null /* separators */, 0 /* backgroundColor */,
On 2016/12/08 18:26:20, Theresa Wellington wrote:
> Should the background color passed in here be Color.Transparent?

Done.

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/res/va...
File ui/android/java/res/values/colors.xml (left):

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/res/va...
ui/android/java/res/values/colors.xml:8: <color
name="dropdown_divider_color">#E5E5E5</color>
On 2016/12/08 18:26:20, Theresa Wellington wrote:
> This XML file's indentation was correct (it should be 4 spaces).

Done.

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:34: // native code. 0
== SK_ColorTRANSPARENT is default value if not overriden by experiment.
On 2016/12/08 18:26:20, Theresa Wellington wrote:
> I'm a little confused by this comment. It looks like if these are set to 0,
the
> values in colors.xml will be used for the divider line and the background will
> be transparent?

Done.

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:37: // 0 => use the
default value for |dropdown_item_height| specified in XML resource file.
On 2016/12/08 18:26:20, Theresa Wellington wrote:
> This comment should go in the JavaDoc for the constructor -- something like:
The
> height of the drop down items in dp(?). If this is 0, the
> @dimens/dropdown_item_height will be used.
> 
> 
> Alternatively, instead of using 0 to indicate "use the default" you could
> introduce a second constructor. See comment below.

Done.

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:92: } else if
(mDividerColor == 0) {
On 2016/12/08 18:26:20, Theresa Wellington wrote:
> Since 0 is a valid color (transparent), would it make sense to use -1 to
> indicate that the default dropdown_divider_color should be used? Or (probably
> better) there could be two constructors, one that takes a dividerColor and one
> that passes R.color.dropdown_divider_color as the dividerColor to the other
> constructor.

I was concerned -1 could also be interpreted as a color. Also, for consistency,
I wanted to use 0 as the default value for all the experiment parameters.

I considered using different constructors but we would need at least 4
constructors.
1. All defaults (i.e. no experiment)
2. Only specify the height (this is a variation we want to try).
3. Change background color and divider color.
4. Change divider color only.

If I had multiple constructors I would push this logic down to
AutofillPopup.java which would need to decide which constructor to use for
DropdownAdapter. Seemed simpler to have the experiment logic in 1 place. 

I added some clarifying comments. Please let me know if that helps.

[Theresa, 2016-12-08 21:38:17]

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:92: } else if
(mDividerColor == 0) {
On 2016/12/08 20:09:01, csashi wrote:
> On 2016/12/08 18:26:20, Theresa Wellington wrote:
> > Since 0 is a valid color (transparent), would it make sense to use -1 to
> > indicate that the default dropdown_divider_color should be used? Or
(probably
> > better) there could be two constructors, one that takes a dividerColor and
one
> > that passes R.color.dropdown_divider_color as the dividerColor to the other
> > constructor.
> 
> I was concerned -1 could also be interpreted as a color.

Good point. It looks like -1 is the value for Color.WHITE.

> Also, for consistency,
> I wanted to use 0 as the default value for all the experiment parameters.
> 
> I considered using different constructors but we would need at least 4
> constructors.
> 1. All defaults (i.e. no experiment)
> 2. Only specify the height (this is a variation we want to try).
> 3. Change background color and divider color.
> 4. Change divider color only.
> 
> If I had multiple constructors I would push this logic down to
> AutofillPopup.java which would need to decide which constructor to use for
> DropdownAdapter. Seemed simpler to have the experiment logic in 1 place. 

Ah, I see. The values are coming from native which makes this a little more
challenging.

> 
> I added some clarifying comments. Please let me know if that helps.

Since this is a ui component class, if the background and divider line color are
configurable Color.Transparent should be allowed. Could these param's be
Integer's with null meaning "use the default"? The autofill code could can map a
native value of 0 -> null.

https://codereview.chromium.org/2531223003/diff/200001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2531223003/diff/200001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:42: * @param
backgroundColor popup background color.
nit: capitalize Popup

https://codereview.chromium.org/2531223003/diff/200001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownDividerDrawable.java (right):

https://codereview.chromium.org/2531223003/diff/200001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownDividerDrawable.java:21: * Creates a
drawable to draw a divider line that separate the list of {@code DropdownItem}
and,
s/separate/separates

{@link DropdownItem}

[msw, 2016-12-08 21:40:09]

c/b/ui/android rubber stamp lgtm

[csashi, 2016-12-08 23:03:30]

Hi Theresa,
Please take a look. Thanks.
-sashi.

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2531223003/diff/180001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:92: } else if
(mDividerColor == 0) {
On 2016/12/08 21:38:17, Theresa Wellington wrote:
> On 2016/12/08 20:09:01, csashi wrote:
> > On 2016/12/08 18:26:20, Theresa Wellington wrote:
> > > Since 0 is a valid color (transparent), would it make sense to use -1 to
> > > indicate that the default dropdown_divider_color should be used? Or
> (probably
> > > better) there could be two constructors, one that takes a dividerColor and
> one
> > > that passes R.color.dropdown_divider_color as the dividerColor to the
other
> > > constructor.
> > 
> > I was concerned -1 could also be interpreted as a color.
> 
> Good point. It looks like -1 is the value for Color.WHITE.
> 
> > Also, for consistency,
> > I wanted to use 0 as the default value for all the experiment parameters.
> > 
> > I considered using different constructors but we would need at least 4
> > constructors.
> > 1. All defaults (i.e. no experiment)
> > 2. Only specify the height (this is a variation we want to try).
> > 3. Change background color and divider color.
> > 4. Change divider color only.
> > 
> > If I had multiple constructors I would push this logic down to
> > AutofillPopup.java which would need to decide which constructor to use for
> > DropdownAdapter. Seemed simpler to have the experiment logic in 1 place. 
> 
> Ah, I see. The values are coming from native which makes this a little more
> challenging.
> 
> > 
> > I added some clarifying comments. Please let me know if that helps.
> 
> Since this is a ui component class, if the background and divider line color
are
> configurable Color.Transparent should be allowed. Could these param's be
> Integer's with null meaning "use the default"? The autofill code could can map
a
> native value of 0 -> null.

Done.

https://codereview.chromium.org/2531223003/diff/200001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right):

https://codereview.chromium.org/2531223003/diff/200001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownAdapter.java:42: * @param
backgroundColor popup background color.
On 2016/12/08 21:38:17, Theresa Wellington wrote:
> nit: capitalize Popup

Done.

https://codereview.chromium.org/2531223003/diff/200001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/DropdownDividerDrawable.java (right):

https://codereview.chromium.org/2531223003/diff/200001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/DropdownDividerDrawable.java:21: * Creates a
drawable to draw a divider line that separate the list of {@code DropdownItem}
and,
On 2016/12/08 21:38:17, Theresa Wellington wrote:
> s/separate/separates
> 
> {@link DropdownItem}

Done.

[Theresa, 2016-12-09 00:01:34]

lgtm

[csashi, 2016-12-09 00:40:51]

The CQ bit was checked by csashi@google.com

[csashi, 2016-12-09 00:40:52]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org,
michaelbai@chromium.org, bsalomon@google.com, isherman@chromium.org,
mathp@chromium.org, dtrainor@chromium.org, msw@chromium.org
Link to the patchset: https://codereview.chromium.org/2531223003/#ps220001
(title: "Uses null to encode default value in DropdownAdapter.")

[commit-bot: I haz the power, 2016-12-09 00:41:14]

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-09 01:09:42]

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

[commit-bot: I haz the power, 2016-12-09 01:09:44]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[csashi, 2016-12-09 01:23:55]

The CQ bit was checked by csashi@google.com

[csashi, 2016-12-09 01:23:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, aelias@chromium.org, msw@chromium.org,
mathp@chromium.org, dtrainor@chromium.org, michaelbai@chromium.org,
bsalomon@google.com, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2531223003/#ps240001
(title: "Fixes compile error.")

[commit-bot: I haz the power, 2016-12-09 01:24:36]

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-09 01:54:15]

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

[commit-bot: I haz the power, 2016-12-09 01:54:17]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[csashi, 2016-12-09 02:00:22]

The CQ bit was checked by csashi@google.com

[csashi, 2016-12-09 02:00:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
twellington@chromium.org, aelias@chromium.org, msw@chromium.org,
mathp@chromium.org, dtrainor@chromium.org, michaelbai@chromium.org,
bsalomon@google.com, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2531223003/#ps260001
(title: "Fixes compile error.")

[commit-bot: I haz the power, 2016-12-09 02:01:10]

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-09 02:29:45]

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

[commit-bot: I haz the power, 2016-12-09 02:29:47]

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

[csashi, 2016-12-09 02:43:59]

csashi@google.com changed reviewers:
+ vabr@chromium.org

[csashi, 2016-12-09 02:44:32]

On 2016/12/09 02:29:47, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Hi Vaclav,
Can you please approve for components/password_manager/core/browser/
Thanks,
-sashi.

[csashi, 2016-12-12 17:53:33]

On 2016/12/09 02:44:32, csashi wrote:
> On 2016/12/09 02:29:47, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> 
> Hi Vaclav,
> Can you please approve for components/password_manager/core/browser/
> Thanks,
> -sashi.

Hi Vaclav,
Can you take a look or reassign to someone who can approve? Thanks,
-sashi.

[vabr (Chromium), 2016-12-12 17:58:50]

On 2016/12/12 17:53:33, csashi wrote:
> On 2016/12/09 02:44:32, csashi wrote:
> > On 2016/12/09 02:29:47, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > 
> > Hi Vaclav,
> > Can you please approve for components/password_manager/core/browser/
> > Thanks,
> > -sashi.
> 
> Hi Vaclav,
> Can you take a look or reassign to someone who can approve? Thanks,
> -sashi.

Hi Sashi,

LGTM, sorry for missing your CL previously!

Cheers,
Vaclav

[csashi, 2016-12-12 18:02:32]

The CQ bit was checked by csashi@google.com

[commit-bot: I haz the power, 2016-12-12 18:02:59]

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

[vabr (Chromium), 2016-12-12 18:03:11]

On 2016/12/12 17:58:50, vabr (Chromium) wrote:
> On 2016/12/12 17:53:33, csashi wrote:
> > On 2016/12/09 02:44:32, csashi wrote:
> > > On 2016/12/09 02:29:47, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > > 
> > > Hi Vaclav,
> > > Can you please approve for components/password_manager/core/browser/
> > > Thanks,
> > > -sashi.
> > 
> > Hi Vaclav,
> > Can you take a look or reassign to someone who can approve? Thanks,
> > -sashi.
> 
> Hi Sashi,
> 
> LGTM, sorry for missing your CL previously!
> 
> Cheers,
> Vaclav

(To explain my delay -- I saw Ilya approving your CL in #17, and started to
ignore the watchlist updates from this CL coming to my inbox. But in the
meantime, Ilya removed himself from the password manager OWNERS, making your CL
technically lose the approval.)

[commit-bot: I haz the power, 2016-12-12 23:44:23]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1481565752061600,
"parent_rev": "53afa6e1ad0612bd0d4b51067bd3da39b2373f78", "commit_rev":
"e7b7ce19767cabee639c97503624c84248ef7848"}

[commit-bot: I haz the power, 2016-12-12 23:44:53]

Description was changed from

==========
Expanded Autofill Credit Card Popup Layout Experiment in Android.
Assets are yet to be added to this change.
BUG=664367
==========

to

==========
Expanded Autofill Credit Card Popup Layout Experiment in Android.
Assets are yet to be added to this change.
BUG=664367

Review-Url: https://codereview.chromium.org/2531223003
==========

[commit-bot: I haz the power, 2016-12-12 23:44:55]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-12-12 23:47:38]

Description was changed from

==========
Expanded Autofill Credit Card Popup Layout Experiment in Android.
Assets are yet to be added to this change.
BUG=664367

Review-Url: https://codereview.chromium.org/2531223003
==========

to

==========
Expanded Autofill Credit Card Popup Layout Experiment in Android.
Assets are yet to be added to this change.
BUG=664367

Committed: https://crrev.com/9ecb43870649edbe295031612d18d074536c9ada
Cr-Commit-Position: refs/heads/master@{#437960}
==========

[commit-bot: I haz the power, 2016-12-12 23:47:41]

Patchset 14 (id:??) landed as
https://crrev.com/9ecb43870649edbe295031612d18d074536c9ada
Cr-Commit-Position: refs/heads/master@{#437960}

[csashi, 2016-12-13 05:45:19]

On 2016/12/12 18:03:11, vabr (Chromium) wrote:
> On 2016/12/12 17:58:50, vabr (Chromium) wrote:
> > On 2016/12/12 17:53:33, csashi wrote:
> > > On 2016/12/09 02:44:32, csashi wrote:
> > > > On 2016/12/09 02:29:47, commit-bot: I haz the power wrote:
> > > > > Try jobs failed on following builders:
> > > > >   chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
> > > > >
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
> > > > 
> > > > Hi Vaclav,
> > > > Can you please approve for components/password_manager/core/browser/
> > > > Thanks,
> > > > -sashi.
> > > 
> > > Hi Vaclav,
> > > Can you take a look or reassign to someone who can approve? Thanks,
> > > -sashi.
> > 
> > Hi Sashi,
> > 
> > LGTM, sorry for missing your CL previously!
> > 
> > Cheers,
> > Vaclav
> 
> (To explain my delay -- I saw Ilya approving your CL in #17, and started to
> ignore the watchlist updates from this CL coming to my inbox. But in the
> meantime, Ilya removed himself from the password manager OWNERS, making your
CL
> technically lose the approval.)

Hi Vaclav, Thanks for the explanation. Got it. I am also trying to learn how to
find pending approvals.
-sashi.