Create an AutofillBackend interface

Create an interface which SyncableServices can use for making changes directly on the Autofill DB.

TBR=akalin@chromium.org (c/b/sync/)
BUG=230920
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=199904

[Cait (Slow), 2013-05-06 22:20:52]

This patch provides a delegate which DBThreaded users of AutofillWebData
(specifically the syncableServices) can use instead of holding a ref to the
WebDataService.

Joi: PTAL at components/webdata/
Ilya: PTAL at components/autofill/browser/webdata/

The actual hooking-up of the delegate will be done in the this CL (to avoid
having too many changes go in at once):
https://codereview.chromium.org/14081043/,

Thanks,
Cait

[Jói, 2013-05-06 23:08:38]

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend.cc (right):

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.cc:26: class
SyncableAutofillDelegate : public SyncableServiceDelegate {
Do you expect that there might be multiple implementations of
SyncableServiceDelegate? If not (and maybe even if), it doesn't really seem like
a delegate to me... more like an interface implemented by AutofillWebDataService
and possibly other services - in which case maybe it should be called
SyncableServiceDatabase or some such?

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_service.cc (right):

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.cc:189: 
spurious added blank line?

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

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.h:116: // All
vended weak pointers are invalidated in ShutdownDatabase().
Suggest documenting which thread this factory is used from, since it can only be
used from one.

https://codereview.chromium.org/14679005/diff/10003/components/webdata/common...
File components/webdata/common/web_database_service.h (right):

https://codereview.chromium.org/14679005/diff/10003/components/webdata/common...
components/webdata/common/web_database_service.h:82: void GetDatabaseOnDB(const
DBCallback& callback);
I'm wondering about naming here. The name could easily be misunderstood to mean
you should call the method from the DB thread... do you know if there is
precedent for this kind of name?

[Ilya Sherman, 2013-05-07 00:02:42]

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend.cc (right):

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.cc:89: };
nit: Please tuck this into an anonymous namespace.

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend.h (right):

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:111: const
base::Closure& on_changed_callback,
nit: Docs for this param

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_service.cc (right):

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.cc:40: 
nit: Spurious newline

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

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.h:109: void
GetDelegateOnDB(const DelegateOnDBCallback& del_callback);
nit: Please don't use abbreviations like "del" in names.

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
File components/autofill/browser/webdata/syncable_service_delegate.h (right):

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_delegate.h:17: class
SyncableServiceDelegate {
I'm still trying to wrap my mind around what this class is meant to be used for.
 Would it make sense to name this class "AutofillWebdataBackend", and modify the
existing class to be named AutofillWebdataBackendImpl and inherit from this one?

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_delegate.h:23: // Add an
observer to |autofill_backend_|.
nit: This class doesn't have an |autofill_backend_| variable.

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_delegate.h:27: // Remove an
observer from |autofill_backend_|.
Ditto.

https://codereview.chromium.org/14679005/diff/10003/components/webdata/common...
File components/webdata/common/web_data_service_backend.cc (right):

https://codereview.chromium.org/14679005/diff/10003/components/webdata/common...
components/webdata/common/web_data_service_backend.cc:101:
callback.Run(db_.get());
This code looks synchronous to me...  What makes the method async?

[Jói, 2013-05-07 09:56:10]

https://codereview.chromium.org/14679005/diff/10003/components/webdata/common...
File components/webdata/common/web_data_service_backend.cc (right):

https://codereview.chromium.org/14679005/diff/10003/components/webdata/common...
components/webdata/common/web_data_service_backend.cc:101:
callback.Run(db_.get());
On 2013/05/07 00:02:43, Ilya Sherman wrote:
> This code looks synchronous to me...  What makes the method async?

Yeah, I stumbled on that one as well, so it definitely needs a bit of extra
documentation. The trick seems to be that it is called from
WebDatabaseService::GetDatabaseOnDB which calls it via a PostTask to the DB
thread.

[Cait (Slow), 2013-05-07 19:22:07]

Following Ilya's suggestion and a discussion with Jói this morning, I've changed
the code so that we now expose a small part of the AutofillWebdataBackend to the
SyncableServices via an interface.

This removes the requirements for callbacks calling callbacks and makes the
whole ordeal much more straight-forward.

PTAL.

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_service.cc (right):

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.cc:40: 
On 2013/05/07 00:02:43, Ilya Sherman wrote:
> nit: Spurious newline

Done.

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.cc:189: 
On 2013/05/06 23:08:39, Jói wrote:
> spurious added blank line?

Done.

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

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.h:116: // All
vended weak pointers are invalidated in ShutdownDatabase().
On 2013/05/06 23:08:39, Jói wrote:
> Suggest documenting which thread this factory is used from, since it can only
be
> used from one.

Done.

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
File components/autofill/browser/webdata/syncable_service_delegate.h (right):

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_delegate.h:17: class
SyncableServiceDelegate {
On 2013/05/07 00:02:43, Ilya Sherman wrote:
> I'm still trying to wrap my mind around what this class is meant to be used
for.
>  Would it make sense to name this class "AutofillWebdataBackend", and modify
the
> existing class to be named AutofillWebdataBackendImpl and inherit from this
one?

Done.

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_delegate.h:23: // Add an
observer to |autofill_backend_|.
On 2013/05/07 00:02:43, Ilya Sherman wrote:
> nit: This class doesn't have an |autofill_backend_| variable.

Done.

https://codereview.chromium.org/14679005/diff/10003/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_delegate.h:27: // Remove an
observer from |autofill_backend_|.
On 2013/05/07 00:02:43, Ilya Sherman wrote:
> Ditto.

Done.

[Jói, 2013-05-07 19:39:41]

The approach looks good, I just have a bunch of nits and such.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend.cc (right):

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.cc:66: if
(!on_changed_callback_ .get()|| on_changed_callback_->is_null())
no space before .get()

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.cc:69:
BrowserThread::UI,
these can align to the parenthesis (with the first parameter immediately
following it)

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend.h (right):

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:40: // the UI
thread of these changes.
"these changes" is a little ambiguous

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_service.cc (right):

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.cc:208:
BrowserThread::DB, FROM_HERE,
indent two more spaces, or start right after parenthesis and indent to it

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

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.h:101: void
GetSyncableBackend(
Needs documentation.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
File components/autofill/browser/webdata/syncable_service_backend.h (right):

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:14: // Intreface
for doing Autofill work directly on the DB thread (used by
Intreface -> Interface

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:16: class
SyncableServiceBackend {
Should this be named AutofillSyncableServiceBackend perhaps?

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:19: // Get a raw
ptr to the WebDatabase
ptr -> pointer, and please end the sentence with a period.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:35: // it
asynchronously notifies listeners on the UI thread.
it asynchronously -> asynchronously

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:39: } // autofill
autofill -> namespace autofill

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:41: #endif
//COMPONENTS_AUTOFILL_BROWSER_WEBDATA_SYNCABLE_SERVICE_BACKEND_H_
two spaces before // and one after

https://codereview.chromium.org/14679005/diff/10004/components/webdata/common...
File components/webdata/common/web_database_service.h (right):

https://codereview.chromium.org/14679005/diff/10004/components/webdata/common...
components/webdata/common/web_database_service.h:78: virtual
scoped_refptr<WebDataServiceBackend> GetBackend() const;
Documentation?

[Cait (Slow), 2013-05-07 20:14:56]

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend.cc (right):

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.cc:66: if
(!on_changed_callback_ .get()|| on_changed_callback_->is_null())
On 2013/05/07 19:39:41, Jói wrote:
> no space before .get()

Done.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.cc:69:
BrowserThread::UI,
On 2013/05/07 19:39:41, Jói wrote:
> these can align to the parenthesis (with the first parameter immediately
> following it)

Done.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend.h (right):

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:40: // the UI
thread of these changes.
On 2013/05/07 19:39:41, Jói wrote:
> "these changes" is a little ambiguous

Done.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_service.cc (right):

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.cc:208:
BrowserThread::DB, FROM_HERE,
On 2013/05/07 19:39:41, Jói wrote:
> indent two more spaces, or start right after parenthesis and indent to it

Done.

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

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.h:101: void
GetSyncableBackend(
On 2013/05/07 19:39:41, Jói wrote:
> Needs documentation.

Done.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
File components/autofill/browser/webdata/syncable_service_backend.h (right):

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:14: // Intreface
for doing Autofill work directly on the DB thread (used by
On 2013/05/07 19:39:41, Jói wrote:
> Intreface -> Interface

Done.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:16: class
SyncableServiceBackend {
On 2013/05/07 19:39:41, Jói wrote:
> Should this be named AutofillSyncableServiceBackend perhaps?

It's in the autofill namespace, so will usually appear as
autofill::SyncableServiceBackend, but if you think it would be clearer to have
"Autofill" as part of the class-name, I could rename it to
AutofillSyncableBackend.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:19: // Get a raw
ptr to the WebDatabase
On 2013/05/07 19:39:41, Jói wrote:
> ptr -> pointer, and please end the sentence with a period.

Done.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:35: // it
asynchronously notifies listeners on the UI thread.
On 2013/05/07 19:39:41, Jói wrote:
> it asynchronously -> asynchronously

Done.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:39: } // autofill
On 2013/05/07 19:39:41, Jói wrote:
> autofill -> namespace autofill

Done.

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:41: #endif
//COMPONENTS_AUTOFILL_BROWSER_WEBDATA_SYNCABLE_SERVICE_BACKEND_H_
On 2013/05/07 19:39:41, Jói wrote:
> two spaces before // and one after

Done.

https://codereview.chromium.org/14679005/diff/10004/components/webdata/common...
File components/webdata/common/web_database_service.h (right):

https://codereview.chromium.org/14679005/diff/10004/components/webdata/common...
components/webdata/common/web_database_service.h:78: virtual
scoped_refptr<WebDataServiceBackend> GetBackend() const;
On 2013/05/07 19:39:41, Jói wrote:
> Documentation?

Done.

[Jói, 2013-05-07 20:19:15]

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
File components/autofill/browser/webdata/syncable_service_backend.h (right):

https://codereview.chromium.org/14679005/diff/10004/components/autofill/brows...
components/autofill/browser/webdata/syncable_service_backend.h:16: class
SyncableServiceBackend {
On 2013/05/07 20:14:56, caitkp wrote:
> On 2013/05/07 19:39:41, Jói wrote:
> > Should this be named AutofillSyncableServiceBackend perhaps?
> 
> It's in the autofill namespace, so will usually appear as
> autofill::SyncableServiceBackend, but if you think it would be clearer to have
> "Autofill" as part of the class-name, I could rename it to
> AutofillSyncableBackend.

With the namespace that seems fine to me; will leave the final word to Ilya.

[Ilya Sherman, 2013-05-08 00:39:05]

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
File components/autofill/browser/webdata/autofill_webdata_backend.cc (right):

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:46: if
(!web_database_backend_)
Why is it possible for web_database_backend_ to be NULL?

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:53: if
(!web_database_backend_ || !web_database_backend_->database())
Why is it possible for web_database_backend_ or
web_database_backend_->database() to be NULL?

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:57:
WebDatabase::COMMIT_NEEDED) {
nit: Indent four more spaces.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:66: if
(!on_changed_callback_.get()|| on_changed_callback_->is_null())
nit: Please add a space before the "||" operator.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:66: if
(!on_changed_callback_.get()|| on_changed_callback_->is_null())
Why is it possible for the callback to be null?

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:67: return;
nit: Please leave a blank line after this one.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:70:
*(on_changed_callback_.get());
This line looks like it's missing a parenthesis...

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:330: }
Hrm, why did this get moved down?

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
File components/autofill/browser/webdata/autofill_webdata_backend.h (right):

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:40: // used to
notify the UI thread of changes initiated by Sync.
Please document that |on_changed_callback| might be Run() multiple times.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:41:
AutofillWebDataBackend(scoped_refptr<WebDataServiceBackend> web_db_backend,
nit: Please name this variable |web_database_backend| to match the member
variable name.  Also, why does the class name use "DataService" whereas the
variable name is "database"?

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:46: OVERRIDE;
nit: Indent two more spaces.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:48:
AutofillWebDataServiceObserverOnDBThread* observer) OVERRIDE;
nit: Indent two more spaces.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:135: // Ptr to
WebDataServiceBackend to allow direct access to DB.
nit: Omit "Ptr to"

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
File components/autofill/browser/webdata/autofill_webdata_service.h (right):

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_service.h:101: // Takes a
callback which will be called on the DBThread with a pointer to a
nit: "DBThread" -> "DB thread"; ditto below.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_service.h:105: const
base::Callback<void(SyncableServiceBackend*)>& callback);
Should this be returned as a scoped_refptr?  What's guaranteeing the lifetime of
the syncable backend for the client?

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_service.h:112: // This
factory is used on the UIThread. All vended weak pointers are
nit: "UIThread" -> "UI thread"

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_service.h:114:
base::WeakPtrFactory<AutofillWebDataService> weak_ptr_factory_;
nit: Member variables should always have private visibility.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
File components/autofill/browser/webdata/syncable_service_backend.h (right):

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/syncable_service_backend.h:16: class
SyncableServiceBackend {
I still think that it would be better to name this class
"AutofillWebDataBackend", and rename the existing class to
"AutofillWebDataBackendImpl", as is the common pattern in the //content/ API.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/syncable_service_backend.h:21: virtual
WebDatabase* GetDatabaseOnDB() = 0;
nit: The "OnDB" suffix seems redundant, since this entire class is meant to be
used only on the DB thread -- right?

https://codereview.chromium.org/14679005/diff/9005/components/webdata/common/...
File components/webdata/common/web_database_service.cc (right):

https://codereview.chromium.org/14679005/diff/9005/components/webdata/common/...
components/webdata/common/web_database_service.cc:96: if (!wds_backend_)
Why can wds_backend_ be NULL?

https://codereview.chromium.org/14679005/diff/9005/components/webdata/common/...
File components/webdata/common/web_database_service.h (right):

https://codereview.chromium.org/14679005/diff/9005/components/webdata/common/...
components/webdata/common/web_database_service.h:80: virtual
scoped_refptr<WebDataServiceBackend> GetBackend() const;
nit: Why is this method virtual?

[Cait (Slow), 2013-05-08 19:16:01]

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
File components/autofill/browser/webdata/autofill_webdata_backend.cc (right):

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:46: if
(!web_database_backend_)
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> Why is it possible for web_database_backend_ to be NULL?

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:53: if
(!web_database_backend_ || !web_database_backend_->database())
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> Why is it possible for web_database_backend_ or
> web_database_backend_->database() to be NULL?

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:57:
WebDatabase::COMMIT_NEEDED) {
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: Indent four more spaces.

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:66: if
(!on_changed_callback_.get()|| on_changed_callback_->is_null())
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: Please add a space before the "||" operator.

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:67: return;
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:70:
*(on_changed_callback_.get());
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> This line looks like it's missing a parenthesis...

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:330: }
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> Hrm, why did this get moved down?

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
File components/autofill/browser/webdata/autofill_webdata_backend.h (right):

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:40: // used to
notify the UI thread of changes initiated by Sync.
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> Please document that |on_changed_callback| might be Run() multiple times.

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:41:
AutofillWebDataBackend(scoped_refptr<WebDataServiceBackend> web_db_backend,
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: Please name this variable |web_database_backend| to match the member
> variable name.  Also, why does the class name use "DataService" whereas the
> variable name is "database"?
Adding a todo to rename WebDataServiceBackend to WebDatabaseBackend. This was a
mistake on my part, and now that the class is used by more than just the
WebDatabaseService, it should definitely be fixed to avoid further confusion.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:46: OVERRIDE;
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: Indent two more spaces.

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:48:
AutofillWebDataServiceObserverOnDBThread* observer) OVERRIDE;
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: Indent two more spaces.

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:135: // Ptr to
WebDataServiceBackend to allow direct access to DB.
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: Omit "Ptr to"

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
File components/autofill/browser/webdata/autofill_webdata_service.h (right):

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_service.h:101: // Takes a
callback which will be called on the DBThread with a pointer to a
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: "DBThread" -> "DB thread"; ditto below.

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_service.h:105: const
base::Callback<void(SyncableServiceBackend*)>& callback);
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> Should this be returned as a scoped_refptr?  What's guaranteeing the lifetime
of
> the syncable backend for the client?
Both the backend and the syncable services are owned by the
AutofillWebDataService. The backend will outlive the syncable services as they
(the syncable services) get destroyed on the first pass of the shutdown process
(ShutdownOnUIThread) and the backend does not get destroyed until the second
pass (dtor).

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_service.h:112: // This
factory is used on the UIThread. All vended weak pointers are
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: "UIThread" -> "UI thread"

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_service.h:114:
base::WeakPtrFactory<AutofillWebDataService> weak_ptr_factory_;
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: Member variables should always have private visibility.

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
File components/autofill/browser/webdata/syncable_service_backend.h (right):

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/syncable_service_backend.h:16: class
SyncableServiceBackend {
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> I still think that it would be better to name this class
> "AutofillWebDataBackend", and rename the existing class to
> "AutofillWebDataBackendImpl", as is the common pattern in the //content/ API.

Done.

https://codereview.chromium.org/14679005/diff/9005/components/autofill/browse...
components/autofill/browser/webdata/syncable_service_backend.h:21: virtual
WebDatabase* GetDatabaseOnDB() = 0;
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: The "OnDB" suffix seems redundant, since this entire class is meant to be
> used only on the DB thread -- right?

Done.

https://codereview.chromium.org/14679005/diff/9005/components/webdata/common/...
File components/webdata/common/web_database_service.cc (right):

https://codereview.chromium.org/14679005/diff/9005/components/webdata/common/...
components/webdata/common/web_database_service.cc:96: if (!wds_backend_)
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> Why can wds_backend_ be NULL?
Because the it does not get created until AddTable() has been called at least
once.

https://codereview.chromium.org/14679005/diff/9005/components/webdata/common/...
File components/webdata/common/web_database_service.h (right):

https://codereview.chromium.org/14679005/diff/9005/components/webdata/common/...
components/webdata/common/web_database_service.h:80: virtual
scoped_refptr<WebDataServiceBackend> GetBackend() const;
On 2013/05/08 00:39:06, Ilya Sherman wrote:
> nit: Why is this method virtual?

Done.

[Ilya Sherman, 2013-05-09 05:04:24]

Thanks, getting close :)

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend_impl.cc
(right):

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.cc:29:
on_changed_callback_(new base::Closure(on_changed_callback)) {
nit: Why are you storing the as a pointer rather than as a direct member?

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.cc:49: return
web_database_backend_->database();
nit: De-indent two spaces.

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.cc:57: }
Is this new behavior that's being added in this CL?  I don't see any
corresponding code being removed from anywhere else in this CL.

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.cc:64:
*(on_changed_callback_.get()));
nit: I don't think the .get() is necessary.

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend_impl.h
(right):

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.h:38: //
|web_db_backend| is used to access the WebDatabase directly for
nit: Please update the variable name here.

https://codereview.chromium.org/14679005/diff/36001/components/webdata/common...
File components/webdata/common/web_database_service.cc (right):

https://codereview.chromium.org/14679005/diff/36001/components/webdata/common...
components/webdata/common/web_database_service.cc:97: return NULL;
nit: These two lines don't seem like they really have any effect.

https://codereview.chromium.org/14679005/diff/36001/components/webdata/common...
File components/webdata/common/web_database_service.h (right):

https://codereview.chromium.org/14679005/diff/36001/components/webdata/common...
components/webdata/common/web_database_service.h:79: // WebDatabaseService).
nit: What does it mean for a scoped_refptr to be owned by some other object?

[Cait (Slow), 2013-05-09 18:29:54]

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend_impl.cc
(right):

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.cc:29:
on_changed_callback_(new base::Closure(on_changed_callback)) {
On 2013/05/09 05:04:24, Ilya Sherman wrote:
> nit: Why are you storing the as a pointer rather than as a direct member?

Done.

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.cc:49: return
web_database_backend_->database();
On 2013/05/09 05:04:24, Ilya Sherman wrote:
> nit: De-indent two spaces.

Done.

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.cc:57: }
On 2013/05/09 05:04:24, Ilya Sherman wrote:
> Is this new behavior that's being added in this CL?  I don't see any
> corresponding code being removed from anywhere else in this CL.
This is duplicating behavior in WebDataServiceBackend. I've refactored it so
that the duplication is no longer needed.

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.cc:64:
*(on_changed_callback_.get()));
On 2013/05/09 05:04:24, Ilya Sherman wrote:
> nit: I don't think the .get() is necessary.

Done.

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
File components/autofill/browser/webdata/autofill_webdata_backend_impl.h
(right):

https://codereview.chromium.org/14679005/diff/36001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend_impl.h:38: //
|web_db_backend| is used to access the WebDatabase directly for
On 2013/05/09 05:04:24, Ilya Sherman wrote:
> nit: Please update the variable name here.

Done.

https://codereview.chromium.org/14679005/diff/36001/components/webdata/common...
File components/webdata/common/web_database_service.cc (right):

https://codereview.chromium.org/14679005/diff/36001/components/webdata/common...
components/webdata/common/web_database_service.cc:97: return NULL;
On 2013/05/09 05:04:24, Ilya Sherman wrote:
> nit: These two lines don't seem like they really have any effect.

Done.

https://codereview.chromium.org/14679005/diff/36001/components/webdata/common...
File components/webdata/common/web_database_service.h (right):

https://codereview.chromium.org/14679005/diff/36001/components/webdata/common...
components/webdata/common/web_database_service.h:79: // WebDatabaseService).
On 2013/05/09 05:04:24, Ilya Sherman wrote:
> nit: What does it mean for a scoped_refptr to be owned by some other object?

Done.

[Ilya Sherman, 2013-05-10 00:08:00]

LGTM with green bots, thanks :)

https://codereview.chromium.org/14679005/diff/45001/components/webdata/common...
File components/webdata/common/web_data_service_backend.h (right):

https://codereview.chromium.org/14679005/diff/45001/components/webdata/common...
components/webdata/common/web_data_service_backend.h:78: // Task runners to run
database tasks
nit: Please end the sentence with a period.

[Cait (Slow), 2013-05-13 14:58:00]

ping: Jói -- for components/webdata/ changes

Cheers,
Cait
 
On 2013/05/10 00:08:00, Ilya Sherman wrote:
> LGTM with green bots, thanks :)
> 
>
https://codereview.chromium.org/14679005/diff/45001/components/webdata/common...
> File components/webdata/common/web_data_service_backend.h (right):
> 
>
https://codereview.chromium.org/14679005/diff/45001/components/webdata/common...
> components/webdata/common/web_data_service_backend.h:78: // Task runners to
run
> database tasks
> nit: Please end the sentence with a period.

[Jói, 2013-05-13 15:06:13]

LGTM

On Mon, May 13, 2013 at 3:58 PM,  <caitkp@chromium.org> wrote:
> ping: Jói -- for components/webdata/ changes
>
> Cheers,
> Cait
>
>
> On 2013/05/10 00:08:00, Ilya Sherman wrote:
>>
>> LGTM with green bots, thanks :)
>
>
>
>
https://codereview.chromium.org/14679005/diff/45001/components/webdata/common...
>>
>> File components/webdata/common/web_data_service_backend.h (right):
>
>
>
>
https://codereview.chromium.org/14679005/diff/45001/components/webdata/common...
>>
>> components/webdata/common/web_data_service_backend.h:78: // Task runners
>> to
>
> run
>>
>> database tasks
>> nit: Please end the sentence with a period.
>
>
>
>
> https://codereview.chromium.org/14679005/

[commit-bot: I haz the power, 2013-05-13 16:09:19]

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

[commit-bot: I haz the power, 2013-05-13 17:49:53]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-13 18:06:04]

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

[commit-bot: I haz the power, 2013-05-13 19:33:34]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_x64_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-13 19:39:53]

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

[commit-bot: I haz the power, 2013-05-13 20:31:08]

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

[commit-bot: I haz the power, 2013-05-13 21:00:22]

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

[commit-bot: I haz the power, 2013-05-14 04:58:48]

Change committed as 199904