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

Issue 16286020: Abstract WebContentsObserver from Autofill shared code (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months, 2 weeks ago by blundell (OOO until April 28)
Modified:
10 months ago
CC:
chromium-reviews_chromium.org, Raman Kakilate, benquan, jam, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman, selim, benm
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Abstract WebContentsObserver from Autofill shared code

Introduces the AutofillDriver, whose purpose is to abstract communication from the renderer and the external world to autofill browser process code. This CL concretely uses AutofillDriverImpl to eliminate AutofillManager and
AutocompleteHistoryManager having to be WebContentsObservers. It also eliminates AutofillManager being a WebContentsUserData, instead changing it to be owned by and accessed from the AutofillDriverImpl.

TBR=benm,yfriedman,avi
TEST=No visible behavior in the change of autofill.

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

Patch Set 1 #

Patch Set 2 : Nits #

Total comments: 10

Patch Set 3 : Add missing DEPS file #

Total comments: 16

Patch Set 4 : Response to reviews #

Patch Set 5 : Nit #

Total comments: 32

Patch Set 6 : Response to review #

Total comments: 16

Patch Set 7 : Rebase #

Patch Set 8 : Response to review of PS #6 #

Total comments: 2

Patch Set 9 : Response to review of PS #8 #

Patch Set 10 : Rebase #

Patch Set 11 : Rebase to fix conflict #

Unified diffs Side-by-side diffs Delta from patch set Stats (+433 lines, -194 lines) Lint Patch
M android_webview/native/DEPS View 1 2 3 1 chunk +1 line, -0 lines 0 comments ? errors Download
M android_webview/native/aw_contents.cc View 1 2 3 4 5 6 7 8 4 chunks +7 lines, -13 lines 0 comments 0 errors Download
M chrome/browser/android/tab_android.cc View 1 2 3 2 chunks +4 lines, -8 lines 0 comments 0 errors Download
M chrome/browser/autofill/autofill_browsertest.cc View 1 2 3 4 5 3 chunks +3 lines, -2 lines 0 comments 0 errors Download
M chrome/browser/autofill/autofill_external_delegate_browsertest.cc View 1 2 3 4 5 6 5 chunks +8 lines, -3 lines 0 comments 0 errors Download
M chrome/browser/autofill/autofill_interactive_uitest.cc View 1 2 3 4 5 3 chunks +3 lines, -2 lines 0 comments 0 errors Download
M chrome/browser/autofill/form_structure_browsertest.cc View 1 2 3 4 5 2 chunks +4 lines, -1 line 0 comments 0 errors Download
M chrome/browser/password_manager/password_manager_delegate_impl.cc View 1 2 3 4 5 10 2 chunks +4 lines, -3 lines 0 comments 0 errors Download
M chrome/browser/ui/autofill/autofill_popup_controller_browsertest.cc View 1 2 3 4 5 2 chunks +3 lines, -1 line 0 comments 0 errors Download
M chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc View 1 2 3 4 5 6 4 chunks +14 lines, -5 lines 0 comments 0 errors Download
M chrome/browser/ui/browser_tab_contents.cc View 1 2 3 3 chunks +8 lines, -11 lines 0 comments 0 errors Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments ? errors Download
M components/autofill.gypi View 1 2 3 4 5 6 10 2 chunks +3 lines, -0 lines 0 comments ? errors Download
M components/autofill/browser/autocomplete_history_manager.h View 1 2 3 4 5 6 7 8 3 chunks +12 lines, -7 lines 0 comments 0 errors Download
M components/autofill/browser/autocomplete_history_manager.cc View 1 2 3 4 5 6 7 10 5 chunks +17 lines, -22 lines 0 comments 0 errors Download
M components/autofill/browser/autocomplete_history_manager_unittest.cc View 1 2 3 4 5 6 7 8 10 5 chunks +23 lines, -25 lines 0 comments 2 errors Download
A components/autofill/browser/autofill_driver.h View 1 2 3 4 5 6 7 1 chunk +27 lines, -0 lines 0 comments 1 errors Download
M components/autofill/browser/autofill_external_delegate_unittest.cc View 1 2 3 4 5 6 10 5 chunks +8 lines, -3 lines 0 comments 0 errors Download
M components/autofill/browser/autofill_manager.h View 1 2 3 4 5 6 10 7 chunks +22 lines, -27 lines 0 comments 0 errors Download
M components/autofill/browser/autofill_manager.cc View 1 2 3 4 5 6 10 16 chunks +27 lines, -50 lines 0 comments 0 errors Download
M components/autofill/browser/autofill_manager_unittest.cc View 1 2 3 4 5 10 7 chunks +9 lines, -5 lines 0 comments ? errors Download
M components/autofill/browser/autofill_metrics_unittest.cc View 1 2 3 4 5 6 7 8 9 10 5 chunks +7 lines, -3 lines 0 comments 1 errors Download
A components/autofill/browser/test_autofill_driver.h View 1 2 3 4 5 1 chunk +34 lines, -0 lines 0 comments 1 errors Download
A components/autofill/browser/test_autofill_driver.cc View 1 2 3 4 5 1 chunk +18 lines, -0 lines 0 comments 1 errors Download
M components/autofill/content/browser/autocheckout_manager_unittest.cc View 1 2 3 4 5 6 7 8 10 5 chunks +7 lines, -3 lines 0 comments 0 errors Download
A components/autofill/content/browser/autofill_driver_impl.h View 1 2 3 4 5 6 7 1 chunk +71 lines, -0 lines 0 comments 1 errors Download
A components/autofill/content/browser/autofill_driver_impl.cc View 1 2 3 4 5 6 7 1 chunk +87 lines, -0 lines 0 comments 4 errors Download
Commit:

Messages

Total messages: 23
blundell (OOO until April 28)
I have not yet introduced DEPS restrictions or added a README in this CL. With ...
10 months, 2 weeks ago #1
Evan Stade
moving the files early seems fine to me. https://codereview.chromium.org/16286020/diff/2001/components/autofill/browser/autocomplete_history_manager.cc File components/autofill/browser/autocomplete_history_manager.cc (left): https://codereview.chromium.org/16286020/diff/2001/components/autofill/browser/autocomplete_history_manager.cc#oldcode66 components/autofill/browser/autocomplete_history_manager.cc:66: IPC_MESSAGE_HANDLER(AutofillHostMsg_RemoveAutocompleteEntry, ...
10 months, 2 weeks ago #2
Ilya Sherman
https://codereview.chromium.org/16286020/diff/2001/components/autofill/browser/autocomplete_history_manager.cc File components/autofill/browser/autocomplete_history_manager.cc (left): https://codereview.chromium.org/16286020/diff/2001/components/autofill/browser/autocomplete_history_manager.cc#oldcode66 components/autofill/browser/autocomplete_history_manager.cc:66: IPC_MESSAGE_HANDLER(AutofillHostMsg_RemoveAutocompleteEntry, On 2013/06/04 18:06:36, Evan Stade wrote: > On ...
10 months, 2 weeks ago #3
blundell (OOO until April 28)
Thanks for the reviews. Most significant change in this iteration is having AutofillDriver be an ...
10 months, 1 week ago #4
blundell (OOO until April 28)
https://codereview.chromium.org/16286020/diff/9001/components/autofill/content/browser/autofill_driver.cc File components/autofill/content/browser/autofill_driver.cc (right): https://codereview.chromium.org/16286020/diff/9001/components/autofill/content/browser/autofill_driver.cc#newcode16 components/autofill/content/browser/autofill_driver.cc:16: const char* kAutofillDriverWebContentsUserDataKey = This comment was meant to ...
10 months, 1 week ago #5
Ilya Sherman
https://codereview.chromium.org/16286020/diff/2001/components/autofill/browser/autocomplete_history_manager.cc File components/autofill/browser/autocomplete_history_manager.cc (left): https://codereview.chromium.org/16286020/diff/2001/components/autofill/browser/autocomplete_history_manager.cc#oldcode66 components/autofill/browser/autocomplete_history_manager.cc:66: IPC_MESSAGE_HANDLER(AutofillHostMsg_RemoveAutocompleteEntry, (b), though not all of autofill_agent will be ...
10 months, 1 week ago #6
blundell (OOO until April 28)
Thanks for the thorough review. Your comment re: commenting the fact that AutofillDriver needing to ...
10 months, 1 week ago #7
blundell (OOO until April 28)
Ping :).
10 months, 1 week ago #8
Ilya Sherman
https://codereview.chromium.org/16286020/diff/44001/components/autofill/browser/autofill_manager.cc File components/autofill/browser/autofill_manager.cc (right): https://codereview.chromium.org/16286020/diff/44001/components/autofill/browser/autofill_manager.cc#newcode206 components/autofill/browser/autofill_manager.cc:206: delegate->GetAutocheckoutWhitelistManager(); Where does this happen now? https://codereview.chromium.org/16286020/diff/57001/components/autofill/browser/autocomplete_history_manager.h File components/autofill/browser/autocomplete_history_manager.h ...
10 months, 1 week ago #9
blundell (OOO until April 28)
Thanks! Had to rebase -- the relevant diff is between PS 7 and PS 8. ...
10 months, 1 week ago #10
Ilya Sherman
Thanks! LGTM % nits. https://codereview.chromium.org/16286020/diff/57001/components/autofill/browser/autocomplete_history_manager_unittest.cc File components/autofill/browser/autocomplete_history_manager_unittest.cc (right): https://codereview.chromium.org/16286020/diff/57001/components/autofill/browser/autocomplete_history_manager_unittest.cc#newcode236 components/autofill/browser/autocomplete_history_manager_unittest.cc:236: } I'm pretty sure this ...
10 months, 1 week ago #11
blundell (OOO until April 28)
Thanks! Evan, I'm going to land this to enable progress on follow-up work, but please ...
10 months, 1 week ago #12
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/16286020/116001
10 months, 1 week ago #13
I haz the power (commit-bot)
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=9385
10 months, 1 week ago #14
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/16286020/125001
10 months, 1 week ago #15
I haz the power (commit-bot)
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=9392
10 months, 1 week ago #16
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/16286020/133001
10 months, 1 week ago #17
blundell (OOO until April 28)
benm: android_webview yfriedman: chrome/browser/android avi: chrome/browser/ui/browser_tab_contents.cc
10 months, 1 week ago #18
I haz the power (commit-bot)
Change committed as 206623
10 months, 1 week ago #19
Avi
browser tab contents LGTM I assume a similar change doesn't need to be made for ...
10 months ago #20
blundell (OOO until April 28)
On 2013/06/17 23:48:36, Avi wrote: > browser tab contents LGTM > > I assume a ...
10 months ago #21
Avi
On 2013/06/18 08:09:03, blundell wrote: > On 2013/06/17 23:48:36, Avi wrote: > > browser tab ...
10 months ago #22
Yaron_ooo-April_21
10 months ago #23
Message was sent while issue was closed.
tab_android lgtm
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6