In components/autofill, move notification handling into content driver.

In components/autofill, move notification handling into content driver.

This CL moves code listening for notifications from AutofillExternalDelegate
(core code) to AutofillDriverImpl (content driver code).

This CL also changes the browsertest that was testing this functionality 
from being on AutofillExternalDelegate to being on AutofillDriverImpl.

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

[blundell, 2013-06-26 20:42:30]

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_driver_impl_browsertest.cc (left):

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:100:
AutofillManager::RegisterUserPrefs(manager_delegate_.GetPrefRegistry());
Deleting this call doesn't seem to have any effect. Is there anything I'm
mising?

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:131:
autofill::GenerateTestAutofillPopup(autofill_external_delegate_.get());
Code inspection made me think that these calls were unnecessary here, and
indeed, removing them doesn't affect the tests. Is there anything I'm missing?

[Ilya Sherman, 2013-06-26 22:36:18]

LGTM

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_driver_impl_browsertest.cc (left):

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:100:
AutofillManager::RegisterUserPrefs(manager_delegate_.GetPrefRegistry());
On 2013/06/26 20:42:31, blundell wrote:
> Deleting this call doesn't seem to have any effect. Is there anything I'm
> mising?

Not sure.  This might be meant to suppress some DCHECK or debug logging... but I
don't remember at this point.

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:131:
autofill::GenerateTestAutofillPopup(autofill_external_delegate_.get());
On 2013/06/26 20:42:31, blundell wrote:
> Code inspection made me think that these calls were unnecessary here, and
> indeed, removing them doesn't affect the tests. Is there anything I'm missing?

These were probably needed when the test relied less on mocks, or the tested
code had a different memory model, or something like that.  I agree that it
looks like they are no longer needed.

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_driver_impl_browsertest.cc (right):

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:59: // Subclass
AutofillDriverImpl so we can create AutofillDriverImpl instance.
nit: "AutofillDriverImpl instance" -> "an AutofillDriverImpl instance"

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:67:
AutofillManager::ENABLE_AUTOFILL_DOWNLOAD_MANAGER) {}
nit: Reduce the indentation of this line by two spaces.

https://codereview.chromium.org/17893010/diff/1/components/autofill/content/b...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/17893010/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.h:80: //
content::NotificationObserver method override.
nit: Please replace "method override." with either "implementation." or just ":"
(i.e. a lone colon).

[blundell, 2013-06-27 14:50:35]

Thanks!

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_driver_impl_browsertest.cc (left):

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:100:
AutofillManager::RegisterUserPrefs(manager_delegate_.GetPrefRegistry());
On 2013/06/26 22:36:18, Ilya Sherman wrote:
> On 2013/06/26 20:42:31, blundell wrote:
> > Deleting this call doesn't seem to have any effect. Is there anything I'm
> > mising?
> 
> Not sure.  This might be meant to suppress some DCHECK or debug logging... but
I
> don't remember at this point.

Restored the line, as neither of us has a definitive argument that it's not
needed for some subtle reason.

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
File chrome/browser/autofill/autofill_driver_impl_browsertest.cc (right):

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:59: // Subclass
AutofillDriverImpl so we can create AutofillDriverImpl instance.
On 2013/06/26 22:36:18, Ilya Sherman wrote:
> nit: "AutofillDriverImpl instance" -> "an AutofillDriverImpl instance"

Done.

https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autof...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:67:
AutofillManager::ENABLE_AUTOFILL_DOWNLOAD_MANAGER) {}
4-space indenting is correct for a wrapped argument
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...).
I moved all the other arguments to that same indentation per what the style
guide seems to indicate for this case.

On 2013/06/26 22:36:18, Ilya Sherman wrote:
> nit: Reduce the indentation of this line by two spaces.

https://codereview.chromium.org/17893010/diff/1/components/autofill/content/b...
File components/autofill/content/browser/autofill_driver_impl.h (right):

https://codereview.chromium.org/17893010/diff/1/components/autofill/content/b...
components/autofill/content/browser/autofill_driver_impl.h:80: //
content::NotificationObserver method override.
On 2013/06/26 22:36:18, Ilya Sherman wrote:
> nit: Please replace "method override." with either "implementation." or just
":"
> (i.e. a lone colon).

Done.

[commit-bot: I haz the power, 2013-06-27 14:50:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/17893010/8002

[commit-bot: I haz the power, 2013-06-27 18:41:56]

Change committed as 208957

[Ilya Sherman, 2013-06-27 22:16:09]

https://chromiumcodereview.appspot.com/17893010/diff/1/chrome/browser/autofil...
File chrome/browser/autofill/autofill_driver_impl_browsertest.cc (right):

https://chromiumcodereview.appspot.com/17893010/diff/1/chrome/browser/autofil...
chrome/browser/autofill/autofill_driver_impl_browsertest.cc:67:
AutofillManager::ENABLE_AUTOFILL_DOWNLOAD_MANAGER) {}
Ah, I'd missed that this was also an argument to the AutofillDriverImpl
constructor.  In that case, I think you actually want to *increase* the
indentation by two spaces, as it should be four spaces from the 'A' in
'AutofillDriverImpl'.  Since this change has already landed, though, I wouldn't
worry about going back to fix this -- just something to keep in mind for the
future :)

(FWIW, I think most editors can be configured to apply these indentation rules
more or less automatically, which removes a lot of the tedium and guesswork in
making sure that things are aligned correctly.)

On 2013/06/27 14:50:35, blundell wrote:
> 4-space indenting is correct for a wrapped argument
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...).
> I moved all the other arguments to that same indentation per what the style
> guide seems to indicate for this case.
> 
> On 2013/06/26 22:36:18, Ilya Sherman wrote:
> > nit: Reduce the indentation of this line by two spaces.
>