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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 2 months ago by blundell
Modified:
2 years, 2 months 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
Project "None" does not have a commit queue.

Messages

Total messages: 23 (0 generated)
blundell
I have not yet introduced DEPS restrictions or added a README in this CL. With ...
2 years, 2 months 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, 2 months 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 ...
2 years, 2 months 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, 2 months 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, 2 months 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 ...
2 years, 2 months 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, 2 months ago (2013-06-12 16:29:36 UTC) #7
blundell
Ping :).
2 years, 2 months 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 ...
2 years, 2 months 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, 2 months 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 ...
2 years, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months 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, 2 months ago (2013-06-15 21:26:25 UTC) #18
commit-bot: I haz the power
Change committed as 206623
2 years, 2 months 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, 2 months 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, 2 months 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, 2 months ago (2013-06-18 14:24:48 UTC) #22
Yaron
2 years, 2 months 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 c33a7a4