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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 1 month ago by blundell
Modified:
2 years, 1 month ago
CC:
chromium-reviews, 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 (Away 7.18-8.03), selim, benm (inactive)
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) Patch
M android_webview/native/DEPS View 1 2 3 1 chunk +1 line, -0 lines 0 comments 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 Download
M chrome/browser/android/tab_android.cc View 1 2 3 2 chunks +4 lines, -8 lines 0 comments Download
M chrome/browser/autofill/autofill_browsertest.cc View 1 2 3 4 5 3 chunks +3 lines, -2 lines 0 comments 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 Download
M chrome/browser/autofill/autofill_interactive_uitest.cc View 1 2 3 4 5 3 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/autofill/form_structure_browsertest.cc View 1 2 3 4 5 2 chunks +4 lines, -1 line 0 comments 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 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 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 Download
M chrome/browser/ui/browser_tab_contents.cc View 1 2 3 3 chunks +8 lines, -11 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M components/autofill.gypi View 1 2 3 4 5 6 10 2 chunks +3 lines, -0 lines 0 comments 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 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 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 Download
A components/autofill/browser/autofill_driver.h View 1 2 3 4 5 6 7 1 chunk +27 lines, -0 lines 0 comments 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 Download
M components/autofill/browser/autofill_manager.h View 1 2 3 4 5 6 10 7 chunks +22 lines, -27 lines 0 comments Download
M components/autofill/browser/autofill_manager.cc View 1 2 3 4 5 6 10 16 chunks +27 lines, -50 lines 0 comments Download
M components/autofill/browser/autofill_manager_unittest.cc View 1 2 3 4 5 10 7 chunks +9 lines, -5 lines 0 comments 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 Download
A components/autofill/browser/test_autofill_driver.h View 1 2 3 4 5 1 chunk +34 lines, -0 lines 0 comments Download
A components/autofill/browser/test_autofill_driver.cc View 1 2 3 4 5 1 chunk +18 lines, -0 lines 0 comments 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 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 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 Download
Commit: CQ not working?

Messages

Total messages: 23 (0 generated)
blundell
I have not yet introduced DEPS restrictions or added a README in this CL. With ...
2 years, 1 month ago (2013-06-04 17:30:48 UTC) #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, ...
2 years, 1 month ago (2013-06-04 18:06:36 UTC) #2
Ilya Sherman (Away 7.18-8.03)
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 ...
2 years, 1 month ago (2013-06-05 10:50:02 UTC) #3
blundell
Thanks for the reviews. Most significant change in this iteration is having AutofillDriver be an ...
2 years, 1 month ago (2013-06-11 15:35:46 UTC) #4
blundell
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 ...
2 years, 1 month ago (2013-06-11 21:19:27 UTC) #5
Ilya Sherman (Away 7.18-8.03)
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 ...
2 years, 1 month ago (2013-06-12 00:07:45 UTC) #6
blundell
Thanks for the thorough review. Your comment re: commenting the fact that AutofillDriver needing to ...
2 years, 1 month ago (2013-06-12 16:29:36 UTC) #7
blundell
Ping :).
2 years, 1 month ago (2013-06-13 20:19:15 UTC) #8
Ilya Sherman (Away 7.18-8.03)
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 ...
2 years, 1 month ago (2013-06-14 02:44:37 UTC) #9
blundell
Thanks! Had to rebase -- the relevant diff is between PS 7 and PS 8. ...
2 years, 1 month ago (2013-06-14 09:54:08 UTC) #10
Ilya Sherman (Away 7.18-8.03)
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 ...
2 years, 1 month ago (2013-06-14 23:22:23 UTC) #11
blundell
Thanks! Evan, I'm going to land this to enable progress on follow-up work, but please ...
2 years, 1 month ago (2013-06-15 12:52:32 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/16286020/116001
2 years, 1 month ago (2013-06-15 12:52:58 UTC) #13
commit-bot: I haz the power
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
2 years, 1 month ago (2013-06-15 13:01:44 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/16286020/125001
2 years, 1 month ago (2013-06-15 16:27:12 UTC) #15
commit-bot: I haz the power
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
2 years, 1 month ago (2013-06-15 16:38:50 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/16286020/133001
2 years, 1 month ago (2013-06-15 21:23:41 UTC) #17
blundell
benm: android_webview yfriedman: chrome/browser/android avi: chrome/browser/ui/browser_tab_contents.cc
2 years, 1 month ago (2013-06-15 21:26:25 UTC) #18
commit-bot: I haz the power
Change committed as 206623
2 years, 1 month ago (2013-06-16 03:06:43 UTC) #19
Avi
browser tab contents LGTM I assume a similar change doesn't need to be made for ...
2 years, 1 month ago (2013-06-17 23:48:36 UTC) #20
blundell
On 2013/06/17 23:48:36, Avi wrote: > browser tab contents LGTM > > I assume a ...
2 years, 1 month ago (2013-06-18 08:09:03 UTC) #21
Avi
On 2013/06/18 08:09:03, blundell wrote: > On 2013/06/17 23:48:36, Avi wrote: > > browser tab ...
2 years, 1 month ago (2013-06-18 14:24:48 UTC) #22
Yaron
2 years, 1 month ago (2013-06-18 15:58:18 UTC) #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 5fa3ca5