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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 11 months ago by blundell
Modified:
1 year, 11 months ago
Reviewers:
Ilya Sherman
CC:
chromium-reviews, 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 (inactive)
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 #

Messages

Total messages: 6 (0 generated)
blundell
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 ...
1 year, 11 months ago (2013-06-26 20:42:30 UTC) #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 ...
1 year, 11 months ago (2013-06-26 22:36:18 UTC) #2
blundell
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: > ...
1 year, 11 months ago (2013-06-27 14:50:35 UTC) #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
1 year, 11 months ago (2013-06-27 14:50:48 UTC) #4
I haz the power - commit-bot
Change committed as 208957
1 year, 11 months ago (2013-06-27 18:41:56 UTC) #5
Ilya Sherman
1 year, 11 months ago (2013-06-27 22:16:09 UTC) #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 ec887be