Hook up Autofill Backend interface to SyncableServices

Make Autofill SyncableServices use a backend interface for DB work, rather than calling WebDataService directly.

This will (eventually) allow WebDataService to live entirely on the UI Thread, and also ensures that we don't expose any more of the backend functionality than is needed by the SyncableServices.

(depends on: https://codereview.chromium.org/14679005/)

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

[Cait (Slow), 2013-04-25 15:41:15]

Apologies...this CL is a bit of a beast. The parts that pertain to the
delegate-getting are:

-AutofillWebDataService (initiates Delegate request with a callback)
-WebDatabaseService (requests a db ptr, with a callback)
-WebDataServiceBackend (calls callback with db ptr)
-AutofillWebDataBackend (builds a delegate, calls client back on DB thread)
-AutofillBackendDelegate (the delegate)

The reason for the roundabout route is that ATM the delegate needs an
AutofillWebDataBackend and a WebDatabase and they live in different places.

[erikwright (departed), 2013-04-25 19:06:10]

https://codereview.chromium.org/14081043/diff/1/components/autofill/browser/w...
File components/autofill/browser/webdata/autofill_webdata_backend.cc (right):

https://codereview.chromium.org/14081043/diff/1/components/autofill/browser/w...
components/autofill/browser/webdata/autofill_webdata_backend.cc:53:
weak_ptr_factory_.InvalidateWeakPtrs();
Not necessary - this is done by ~WeakPtrFactory.

Just re-order the member declarations so that they are destroyed in this order.

https://codereview.chromium.org/14081043/diff/1/components/autofill/browser/w...
File components/autofill/browser/webdata/autofill_webdata_backend.h (right):

https://codereview.chromium.org/14081043/diff/1/components/autofill/browser/w...
components/autofill/browser/webdata/autofill_webdata_backend.h:85: // All vended
weak pointers are invalidated in dtor().
This comment is unnecessary (this is intrinsic in WeakPtrFactory).

https://codereview.chromium.org/14081043/diff/1/components/autofill/browser/w...
components/autofill/browser/webdata/autofill_webdata_backend.h:90:
scoped_ptr<AutofillBackendDelegate> delegate_;
Shouldn't the delegate be owned by whoever is using it? That's the point of the
WeakPtrs - the client can hold the delegate past the lifetime of the backend,
but at some point it will become a no-op.

Otherwise, instead of the delegate holding weakptrs to things it could hold raw
ptrs but the client holds a weak ptr to the delegate.

[tim (not reviewing), 2013-04-25 19:41:13]

Just to confirm that a few necessary sync data type invariants remain unchanged
with this patch (I haven't looked to deeply as this patch is pretty big!)

- autofill data still "lives" on the DB thread (there are no caches of data on
other threads)
- sync change processor is notified of changes synchronously on the DB thread
- there are no "async" steps involved in either direction of ProcessSyncChanges
(i.e anything proxied through DatabaseModelWorker from sync via
DoWorkAndWaitUntilDone will occur synchronously, and calls to ProcessSyncChanges
from the SyncableService will happen synchronously after native edits are
flushed to the DB on the DB thread).

[Cait (Slow), 2013-04-25 23:06:49]

On 2013/04/25 19:41:13, timsteele wrote:
> Just to confirm that a few necessary sync data type invariants remain
unchanged
> with this patch (I haven't looked to deeply as this patch is pretty big!)
> 
> - autofill data still "lives" on the DB thread (there are no caches of data on
> other threads)
> - sync change processor is notified of changes synchronously on the DB thread
> - there are no "async" steps involved in either direction of
ProcessSyncChanges
> (i.e anything proxied through DatabaseModelWorker from sync via
> DoWorkAndWaitUntilDone will occur synchronously, and calls to
ProcessSyncChanges
> from the SyncableService will happen synchronously after native edits are
> flushed to the DB on the DB thread).

I don't think any of these invariants will be affected by this change. On the
SyncableService side, the behavior is still the same, except access to the
database is provided via a delegate class, rather than the WebDataService
itself.

[Cait (Slow), 2013-05-13 17:41:09]

Ilya: PTAL at components/autofill/ and c/b/webdata/(autofill|autocomplete)*

Joi: PTAL at c/b/webdata/web_data_service_factory

Tim: PTAL at c/b/sync (and optionally, Autofill and Autocomplete
SyncableServices from a Sync perspective).

Thanks all.

[Ilya Sherman, 2013-05-13 23:44:47]

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:144:
scoped_ptr<AutofillWebDataBackend> autofill_backend_;
nit: Can this be a direct class member rather than a scoped_ptr?

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:152: :
web_database_(web_database),
nit: Indent each of these three two more spaces.  Wrapped lines due to 80-col
should be indented a total of four spaces each from where non-wrapped lines are
indented.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:154: }
nit: De-indent this line two spaces, or move it to the previous line to match.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:198:
autofill_webdata_backend_->RemoveExpiredFormElementsWrapper();
nit: Seems weird for this method, which is part of the public API, to have a
"Wrapper()" suffix.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:436: &timestamps)) {
nit: De-indent each of these lines four spaces.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.h (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.h:52: static void
CreateForWebDataService(
Optional nit: Should this name be updated to
"CreateForWebDataServiceAndBackend()"?  That name is a little unwieldy though :/

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.h:54:
autofill::AutofillWebDataBackend* syncable_service_backend);
nit: Name this "autofill_webdata_backend" to match the member variable, or
perhaps drop the "autofill_" prefix from both.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.h:91:
autofill::AutofillWebDataBackend* syncable_service_backend);
nit: Ditto.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
File chrome/browser/webdata/autofill_profile_syncable_service.h (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autofill_profile_syncable_service.h:183:
autofill::AutofillWebDataBackend* autofill_webdata_backend_;  // WEAK
nit: Perhaps just "webdata_backend_"?

https://codereview.chromium.org/14081043/diff/24001/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_service.h (right):

https://codereview.chromium.org/14081043/diff/24001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.h:98: void
NotifyAutofillMultipleChangedOnUIThread();
nit: Why did you change the visibility of this method?

[Jói, 2013-05-14 21:43:18]

LGTM once Ilya's comments are addressed.

[Cait (Slow), 2013-05-14 22:30:08]

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:144:
scoped_ptr<AutofillWebDataBackend> autofill_backend_;
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> nit: Can this be a direct class member rather than a scoped_ptr?

Done.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/profi...
File chrome/browser/sync/profile_sync_service_autofill_unittest.cc (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:152: :
web_database_(web_database),
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> nit: Indent each of these three two more spaces.  Wrapped lines due to 80-col
> should be indented a total of four spaces each from where non-wrapped lines
are
> indented.

Done.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:154: }
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> nit: De-indent this line two spaces, or move it to the previous line to match.

Done.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:198:
autofill_webdata_backend_->RemoveExpiredFormElementsWrapper();
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> nit: Seems weird for this method, which is part of the public API, to have a
> "Wrapper()" suffix.
Agreed. Since this is the only place we call this method from, I've renamed it
(and removed the corresponding method on AutofillWebData, since this is only
getting called from the DB thread) -- WDYT?

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:436: &timestamps)) {
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> nit: De-indent each of these lines four spaces.

Done.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.h (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.h:52: static void
CreateForWebDataService(
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> Optional nit: Should this name be updated to
> "CreateForWebDataServiceAndBackend()"?  That name is a little unwieldy though
:/

Done.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.h:54:
autofill::AutofillWebDataBackend* syncable_service_backend);
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> nit: Name this "autofill_webdata_backend" to match the member variable, or
> perhaps drop the "autofill_" prefix from both.

Done.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.h:91:
autofill::AutofillWebDataBackend* syncable_service_backend);
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> nit: Ditto.

Done.

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
File chrome/browser/webdata/autofill_profile_syncable_service.h (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autofill_profile_syncable_service.h:183:
autofill::AutofillWebDataBackend* autofill_webdata_backend_;  // WEAK
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> nit: Perhaps just "webdata_backend_"?

Done.

https://codereview.chromium.org/14081043/diff/24001/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_service.h (right):

https://codereview.chromium.org/14081043/diff/24001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.h:98: void
NotifyAutofillMultipleChangedOnUIThread();
On 2013/05/13 23:44:47, Ilya Sherman wrote:
> nit: Why did you change the visibility of this method?
This method is passed to the backend as a callback. It will be used to notify
the UI thread, instead of the the static NotifyOfMultipleAutofillChanges()
method. (That said, it doesn't need to be public. Protected should suffice).

[Ilya Sherman, 2013-05-14 23:03:15]

LGTM, thanks :)

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/14081043/diff/24001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:198:
autofill_webdata_backend_->RemoveExpiredFormElementsWrapper();
On 2013/05/14 22:30:08, caitkp wrote:
> On 2013/05/13 23:44:47, Ilya Sherman wrote:
> > nit: Seems weird for this method, which is part of the public API, to have a
> > "Wrapper()" suffix.
> Agreed. Since this is the only place we call this method from, I've renamed it
> (and removed the corresponding method on AutofillWebData, since this is only
> getting called from the DB thread) -- WDYT? 

Much nicer, thanks :)

https://codereview.chromium.org/14081043/diff/34001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/14081043/diff/34001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:338:
webdata_backend_->GetDatabase())->UpdateAutofillEntries(new_entries)) {
nit: Indent this line two more spaces.

[Cait (Slow), 2013-05-16 15:04:00]

ping: timsteele@chromium.org for c/b/sync changes.

https://codereview.chromium.org/14081043/diff/34001/chrome/browser/webdata/au...
File chrome/browser/webdata/autocomplete_syncable_service.cc (right):

https://codereview.chromium.org/14081043/diff/34001/chrome/browser/webdata/au...
chrome/browser/webdata/autocomplete_syncable_service.cc:338:
webdata_backend_->GetDatabase())->UpdateAutofillEntries(new_entries)) {
On 2013/05/14 23:03:15, Ilya Sherman wrote:
> nit: Indent this line two more spaces.

Done.

[tim (not reviewing), 2013-05-23 01:15:14]

sync/ LGTM

[commit-bot: I haz the power, 2013-05-23 15:19:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/14081043/50001

[commit-bot: I haz the power, 2013-05-23 17:22:24]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-23 18:37:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/caitkp@chromium.org/14081043/50001

[commit-bot: I haz the power, 2013-05-24 06:23:27]

Change committed as 201979