Ignore ajax on specified pages.

Ignore ajax on specified pages.

BUG=247522
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207923

[Dane Wallinga, 2013-06-07 01:18:48]

PTAL

Still trying to figure out where else I need testing, but otherwise everything
appears to work as intended.

[Raman Kakilate, 2013-06-07 01:34:25]

approach lgtm.

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:414: if (is_post_document_load)
{
This code should have test.

[ahutter, 2013-06-07 17:37:47]

lgtm after test Raman requested are added

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
File components/autofill/browser/autocheckout_manager.h (right):

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.h:64: bool
ShouldIgnoreDynamicFormChanges();
Docs.

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
File components/autofill/browser/autocheckout_page_meta_data.h (right):

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
components/autofill/browser/autocheckout_page_meta_data.h:41: bool ignore_ajax;
docs

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
File components/autofill/browser/autofill_xml_parser.cc (right):

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
components/autofill/browser/autofill_xml_parser.cc:123:
page_meta_data_->ignore_ajax = (strcmp(*attrs, "true") == 0);
these might be extraneous parens according to the style guide

[Dane Wallinga, 2013-06-07 21:06:46]

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
File components/autofill/browser/autocheckout_manager.h (right):

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
components/autofill/browser/autocheckout_manager.h:64: bool
ShouldIgnoreDynamicFormChanges();
On 2013/06/07 17:37:47, ahutter wrote:
> Docs.

Done.

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
File components/autofill/browser/autocheckout_page_meta_data.h (right):

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
components/autofill/browser/autocheckout_page_meta_data.h:41: bool ignore_ajax;
On 2013/06/07 17:37:47, ahutter wrote:
> docs

Done.

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
components/autofill/browser/autofill_manager.cc:414: if (is_post_document_load)
{
On 2013/06/07 01:34:25, Raman Kakilate wrote:
> This code should have test.

Done.

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
File components/autofill/browser/autofill_xml_parser.cc (right):

https://codereview.chromium.org/16611003/diff/5001/components/autofill/browse...
components/autofill/browser/autofill_xml_parser.cc:123:
page_meta_data_->ignore_ajax = (strcmp(*attrs, "true") == 0);
On 2013/06/07 17:37:47, ahutter wrote:
> these might be extraneous parens according to the style guide

Done.

[Dane Wallinga, 2013-06-11 00:20:18]

@isherman - PTAL

[Ilya Sherman, 2013-06-11 01:11:20]

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
File components/autofill/browser/autocheckout_manager.cc (right):

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autocheckout_manager.cc:259: return
in_autocheckout_flow_ && page_meta_data_->ignore_ajax;
nit: Would be nice to use "DynamicFormChanges" or "ajax" consistently, rather
than using each in different places.

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
File components/autofill/browser/autocheckout_manager.h (right):

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autocheckout_manager.h:90: in_autocheckout_flow_ =
in_autocheckout_flow;
nit: Reduce indentation by two spaces.

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autocheckout_manager.h:166:
FRIEND_TEST_ALL_PREFIXES(AutofillManagerTest, DynamicFormsSeenAndIgnored);
Why are these needed?  Almost always, you should prefer to expose a subset of
the private portion of the class through protected visibility methods +
customized test classes.

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:413: // If new forms were added
via AJAX or DHML, treat as new page.
nit: Please update this comment.

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
File components/autofill/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:1024: form_structures =
autofill_manager_->GetFormStructures();
Why is this line duplicated?

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
File components/autofill/browser/autofill_xml_parser.cc (right):

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autofill_xml_parser.cc:123:
page_meta_data_->ignore_ajax = strcmp(*attrs, "true") == 0;
Hmm, using strcmp here makes me a little nervous.  Please double-check with the
security team whether this is ok.

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
File components/autofill/browser/autofill_xml_parser_unittest.cc (right):

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autofill_xml_parser_unittest.cc:282: // Clear
|field_infos_| for the next test;
nit: semicolon -> period

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autofill_xml_parser_unittest.cc:299:
EXPECT_TRUE(page_meta_data_.ignore_ajax);
nit: Please include a negative test case as well.

[Dane Wallinga, 2013-06-11 19:03:32]

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
File components/autofill/browser/autocheckout_manager.cc (right):

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autocheckout_manager.cc:259: return
in_autocheckout_flow_ && page_meta_data_->ignore_ajax;
On 2013/06/11 01:11:20, Ilya Sherman wrote:
> nit: Would be nice to use "DynamicFormChanges" or "ajax" consistently, rather
> than using each in different places.

Done.

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
File components/autofill/browser/autocheckout_manager.h (right):

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autocheckout_manager.h:90: in_autocheckout_flow_ =
in_autocheckout_flow;
On 2013/06/11 01:11:20, Ilya Sherman wrote:
> nit: Reduce indentation by two spaces.

Done.

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autocheckout_manager.h:166:
FRIEND_TEST_ALL_PREFIXES(AutofillManagerTest, DynamicFormsSeenAndIgnored);
On 2013/06/11 01:11:20, Ilya Sherman wrote:
> Why are these needed?  Almost always, you should prefer to expose a subset of
> the private portion of the class through protected visibility methods +
> customized test classes.

*shrug* - this is the pattern we appear to be using with autofill_manager.

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
File components/autofill/browser/autofill_manager.cc (right):

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autofill_manager.cc:413: // If new forms were added
via AJAX or DHML, treat as new page.
On 2013/06/11 01:11:20, Ilya Sherman wrote:
> nit: Please update this comment.

Done.

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
File components/autofill/browser/autofill_manager_unittest.cc (right):

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autofill_manager_unittest.cc:1024: form_structures =
autofill_manager_->GetFormStructures();
On 2013/06/11 01:11:20, Ilya Sherman wrote:
> Why is this line duplicated?

Just wanted to be absolutely sure it got executed.

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
File components/autofill/browser/autofill_xml_parser.cc (right):

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autofill_xml_parser.cc:123:
page_meta_data_->ignore_ajax = strcmp(*attrs, "true") == 0;
On 2013/06/11 01:11:20, Ilya Sherman wrote:
> Hmm, using strcmp here makes me a little nervous.  Please double-check with
the
> security team whether this is ok.

Even though this file is already replete with strcmps?

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
File components/autofill/browser/autofill_xml_parser_unittest.cc (right):

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autofill_xml_parser_unittest.cc:282: // Clear
|field_infos_| for the next test;
On 2013/06/11 01:11:20, Ilya Sherman wrote:
> nit: semicolon -> period

Done.

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autofill_xml_parser_unittest.cc:299:
EXPECT_TRUE(page_meta_data_.ignore_ajax);
On 2013/06/11 01:11:20, Ilya Sherman wrote:
> nit: Please include a negative test case as well.

Line 182

[Ilya Sherman, 2013-06-11 23:14:32]

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
File components/autofill/browser/autocheckout_manager.h (right):

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autocheckout_manager.h:166:
FRIEND_TEST_ALL_PREFIXES(AutofillManagerTest, DynamicFormsSeenAndIgnored);
On 2013/06/11 19:03:32, Dane Wallinga wrote:
> On 2013/06/11 01:11:20, Ilya Sherman wrote:
> > Why are these needed?  Almost always, you should prefer to expose a subset
of
> > the private portion of the class through protected visibility methods +
> > customized test classes.
> 
> *shrug* - this is the pattern we appear to be using with autofill_manager.

Alright, let me be clearer: Please do not friend tests other in exceptional
circumstances.  The tradeoff might seem minor in the short-term, but makes a big
difference for long-term maintainability.

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
File components/autofill/browser/autofill_xml_parser.cc (right):

https://chromiumcodereview.appspot.com/16611003/diff/14001/components/autofil...
components/autofill/browser/autofill_xml_parser.cc:123:
page_meta_data_->ignore_ajax = strcmp(*attrs, "true") == 0;
On 2013/06/11 19:03:32, Dane Wallinga wrote:
> On 2013/06/11 01:11:20, Ilya Sherman wrote:
> > Hmm, using strcmp here makes me a little nervous.  Please double-check with
> the
> > security team whether this is ok.
> 
> Even though this file is already replete with strcmps?

By replete with, I guess you mean "has two strcmps"?  I'd still prefer to have
the security team double-check this, but I'm less worried about it now that I
see the two pre-existing uses.

[Dane Wallinga, 2013-06-12 20:33:37]

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
File components/autofill/browser/autocheckout_manager.h (right):

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autocheckout_manager.h:166:
FRIEND_TEST_ALL_PREFIXES(AutofillManagerTest, DynamicFormsSeenAndIgnored);
On 2013/06/11 23:14:32, Ilya Sherman wrote:
> On 2013/06/11 19:03:32, Dane Wallinga wrote:
> > On 2013/06/11 01:11:20, Ilya Sherman wrote:
> > > Why are these needed?  Almost always, you should prefer to expose a subset
> of
> > > the private portion of the class through protected visibility methods +
> > > customized test classes.
> > 
> > *shrug* - this is the pattern we appear to be using with autofill_manager.
> 
> Alright, let me be clearer: Please do not friend tests other in exceptional
> circumstances.  The tradeoff might seem minor in the short-term, but makes a
big
> difference for long-term maintainability.

Ok, so AutofillManager has a member AutocheckoutManager autocheckout_manager_.
What I would need then is for TestAutofillManager (defined in
autofill_manager_unittest.cc and extending AutofillManager) to replace this
autocheckout_manager_ with a TestAutocheckoutManager that I would define. What
is the cleanest way I can do that? Keeping in mind that most of what I know
about C++ I've picked up in the last quarter, the simplest thing I can think of
is to turn autocheckout_manager_ into a pointer, and then reassign it in
TestAutofillManager's constructor, but of course that's going to hurt the
performance of AutofillManager just for the sake of a test. Any suggestions?

[Ilya Sherman, 2013-06-15 01:18:19]

With apologies for the delay:

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
File components/autofill/browser/autocheckout_manager.h (right):

https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
components/autofill/browser/autocheckout_manager.h:166:
FRIEND_TEST_ALL_PREFIXES(AutofillManagerTest, DynamicFormsSeenAndIgnored);
On 2013/06/12 20:33:37, Dane Wallinga wrote:
> On 2013/06/11 23:14:32, Ilya Sherman wrote:
> > On 2013/06/11 19:03:32, Dane Wallinga wrote:
> > > On 2013/06/11 01:11:20, Ilya Sherman wrote:
> > > > Why are these needed?  Almost always, you should prefer to expose a
subset
> > of
> > > > the private portion of the class through protected visibility methods +
> > > > customized test classes.
> > > 
> > > *shrug* - this is the pattern we appear to be using with autofill_manager.
> > 
> > Alright, let me be clearer: Please do not friend tests other in exceptional
> > circumstances.  The tradeoff might seem minor in the short-term, but makes a
> big
> > difference for long-term maintainability.
> 
> Ok, so AutofillManager has a member AutocheckoutManager autocheckout_manager_.
> What I would need then is for TestAutofillManager (defined in
> autofill_manager_unittest.cc and extending AutofillManager) to replace this
> autocheckout_manager_ with a TestAutocheckoutManager that I would define. What
> is the cleanest way I can do that? Keeping in mind that most of what I know
> about C++ I've picked up in the last quarter, the simplest thing I can think
of
> is to turn autocheckout_manager_ into a pointer, and then reassign it in
> TestAutofillManager's constructor, but of course that's going to hurt the
> performance of AutofillManager just for the sake of a test. Any suggestions?

Looking more closely at the test code, IMO the best thing would be to not expose
a method set_in_autocheckout_flow() at all, and instead call through the public
API in the normal fashion to trigger an update to this variable.  The reason is
that you're currently not really "mocking out" the boolean's functionality;
you're just editing private state by setting the boolean directly.  The value of
set_foo() methods exposed for testing is in allowing you to mock out
dependencies, rather than to muck with private state.

If for some reason you think that's not appropriate, then yes, the way to
implement this would be to make AutocheckoutManager be a pointer owned by the
AutofillManager class, which test code could override.  The Autofill code is not
so performance-sensitive to where the extra pointer dereference should have a
noticeable performance impact.

[Dane Wallinga, 2013-06-17 20:12:47]

On 2013/06/15 01:18:19, Ilya Sherman wrote:
> With apologies for the delay:
> 
>
https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
> File components/autofill/browser/autocheckout_manager.h (right):
> 
>
https://codereview.chromium.org/16611003/diff/14001/components/autofill/brows...
> components/autofill/browser/autocheckout_manager.h:166:
> FRIEND_TEST_ALL_PREFIXES(AutofillManagerTest, DynamicFormsSeenAndIgnored);
> On 2013/06/12 20:33:37, Dane Wallinga wrote:
> > On 2013/06/11 23:14:32, Ilya Sherman wrote:
> > > On 2013/06/11 19:03:32, Dane Wallinga wrote:
> > > > On 2013/06/11 01:11:20, Ilya Sherman wrote:
> > > > > Why are these needed?  Almost always, you should prefer to expose a
> subset
> > > of
> > > > > the private portion of the class through protected visibility methods
+
> > > > > customized test classes.
> > > > 
> > > > *shrug* - this is the pattern we appear to be using with
autofill_manager.
> > > 
> > > Alright, let me be clearer: Please do not friend tests other in
exceptional
> > > circumstances.  The tradeoff might seem minor in the short-term, but makes
a
> > big
> > > difference for long-term maintainability.
> > 
> > Ok, so AutofillManager has a member AutocheckoutManager
autocheckout_manager_.
> > What I would need then is for TestAutofillManager (defined in
> > autofill_manager_unittest.cc and extending AutofillManager) to replace this
> > autocheckout_manager_ with a TestAutocheckoutManager that I would define.
What
> > is the cleanest way I can do that? Keeping in mind that most of what I know
> > about C++ I've picked up in the last quarter, the simplest thing I can think
> of
> > is to turn autocheckout_manager_ into a pointer, and then reassign it in
> > TestAutofillManager's constructor, but of course that's going to hurt the
> > performance of AutofillManager just for the sake of a test. Any suggestions?
> 
> Looking more closely at the test code, IMO the best thing would be to not
expose
> a method set_in_autocheckout_flow() at all, and instead call through the
public
> API in the normal fashion to trigger an update to this variable.  The reason
is
> that you're currently not really "mocking out" the boolean's functionality;
> you're just editing private state by setting the boolean directly.  The value
of
> set_foo() methods exposed for testing is in allowing you to mock out
> dependencies, rather than to muck with private state.
> 
> If for some reason you think that's not appropriate, then yes, the way to
> implement this would be to make AutocheckoutManager be a pointer owned by the
> AutofillManager class, which test code could override.  The Autofill code is
not
> so performance-sensitive to where the extra pointer dereference should have a
> noticeable performance impact.

Trying to go through the real sequence of calls, or even an approximation of
them, requires a bunch of UI interaction that I wouldn't know how to easily
bypass in a unit test.

[Ilya Sherman, 2013-06-18 05:30:03]

> Trying to go through the real sequence of calls, or even an approximation of
> them, requires a bunch of UI interaction that I wouldn't know how to easily
> bypass in a unit test.

Looks like the test already has a MockAutofillManagerDelegate designed exactly
for this purpose.  It looks like all you need to do is to call
MaybeShowAutocheckoutBubble(), and the existing test scaffolding should take
care of the rest.

[Dane Wallinga, 2013-06-18 20:16:53]

On 2013/06/18 05:30:03, Ilya Sherman wrote:
> > Trying to go through the real sequence of calls, or even an approximation of
> > them, requires a bunch of UI interaction that I wouldn't know how to easily
> > bypass in a unit test.
> 
> Looks like the test already has a MockAutofillManagerDelegate designed exactly
> for this purpose.  It looks like all you need to do is to call
> MaybeShowAutocheckoutBubble(), and the existing test scaffolding should take
> care of the rest.

That's not actually going to set off the flow. What needs to be called is
ReturnAutocheckoutData, which would normally by passed as a callback to
MaybeShowAutocheckoutDialog, but won't get triggered with the mock delegate.
What I can do is expose ReturnAutocheckoutData and call that directly. Not
exactly the real flow, but closer than setting the boolean directly.

[Ilya Sherman, 2013-06-19 00:18:32]

On 2013/06/18 20:16:53, Dane Wallinga wrote:
> On 2013/06/18 05:30:03, Ilya Sherman wrote:
> > > Trying to go through the real sequence of calls, or even an approximation
of
> > > them, requires a bunch of UI interaction that I wouldn't know how to
easily
> > > bypass in a unit test.
> > 
> > Looks like the test already has a MockAutofillManagerDelegate designed
exactly
> > for this purpose.  It looks like all you need to do is to call
> > MaybeShowAutocheckoutBubble(), and the existing test scaffolding should take
> > care of the rest.
> 
> That's not actually going to set off the flow. What needs to be called is
> ReturnAutocheckoutData, which would normally by passed as a callback to
> MaybeShowAutocheckoutDialog, but won't get triggered with the mock delegate.
> What I can do is expose ReturnAutocheckoutData and call that directly. Not
> exactly the real flow, but closer than setting the boolean directly.

Sorry, I was referring to the mock in the AutocheckoutManager unit test, rather
than the AutofillManager unit test:
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...

AFAICT that should handle all of the callbacks correctly, and is the "right" way
to test this code.

[Dane Wallinga, 2013-06-19 19:33:16]

On 2013/06/19 00:18:32, Ilya Sherman wrote:
> On 2013/06/18 20:16:53, Dane Wallinga wrote:
> > On 2013/06/18 05:30:03, Ilya Sherman wrote:
> > > > Trying to go through the real sequence of calls, or even an
approximation
> of
> > > > them, requires a bunch of UI interaction that I wouldn't know how to
> easily
> > > > bypass in a unit test.
> > > 
> > > Looks like the test already has a MockAutofillManagerDelegate designed
> exactly
> > > for this purpose.  It looks like all you need to do is to call
> > > MaybeShowAutocheckoutBubble(), and the existing test scaffolding should
take
> > > care of the rest.
> > 
> > That's not actually going to set off the flow. What needs to be called is
> > ReturnAutocheckoutData, which would normally by passed as a callback to
> > MaybeShowAutocheckoutDialog, but won't get triggered with the mock delegate.
> > What I can do is expose ReturnAutocheckoutData and call that directly. Not
> > exactly the real flow, but closer than setting the boolean directly.
> 
> Sorry, I was referring to the mock in the AutocheckoutManager unit test,
rather
> than the AutofillManager unit test:
>
https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil...
> 
> AFAICT that should handle all of the callbacks correctly, and is the "right"
way
> to test this code.

I feel as though this is starting to test a bit more than I would generally
intend for a unit test, but it should be working.

[Ilya Sherman, 2013-06-19 23:25:13]

Thanks, looking pretty good now :)

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:393: else
nit: No else stmt after a return, plz.

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
File components/autofill/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:3285:
ASSERT_TRUE(delegate.autocheckout_bubble_shown());
nit: EXPECT_TRUE

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:3315:
ASSERT_FALSE(delegate.autocheckout_bubble_shown());
nit: EXPECT_FALSE

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:3353:
TestFormStructure* form_structure = new TestFormStructure(address);
nit: Wrap this in a scoped_ptr.

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:3363: // 
FormsSeen(forms);
Why doesn't this line get to join the party?

https://codereview.chromium.org/16611003/diff/47001/components/autofill/conte...
File components/autofill/content/browser/autocheckout_manager.h (right):

https://codereview.chromium.org/16611003/diff/47001/components/autofill/conte...
components/autofill/content/browser/autocheckout_manager.h:11: #include
"base/gtest_prod_util.h"
nit: No longer needed.

[Dane Wallinga, 2013-06-20 19:46:40]

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
File components/autofill/browser/autofill_manager.cc (right):

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager.cc:393: else
On 2013/06/19 23:25:13, Ilya Sherman wrote:
> nit: No else stmt after a return, plz.

Done.

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
File components/autofill/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:3285:
ASSERT_TRUE(delegate.autocheckout_bubble_shown());
On 2013/06/19 23:25:13, Ilya Sherman wrote:
> nit: EXPECT_TRUE

Done.

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:3315:
ASSERT_FALSE(delegate.autocheckout_bubble_shown());
On 2013/06/19 23:25:13, Ilya Sherman wrote:
> nit: EXPECT_FALSE

Done.

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:3353:
TestFormStructure* form_structure = new TestFormStructure(address);
On 2013/06/19 23:25:13, Ilya Sherman wrote:
> nit: Wrap this in a scoped_ptr.

You sure? A raw pointer is used elsewhere in this file for form_structure, and
I'm getting some sort of error during the tear down if I try wrapping it.

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:3363: // 
FormsSeen(forms);
On 2013/06/19 23:25:13, Ilya Sherman wrote:
> Why doesn't this line get to join the party?

heh, oops.

https://codereview.chromium.org/16611003/diff/47001/components/autofill/conte...
File components/autofill/content/browser/autocheckout_manager.h (right):

https://codereview.chromium.org/16611003/diff/47001/components/autofill/conte...
components/autofill/content/browser/autocheckout_manager.h:11: #include
"base/gtest_prod_util.h"
On 2013/06/19 23:25:13, Ilya Sherman wrote:
> nit: No longer needed.

Done.

[Ilya Sherman, 2013-06-21 05:10:14]

LGTM, thanks.

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
File components/autofill/browser/autofill_manager_unittest.cc (right):

https://codereview.chromium.org/16611003/diff/47001/components/autofill/brows...
components/autofill/browser/autofill_manager_unittest.cc:3353:
TestFormStructure* form_structure = new TestFormStructure(address);
On 2013/06/20 19:46:41, Dane Wallinga wrote:
> On 2013/06/19 23:25:13, Ilya Sherman wrote:
> > nit: Wrap this in a scoped_ptr.
> 
> You sure? A raw pointer is used elsewhere in this file for form_structure, and
> I'm getting some sort of error during the tear down if I try wrapping it.

The preferred style is to wrap all new calls in scoped_ptrs, and then call
.Pass() or .release() to transfer ownership.

[commit-bot: I haz the power, 2013-06-21 19:17:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dgwallinga@chromium.org/16611003/63001

[commit-bot: I haz the power, 2013-06-21 21:19:20]

Change committed as 207923