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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years ago by blundell
Modified:
3 years 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, 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 queue not available (can’t edit this change).

Messages

Total messages: 23 (0 generated)
blundell
I have not yet introduced DEPS restrictions or added a README in this CL. With ...
3 years 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, ...
3 years ago (2013-06-04 18:06:36 UTC) #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 ...
3 years 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 ...
3 years 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 ...
3 years ago (2013-06-11 21:19:27 UTC) #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 ...
3 years 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 ...
3 years ago (2013-06-12 16:29:36 UTC) #7
blundell
Ping :).
3 years ago (2013-06-13 20:19:15 UTC) #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 ...
3 years ago (2013-06-14 02:44:37 UTC) #9
blundell
Thanks! Had to rebase -- the relevant diff is between PS 7 and PS 8. ...
3 years ago (2013-06-14 09:54:08 UTC) #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 ...
3 years 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 ...
3 years 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
3 years 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
3 years 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
3 years 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
3 years 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
3 years 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
3 years ago (2013-06-15 21:26:25 UTC) #18
commit-bot: I haz the power
Change committed as 206623
3 years ago (2013-06-16 03:06:43 UTC) #19
Avi (OOO 4 July - 8 July)
browser tab contents LGTM I assume a similar change doesn't need to be made for ...
3 years 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 ...
3 years ago (2013-06-18 08:09:03 UTC) #21
Avi (OOO 4 July - 8 July)
On 2013/06/18 08:09:03, blundell wrote: > On 2013/06/17 23:48:36, Avi wrote: > > browser tab ...
3 years ago (2013-06-18 14:24:48 UTC) #22
Yaron
3 years 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 2abcc9e