Fix list focus after tab key in chrome://settings/autofillEditAddress page.

Fix two focus regressions in chrome://settings/autofillEditAddress page.

Sets list item selection and focus correctly when edit is committed in
name/phone/email lists on chrome://settings/autofillEditAddress page.

"Correctly" means the just-edited list item remains the selected item in
the list, and the next focusable element after the list gets focus. The
list no longer has focus, but if the user gives focus back to the list
using tab/shift+tab then the just-edited list item will receive focus.

This is different than the default InlineEditableItemList behavior
because most addresses will have only a single name/phone/email, so it's
better to move to the next field type rather than to the "Add another"
placeholder of the same field type. I tried to keep the changes
contained to autofill_options_list.js because that's the only page that
needs this focus behavior. Generalizing the code into
inline_editable_list.js might result in more regressions to fix
elsewhere.

The fix implementation is to disable list events during list operations
in autofillOptions.ValuesListItem.onEditCommitted_, so that focus is not
returned back to the list that was just tabbed away from. Then
onEditCommitted_ calls the new
InlineEditableItemList.SelectIndexWithoutFocusing method to set the
selected list item without setting focus back to the list.

Same fix for both regressions.

https://crbug.com/440760 was caused by me.
https://crbug.com/443491 was from a CL by hcarmona@, but we agreed I
should take it because I'm more familiar with this code.

BUG=440760, 443491
Committed: https://crrev.com/10a45b9048e77944987d387b0eff3b5fcfd7a0bc
Cr-Commit-Position: refs/heads/master@{#314675}

[bondd, 2014-12-23 02:11:54]

bondd@chromium.org changed reviewers:
+ dbeam@chromium.org

[bondd, 2014-12-23 02:11:54]

bondd@chromium.org changed required reviewers:
+ dbeam@chromium.org

[bondd, 2014-12-23 02:11:55]

I racked my brain trying to find a better way to do this but no dice. I tried to
keep it contained to autofill_options_list.js because that's the only page that
needs this focus behavior. Generalizing the code into inline_editable_list.js
might result in more regressions to fix elsewhere.

[bondd, 2015-01-06 18:33:33]

bondd@chromium.org changed reviewers:
+ jhawkins@chromium.org
- dbeam@chromium.org

[bondd, 2015-01-06 18:33:33]

bondd@chromium.org changed required reviewers:
+ jhawkins@chromium.org
- dbeam@chromium.org

[James Hawkins, 2015-01-06 18:54:51]

Can you add an explanation to the CL description why this particular fix is
necessary (and what the fix actually is)?

https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:582: // @type {number}
It's legit now to do:

// @private {number}

But this block should be a JSDoc block:

/**
 * Event handling is disabled if > 0.
 * @private {number}
 */
eventsDisabledCount_: 0,

[bondd, 2015-01-06 23:07:24]

Addressed reviewer comments and added requested details to CL description.

https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/inline_editable_list.js:582: // @type {number}
On 2015/01/06 18:54:51, James Hawkins wrote:
> It's legit now to do:
> 
> // @private {number}
> 
> But this block should be a JSDoc block:
> 
> /**
>  * Event handling is disabled if > 0.
>  * @private {number}
>  */
> eventsDisabledCount_: 0,

Done. I made it a JSDoc block, but left it as
  * @type {number}
  * @private
rather than
  * @private {number}
to match the existing style in the rest of the file.

[James Hawkins, 2015-01-08 18:34:38]

On 2015/01/06 23:07:24, bondd wrote:
> Addressed reviewer comments and added requested details to CL description.
> 
>
https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
> File chrome/browser/resources/options/inline_editable_list.js (right):
> 
>
https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
> chrome/browser/resources/options/inline_editable_list.js:582: // @type
{number}
> On 2015/01/06 18:54:51, James Hawkins wrote:
> > It's legit now to do:
> > 
> > // @private {number}
> > 
> > But this block should be a JSDoc block:
> > 
> > /**
> >  * Event handling is disabled if > 0.
> >  * @private {number}
> >  */
> > eventsDisabledCount_: 0,
> 
> Done. I made it a JSDoc block, but left it as
>   * @type {number}
>   * @private
> rather than
>   * @private {number}
> to match the existing style in the rest of the file.

Please add tests.

[bondd, 2015-01-14 00:33:43]

Added tests.
Tests check for regression of both bugs: http://crbug.com/440760 and
http://crbug.com/443491

name and email lists are tested. I disabled the test for the phone list because
after tab is pressed, phone list waits for a validation callback before
repopulating the list. It's not clear to me how to wait for that on the C++
testing side. I spent several hours trying to figure it out, and estade@
couldn't remember how to do it. If we want to fix it I think the best thing to
do is wait until dbeam@ is back and ask him for advice. The tests as they are
now are still very valid and useful. Neither regression involved the phone list
specifically; I had just wanted to add it to the tests for completeness.

[James Hawkins, 2015-01-14 17:32:52]

https://codereview.chromium.org/819193003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/autofill_options_list.js (right):

https://codereview.chromium.org/819193003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/autofill_options_list.js:211:
this.list.eventsDisabled = true;
This seems really fragile.  I'm really not confident about the eventsDisabled
solution.

[bondd, 2015-01-14 23:13:54]

https://codereview.chromium.org/819193003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/autofill_options_list.js (right):

https://codereview.chromium.org/819193003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/autofill_options_list.js:211:
this.list.eventsDisabled = true;
On 2015/01/14 17:32:52, James Hawkins wrote:
> This seems really fragile.  I'm really not confident about the eventsDisabled
> solution.

I like it because its effect is guaranteed to be limited to the
InlineEditableItemList subclasses that use it (in this case, the three lists on
the autofillEditAddress page). I'm not a fan of adding special flags like
|eventsDisabled|, but in this case I think it may be the best solution. I
investigated other solutions but they all involved making more far reaching
changes. WebUI is already very fragile, so I am trying to limit the scope and
reach of my changes to avoid unintended consequences elsewhere.

Maybe I am missing something super obvious though; do you have any ideas for a
solution that doesn't use |eventsDisabled|? I spent a long time thinking about
it before Christmas and couldn't come up with anything that didn't have the
potential to cause a lot more bugs.

[James Hawkins, 2015-01-14 23:17:11]

On 2015/01/14 23:13:54, bondd wrote:
>
https://codereview.chromium.org/819193003/diff/40001/chrome/browser/resources...
> File chrome/browser/resources/options/autofill_options_list.js (right):
> 
>
https://codereview.chromium.org/819193003/diff/40001/chrome/browser/resources...
> chrome/browser/resources/options/autofill_options_list.js:211:
> this.list.eventsDisabled = true;
> On 2015/01/14 17:32:52, James Hawkins wrote:
> > This seems really fragile.  I'm really not confident about the
eventsDisabled
> > solution.
> 
> I like it because its effect is guaranteed to be limited to the
> InlineEditableItemList subclasses that use it (in this case, the three lists
on
> the autofillEditAddress page). I'm not a fan of adding special flags like
> |eventsDisabled|, but in this case I think it may be the best solution. I
> investigated other solutions but they all involved making more far reaching
> changes. WebUI is already very fragile, so I am trying to limit the scope and
> reach of my changes to avoid unintended consequences elsewhere.
> 
> Maybe I am missing something super obvious though; do you have any ideas for a
> solution that doesn't use |eventsDisabled|? I spent a long time thinking about
> it before Christmas and couldn't come up with anything that didn't have the
> potential to cause a lot more bugs.

I'm not that deep into the code to be able to offer an alternative.  If we
really must move forward with this notion of disabling events temporarily, I
suggest creating a wrapper that takes a callback to a block of code which will
be executed within this disabled state.  The wrapper would be like:

disabled = true;
callback();
disabled = false;

The client code is thus simpler and less error-prone.

[bondd, 2015-01-15 03:04:02]

Added wrapper function for disabling events. Good idea, it's definitely better
and safer this way.

Changed CL description to be more descriptive.

https://codereview.chromium.org/819193003/diff/40001/chrome/browser/resources...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/40001/chrome/browser/resources...
chrome/browser/resources/options/inline_editable_list.js:627: 
This wasn't necessary.

[James Hawkins, 2015-01-15 16:10:44]

I'm really concerned that we're piling hacks on top of hacks.  I'd like to
revisit rolling back previous accessibility CLs instead.

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/inline_editable_list.js:88: if
(!this.parentNode.changeEventsDisabled_)
An object should not be accessing private data of another object.

[Dan Beam, 2015-01-21 18:18:07]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-01-21 18:18:08]

Changing the data or selection models should probably change the UI by default. 
Most classes will want this behavior.  Unfortunately, there's bound to be time
where this is unexpected and special code required to handle edge cases.

It seems like there's 2 solutions:

1) unsubscribe from notifications when they're not needed, or somehow otherwise
filter out extraneous ones (the flag you've added does accomplish this but I
think unsubscribing or annotating events with a "event.fromScripting = true"
while dispatching is a cleaner way to ignore).

2) change either of the two models to do less automagic updating/changing
(either by changing the model for all list or just specific to the cases we need
a dumber model).

https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options_list.js (right):

https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:225: }
} else {

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/inline_editable_list.js:88: if
(!this.parentNode.changeEventsDisabled_)
On 2015/01/15 16:10:44, James Hawkins wrote:
> An object should not be accessing private data of another object.

[closure's] @private is file-level

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/inline_editable_list.js:638: handleLeadChange:
function(e) {
where is this subscribed?  can we just turn off notifications with an event
registrar when we don't need them rather than adding a flag?

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:50: bool
GetContainsActiveElement(std::string element_selector) {
nit: ContainsActiveElement, double verb methods are hard to
ReadTellWhatTheyMean.

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:254:
TestEditAddressPhoneListTabKey) {
uncomment code but prefix with DISABLED_

https://codereview.chromium.org/819193003/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/819193003/diff/60001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:833:
'../chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc',
this file is already in chrome/, no reason to cd ../chrome/, has no effect

https://codereview.chromium.org/819193003/diff/60001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:904: 'browser/ui/toolbar/test_toolbar_model.h',
should be

  'browser/ui/webui

instead of

  '../chrome/browser/ui/webui

[bondd, 2015-01-21 22:54:01]

Patchset #5 (id:80001) has been deleted

[bondd, 2015-01-22 01:05:05]

On 2015/01/21 18:18:08, Dan Beam wrote:
> Changing the data or selection models should probably change the UI by
default. 
> Most classes will want this behavior.  Unfortunately, there's bound to be time
> where this is unexpected and special code required to handle edge cases.
> 
> It seems like there's 2 solutions:
> 
> 1) unsubscribe from notifications when they're not needed, or somehow
otherwise
> filter out extraneous ones (the flag you've added does accomplish this but I
> think unsubscribing or annotating events with a "event.fromScripting = true"
> while dispatching is a cleaner way to ignore).

Unfortunately annotating an event isn't possible because lead change events that
we want to ignore are generated by dataModel.updateIndex and dataModel.splice
called from autofill_options_list.js. The events are created in ArrayDataModel.

I investigated unsubscribing from events, but selectionChanged is triggered by
the 'selected' property (set up using cr.defineProperty). From what I can see
it's not easy/possible to temporarily disable a property.

I'd prefer to leave it the way it is with the flag. The flag is modified in a
single method and accessed in two methods, all in a single source file. I think
that is a simpler and easier to understand solution than doing disable/enable of
both an event and a property each time.

> 
> 2) change either of the two models to do less automagic updating/changing
> (either by changing the model for all list or just specific to the cases we
need
> a dumber model).
> 
>
https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
> File chrome/browser/resources/options/autofill_options_list.js (right):
> 
>
https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
> chrome/browser/resources/options/autofill_options_list.js:225: }
> } else {
> 
>
https://codereview.chromium.org/819193003/diff/60001/chrome/browser/resources...
> File chrome/browser/resources/options/inline_editable_list.js (right):
> 
>
https://codereview.chromium.org/819193003/diff/60001/chrome/browser/resources...
> chrome/browser/resources/options/inline_editable_list.js:88: if
> (!this.parentNode.changeEventsDisabled_)
> On 2015/01/15 16:10:44, James Hawkins wrote:
> > An object should not be accessing private data of another object.
> 
> [closure's] @private is file-level
> 
>
https://codereview.chromium.org/819193003/diff/60001/chrome/browser/resources...
> chrome/browser/resources/options/inline_editable_list.js:638:
handleLeadChange:
> function(e) {
> where is this subscribed?  can we just turn off notifications with an event
> registrar when we don't need them rather than adding a flag?
> 
>
https://codereview.chromium.org/819193003/diff/60001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
> (right):
> 
>
https://codereview.chromium.org/819193003/diff/60001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:50:
bool
> GetContainsActiveElement(std::string element_selector) {
> nit: ContainsActiveElement, double verb methods are hard to
> ReadTellWhatTheyMean.
> 
>
https://codereview.chromium.org/819193003/diff/60001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:254:
> TestEditAddressPhoneListTabKey) {
> uncomment code but prefix with DISABLED_
> 
> https://codereview.chromium.org/819193003/diff/60001/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
https://codereview.chromium.org/819193003/diff/60001/chrome/chrome_tests.gypi...
> chrome/chrome_tests.gypi:833:
> '../chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc',
> this file is already in chrome/, no reason to cd ../chrome/, has no effect
> 
>
https://codereview.chromium.org/819193003/diff/60001/chrome/chrome_tests.gypi...
> chrome/chrome_tests.gypi:904: 'browser/ui/toolbar/test_toolbar_model.h',
> should be
> 
>   'browser/ui/webui
> 
> instead of
> 
>   '../chrome/browser/ui/webui

[bondd, 2015-01-22 01:05:23]

https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/autofill_options_list.js (right):

https://codereview.chromium.org/819193003/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/autofill_options_list.js:225: }
On 2015/01/21 18:18:07, Dan Beam wrote:
> } else {

Done.

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:50: bool
GetContainsActiveElement(std::string element_selector) {
On 2015/01/21 18:18:07, Dan Beam wrote:
> nit: ContainsActiveElement, double verb methods are hard to
> ReadTellWhatTheyMean.

Done.

https://codereview.chromium.org/819193003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:254:
TestEditAddressPhoneListTabKey) {
On 2015/01/21 18:18:08, Dan Beam wrote:
> uncomment code but prefix with DISABLED_

Done.

https://codereview.chromium.org/819193003/diff/60001/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

https://codereview.chromium.org/819193003/diff/60001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:833:
'../chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc',
On 2015/01/21 18:18:08, Dan Beam wrote:
> this file is already in chrome/, no reason to cd ../chrome/, has no effect

Done.

https://codereview.chromium.org/819193003/diff/60001/chrome/chrome_tests.gypi...
chrome/chrome_tests.gypi:904: 'browser/ui/toolbar/test_toolbar_model.h',
On 2015/01/21 18:18:08, Dan Beam wrote:
> should be
> 
>   'browser/ui/webui
> 
> instead of
> 
>   '../chrome/browser/ui/webui

Done.

[Dan Beam, 2015-01-22 05:38:49]

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/autofill_options_list.js (right):

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/autofill_options_list.js:211:
this.list.executeWithChangeEventsDisabled(function() {
i guess this pattern is ok, but:

  this.list.setChangeEventsEnabled(false);
  this.list.dataModel.updateIndex(i);
  this.list.setChangeEventsEnabled(true);

is just as many lines and arguably more generic/easier to understand/less
complex or error prone (though it is duplicative)

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/inline_editable_list.js:686:
executeWithChangeEventsDisabled: function(callback) {
nit: ignoreChangeEvents

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/inline_editable_list.js:689:
this.changeEventsDisabled_ = false;
what if callback() calls executeWithChangeEventsDisabled_ @_@

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/inline_editable_list.js:698:
setSelectedIndexWithoutFocusing: function(index) {
nit: selectIndexWithoutFocusing

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:50: bool
ContainsActiveElement(std::string element_selector) {
all these args should be const std::string&

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:139: bool
PressKey(ui::KeyboardCode key_code, bool shift) {
nit: PressTab() until you actually use key_code for something else, IMO

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:154:
std::string input_text) {
nit:

  LineUpParamNames(const std::string& like_this,
                   const std::string& and_this) {

OR

  PutBothOnNextLine(
      const std::string& instead, const std::string& like_this) {

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:163:
EXPECT_EQ(GetListSize(list_selector), 2);
EXPECT_*() 2 parameters should be in the order of <expected>, <actual> (right
now they're reversed)

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:213:
input_text = "second" + input_text;
make a new var

https://codereview.chromium.org/819193003/diff/100001/chrome/common/url_const...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/819193003/diff/100001/chrome/common/url_const...
chrome/common/url_constants.h:312: extern const char
kAutofillEditAddressSubPage[];
eh, just put this in your test until somewhere in chrome uses it.  otherwise
chrome's binary gets bigger (maybe) for test-only code.

[bondd, 2015-01-22 20:23:58]

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/autofill_options_list.js (right):

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/autofill_options_list.js:211:
this.list.executeWithChangeEventsDisabled(function() {
On 2015/01/22 05:38:48, Dan Beam wrote:
> i guess this pattern is ok, but:
> 
>   this.list.setChangeEventsEnabled(false);
>   this.list.dataModel.updateIndex(i);
>   this.list.setChangeEventsEnabled(true);
> 
> is just as many lines and arguably more generic/easier to understand/less
> complex or error prone (though it is duplicative)

Acknowledged.
On the plus side it makes it 100% impossible to accidentally leave the events
disabled. For example (not saying this is good, but it's possible)
  this.setChangeEventsEnabled(false);
  if (something) {
    ...
  } else {
    ...
    if (somethingElse)
      return;
    ...
  }
  this.setChangeEventsEnabled(true);

If it's all the same to you I'll leave it as is.

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/inline_editable_list.js:686:
executeWithChangeEventsDisabled: function(callback) {
On 2015/01/22 05:38:48, Dan Beam wrote:
> nit: ignoreChangeEvents

Done.

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/inline_editable_list.js:689:
this.changeEventsDisabled_ = false;
On 2015/01/22 05:38:48, Dan Beam wrote:
> what if callback() calls executeWithChangeEventsDisabled_ @_@

Done.

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/resource...
chrome/browser/resources/options/inline_editable_list.js:698:
setSelectedIndexWithoutFocusing: function(index) {
On 2015/01/22 05:38:48, Dan Beam wrote:
> nit: selectIndexWithoutFocusing

Done.

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:50: bool
ContainsActiveElement(std::string element_selector) {
On 2015/01/22 05:38:48, Dan Beam wrote:
> all these args should be const std::string&

Done.

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:139: bool
PressKey(ui::KeyboardCode key_code, bool shift) {
On 2015/01/22 05:38:48, Dan Beam wrote:
> nit: PressTab() until you actually use key_code for something else, IMO

Done.

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:154:
std::string input_text) {
On 2015/01/22 05:38:48, Dan Beam wrote:
> nit:
> 
>   LineUpParamNames(const std::string& like_this,
>                    const std::string& and_this) {
> 
> OR
> 
>   PutBothOnNextLine(
>       const std::string& instead, const std::string& like_this) {

Done.

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:163:
EXPECT_EQ(GetListSize(list_selector), 2);
On 2015/01/22 05:38:48, Dan Beam wrote:
> EXPECT_*() 2 parameters should be in the order of <expected>, <actual> (right
> now they're reversed)

Done.

https://codereview.chromium.org/819193003/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:213:
input_text = "second" + input_text;
On 2015/01/22 05:38:48, Dan Beam wrote:
> make a new var

Done.

https://codereview.chromium.org/819193003/diff/100001/chrome/common/url_const...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/819193003/diff/100001/chrome/common/url_const...
chrome/common/url_constants.h:312: extern const char
kAutofillEditAddressSubPage[];
On 2015/01/22 05:38:49, Dan Beam wrote:
> eh, just put this in your test until somewhere in chrome uses it.  otherwise
> chrome's binary gets bigger (maybe) for test-only code.

Done.

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/inline_editable_list.js:88: if
(this.parentNode.ignoreChangeEventsCount_ == 0)
If you prefer, I could make an accessor
getIgnoreChangeEvents_() {
  return this.ignoreChangeEventsCount_ > 0;
}
Seems fine either way to me.

[Dan Beam, 2015-01-23 22:46:15]

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/inline_editable_list.js:687:
this.ignoreChangeEventsCount_++;
leaving as boolean and doing:

  assert(!this.ignoreChangeEvents_);

is easier than a counter, imo (if there's no nesting callers as of now)

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:19: //
This class tests the autofill options settings.
nit: capitalize Autofill when it's not used as a verb

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:21: //
because it is necessary to emulate pushing the tab key.
i think it's actually because focus is hard to share when multiple browser
windows are showing, but sure...

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:129: void
InsertText(const std::string& text) {
nit: ASSERT_EQ(std::string::npos, text.find("'")); (or just
ASSERT(std::string::npos == text.find("'"));)

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:153:
std::string input_text) {
make input_text const-ref

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:249: //
that here on the C++ testing side. Disabling phone list test for now.
when the validation result comes back does the focus change?

otherwise you can somehow call domAutomationController.send() when the async
result finishes.

[bondd, 2015-01-29 03:54:52]

Patchset #7 (id:140001) has been deleted

[bondd, 2015-01-29 19:01:01]

bondd@chromium.org changed reviewers:
+ jhawkins@google.com
- jhawkins@chromium.org

[bondd, 2015-01-29 19:01:01]

bondd@chromium.org changed required reviewers:
+ dbeam@chromium.org
- jhawkins@chromium.org

[bondd, 2015-01-29 19:01:02]

Enable phone list test and made it work with validation callback.
Phone list validation requires PersonalDataManager to be loaded to complete,
otherwise it early-outs here
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
and never calls AutofillEditAddressOverlay.setValidatedPhoneNumbers later in
that method.
I took the code to init a test PersonalDataManager and pulled it out into a new
common file autofill_uitest_util.cc.

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/inline_editable_list.js (right):

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/inline_editable_list.js:687:
this.ignoreChangeEventsCount_++;
On 2015/01/23 22:46:14, Dan Beam wrote:
> leaving as boolean and doing:
> 
>   assert(!this.ignoreChangeEvents_);
> 
> is easier than a counter, imo (if there's no nesting callers as of now)

Done.

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:19: //
This class tests the autofill options settings.
On 2015/01/23 22:46:14, Dan Beam wrote:
> nit: capitalize Autofill when it's not used as a verb

Done.

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:21: //
because it is necessary to emulate pushing the tab key.
On 2015/01/23 22:46:14, Dan Beam wrote:
> i think it's actually because focus is hard to share when multiple browser
> windows are showing, but sure...

Acknowledged.

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:129: void
InsertText(const std::string& text) {
On 2015/01/23 22:46:14, Dan Beam wrote:
> nit: ASSERT_EQ(std::string::npos, text.find("'")); (or just
> ASSERT(std::string::npos == text.find("'"));)

Done.

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:153:
std::string input_text) {
On 2015/01/23 22:46:14, Dan Beam wrote:
> make input_text const-ref

Done.

https://codereview.chromium.org/819193003/diff/120001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:249: //
that here on the C++ testing side. Disabling phone list test for now.
On 2015/01/23 22:46:14, Dan Beam wrote:
> when the validation result comes back does the focus change?
> 
> otherwise you can somehow call domAutomationController.send() when the async
> result finishes.

Done.

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:204: //
It becomes selected later when the list is given focus.
I'm leaving this as a TODO because IMO it's well past the point of diminishing
returns for this test.

[Dan Beam, 2015-01-29 19:31:45]

lgtm

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_uitest_util.cc (right):

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.cc:61: PersonalDataManager*
GetPersonalDataManager(Browser* browser) {
nit: ask for a Profile* instead of a Browser*

[Evan Stade, 2015-01-29 22:20:10]

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

[Evan Stade, 2015-01-29 22:20:11]

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_uitest_util.cc (right):

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.cc:67: test::SetProfileInfo(
why not GetFullProfile()?

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_uitest_util.h (right):

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.h:19: class
WindowedPersonalDataManagerObserver
I don't understand what "Windowed" means in this context.

This class needs a class-level comment.

Does this class need to be in the header at all or can it just be in the cc
file?

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.h:39: InfoBarService*
infobar_service_;
forward declare this instead of including

[bondd, 2015-01-30 03:57:48]

Patchset #9 (id:200001) has been deleted

[bondd, 2015-01-30 04:10:35]

estade@: I moved WindowedPersonalDataManagerObserver entirely to the cc file as
you suggested. It required moving AddProfile(), SetProfile(), and SetProfiles()
there as well. I think it's better this way. Please let me know what you think.

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_uitest_util.cc (right):

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.cc:61: PersonalDataManager*
GetPersonalDataManager(Browser* browser) {
On 2015/01/29 19:31:44, Dan Beam wrote:
> nit: ask for a Profile* instead of a Browser*

Done.

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.cc:67: test::SetProfileInfo(
On 2015/01/29 22:20:11, Evan Stade wrote:
> why not GetFullProfile()?

Some tests in autofill_interactive_uitest.cc expect the "Milton Waddams" data,
for example
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/aut...

I moved CreateTestProfile back into the two separate files so that
autofill_interactive_uitest.cc can have its "Milton Waddams" data, and
autofill_options_interactive_uitest.cc can use GetFullProfile. Both methods are
shorter now that they are using common helper method AddTestProfile().

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_uitest_util.h (right):

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.h:19: class
WindowedPersonalDataManagerObserver
On 2015/01/29 22:20:11, Evan Stade wrote:
> I don't understand what "Windowed" means in this context.
> 
> This class needs a class-level comment.
> 
> Does this class need to be in the header at all or can it just be in the cc
> file?

Done. Moved it to cc file and added class-level comment.

I'm not 100% clear on what "Windowed" means either. I moved this code here
(well, now to the cc file) from autofill_interactive_uitest.cc so it could be
shared with autofill_options_interactive_uitest.cc. I added a comment describing
what the class does. If you have a better name for the class please let me know,
I'm happy to change it.

https://codereview.chromium.org/819193003/diff/180001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.h:39: InfoBarService*
infobar_service_;
On 2015/01/29 22:20:11, Evan Stade wrote:
> forward declare this instead of including

Acknowledged. No longer necessary after moving class to cc file.

[Evan Stade, 2015-01-30 19:16:33]

https://codereview.chromium.org/819193003/diff/220001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_uitest_util.cc (right):

https://codereview.chromium.org/819193003/diff/220001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.cc:19: // This class is used to
wait for asynchronous updates to PersonalDataManager
still don't know what Windowed means here

https://codereview.chromium.org/819193003/diff/220001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.cc:84:
WindowedPersonalDataManagerObserver observer(browser);
too much indent

[bondd, 2015-01-30 20:24:17]

https://codereview.chromium.org/819193003/diff/220001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_uitest_util.cc (right):

https://codereview.chromium.org/819193003/diff/220001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.cc:19: // This class is used to
wait for asynchronous updates to PersonalDataManager
On 2015/01/30 19:16:33, Evan Stade wrote:
> still don't know what Windowed means here

I think we should just rename it. Requesting suggestions.
InfoBarPersonalDataManagerObserver? MyPersonalDataManagerObserver? WaitObserver?
AsyncWaitObserver?

I'm not sure what "Windowed" means here either. There are a bunch of copies of
this code floating around in different tests. I don't know which is the original
or who named it first, I 'git blame'd it back to at least 2012.

[Evan Stade, 2015-01-30 20:32:47]

On 2015/01/30 20:24:17, bondd wrote:
>
https://codereview.chromium.org/819193003/diff/220001/chrome/browser/autofill...
> File chrome/browser/autofill/autofill_uitest_util.cc (right):
> 
>
https://codereview.chromium.org/819193003/diff/220001/chrome/browser/autofill...
> chrome/browser/autofill/autofill_uitest_util.cc:19: // This class is used to
> wait for asynchronous updates to PersonalDataManager
> On 2015/01/30 19:16:33, Evan Stade wrote:
> > still don't know what Windowed means here
> 
> I think we should just rename it. Requesting suggestions.
> InfoBarPersonalDataManagerObserver? MyPersonalDataManagerObserver?
WaitObserver?
> AsyncWaitObserver?
> 
> I'm not sure what "Windowed" means here either. There are a bunch of copies of
> this code floating around in different tests. I don't know which is the
original
> or who named it first, I 'git blame'd it back to at least 2012.

PdmChangeWaiter

[bondd, 2015-01-30 22:10:00]

Renamed WindowedPersonalDataManagerObserver to PdmChangeWaiter.
Noticed WindowedPersonalDataManagerObserver is duplicated in two places in other
test code. Added TODOs to use the common version.

https://codereview.chromium.org/819193003/diff/220001/chrome/browser/autofill...
File chrome/browser/autofill/autofill_uitest_util.cc (right):

https://codereview.chromium.org/819193003/diff/220001/chrome/browser/autofill...
chrome/browser/autofill/autofill_uitest_util.cc:84:
WindowedPersonalDataManagerObserver observer(browser);
On 2015/01/30 19:16:33, Evan Stade wrote:
> too much indent

Done.

[Evan Stade, 2015-01-30 22:26:41]

lgtm

[Dan Beam, 2015-02-04 19:50:46]

if you'd like to commit now and let me fix the button strip issue, that's fine
as well

https://codereview.chromium.org/819193003/diff/400001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/400001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:354: #if
defined(TOOLKIT_VIEWS)
a) just #if !defined() ... code ... #endif
b) why can't we fix this just to work differently on platforms that reverse
button strips?

[Dan Beam, 2015-02-04 20:47:31]

lgtm

https://codereview.chromium.org/819193003/diff/420001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:296:
LOG(INFO) << "Starting TestEditAddressListTabKeyEditItem";
remove debugging statements

https://codereview.chromium.org/819193003/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:375: #if
defined(OS_WIN) || defined(OS_CHROMEOS)
add a note that this because the button strip is reversed on these platforms

[bondd, 2015-02-04 22:13:05]

New patchsets have been uploaded after l-g-t-m from dbeam@chromium.org

[bondd, 2015-02-04 22:14:17]

https://codereview.chromium.org/819193003/diff/420001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc
(right):

https://codereview.chromium.org/819193003/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:296:
LOG(INFO) << "Starting TestEditAddressListTabKeyEditItem";
On 2015/02/04 20:47:29, Dan Beam wrote:
> remove debugging statements

Done.

https://codereview.chromium.org/819193003/diff/420001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/autofill_options_interactive_uitest.cc:375: #if
defined(OS_WIN) || defined(OS_CHROMEOS)
On 2015/02/04 20:47:29, Dan Beam wrote:
> add a note that this because the button strip is reversed on these platforms

Done.

[bondd, 2015-02-04 22:14:35]

The CQ bit was checked by bondd@chromium.org

[commit-bot: I haz the power, 2015-02-04 22:14:56]

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

[commit-bot: I haz the power, 2015-02-04 23:30:25]

Committed patchset #20 (id:440001)

[commit-bot: I haz the power, 2015-02-04 23:31:56]

Patchset 20 (id:??) landed as
https://crrev.com/10a45b9048e77944987d387b0eff3b5fcfd7a0bc
Cr-Commit-Position: refs/heads/master@{#314675}