Chromium Code Reviews
Help | Chromium Project | Sign in
(54)

Issue 17893010: In components/autofill, move notification handling into content driver. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 months, 3 weeks ago by blundell (OOO until April 28)
Modified:
9 months, 3 weeks ago
Reviewers:
Ilya Sherman
CC:
chromium-reviews_chromium.org, Raman Kakilate, benquan, jam, ahutter, browser-components-watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org, selim, benm
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 12

Patch Set 2 : Response to review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -267 lines) Lint Patch
A + chrome/browser/autofill/autofill_driver_impl_browsertest.cc View 1 4 chunks +22 lines, -45 lines 0 comments ? errors Download
D chrome/browser/autofill/autofill_external_delegate_browsertest.cc View 1 chunk +0 lines, -168 lines 0 comments ? errors Download
M chrome/chrome_tests.gypi View 1 chunk +1 line, -1 line 0 comments ? errors Download
M components/autofill/content/browser/autofill_driver_impl.h View 1 3 chunks +11 lines, -0 lines 0 comments ? errors Download
M components/autofill/content/browser/autofill_driver_impl.cc View 3 chunks +27 lines, -0 lines 0 comments ? errors Download
M components/autofill/core/browser/DEPS View 1 chunk +0 lines, -7 lines 0 comments ? errors Download
M components/autofill/core/browser/autofill_external_delegate.h View 4 chunks +1 line, -12 lines 0 comments 0 errors Download
M components/autofill/core/browser/autofill_external_delegate.cc View 3 chunks +0 lines, -27 lines 0 comments 0 errors Download
M components/autofill/core/browser/personal_data_manager_unittest.cc View 2 chunks +0 lines, -7 lines 0 comments 0 errors Download
Commit:

Messages

Total messages: 6
blundell (OOO until April 28)
https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autofill_driver_impl_browsertest.cc File chrome/browser/autofill/autofill_driver_impl_browsertest.cc (left): https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autofill_driver_impl_browsertest.cc#oldcode100 chrome/browser/autofill/autofill_driver_impl_browsertest.cc:100: AutofillManager::RegisterUserPrefs(manager_delegate_.GetPrefRegistry()); Deleting this call doesn't seem to have any ...
9 months, 3 weeks ago #1
Ilya Sherman
LGTM https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autofill_driver_impl_browsertest.cc File chrome/browser/autofill/autofill_driver_impl_browsertest.cc (left): https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autofill_driver_impl_browsertest.cc#oldcode100 chrome/browser/autofill/autofill_driver_impl_browsertest.cc:100: AutofillManager::RegisterUserPrefs(manager_delegate_.GetPrefRegistry()); On 2013/06/26 20:42:31, blundell wrote: > Deleting ...
9 months, 3 weeks ago #2
blundell (OOO until April 28)
Thanks! https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autofill_driver_impl_browsertest.cc File chrome/browser/autofill/autofill_driver_impl_browsertest.cc (left): https://codereview.chromium.org/17893010/diff/1/chrome/browser/autofill/autofill_driver_impl_browsertest.cc#oldcode100 chrome/browser/autofill/autofill_driver_impl_browsertest.cc:100: AutofillManager::RegisterUserPrefs(manager_delegate_.GetPrefRegistry()); On 2013/06/26 22:36:18, Ilya Sherman wrote: > ...
9 months, 3 weeks ago #3
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/17893010/8002
9 months, 3 weeks ago #4
I haz the power (commit-bot)
Change committed as 208957
9 months, 3 weeks ago #5
Ilya Sherman
9 months, 3 weeks ago #6
Message was sent while issue was closed.
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.
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1275:d14800f88434