Move AutofillWebData DB methods into separate class

Move AutofillWebData DB methods into separate class

SyncableService code will move in a later CL as it will require a bit more refactoring.

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

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

[Cait (Slow), 2013-04-29 19:36:12]

Ilya, PTAL. This is mostly a mechanical change, which moves all database work
done by AutofillWebData into a separate backend class. This is a necessary step
towards cleaning up the ownership model of the AutofillWebDataService.

BTW, are you the correct person to send these sorts of changes to? (I had
previously been sending them to dhollowa@, but he is no longer listed as an
Autofill OWNER).

Thanks,
Cait

[Ilya Sherman, 2013-04-30 00:14:08]

On 2013/04/29 19:36:12, caitkp wrote:
> BTW, are you the correct person to send these sorts of changes to?
> (I had previously been sending them to dhollowa@, but he is no
> longer listed as an Autofill OWNER).

Yep, I can review these sorts of changes.  David is in the process of
transitioning off the Chrome team (though he's still at Google).

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager_unittest.cc:118:
db_thread_.Stop();
Hmm, can you just use content::RunAllPendingInMessageLoop() from
//contents/public/test/test_utils.h?

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

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:22: namespace
autofill {
nit: Please leave a blank line after this one.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:316: }
nit: "  // namespace autofill"

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

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:20: class
AutofillWebDataBackend
Please add documentation describing the purpose of this class.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:22:
content::BrowserThread::DeleteOnDBThread> {
Why is this class reference counted, rather than simply always accessed on the
DB thread?

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:26:
WebDatabase::State AddFormElementsImpl(
nit: Could we drop the "Impl" suffix for all of these methods?

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:27: const
std::vector<FormFieldData>& fields, WebDatabase* db);
nit: Please leave a blank line after each method declaration, and please add
documentation for the methods.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:27: const
std::vector<FormFieldData>& fields, WebDatabase* db);
nit: If the entire method declaration doesn't fit on a single line, please wrap
things so that there is exactly one parameter per line.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:65: // We have to
friend RCTS<> so WIN shared-lib build is happy (crbug/112250).
nit: Please prepend "http://" to the URL so that it is hyperlinked.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:81: #endif  //
COMPONENTS_AUTOFILL_BROWSER_WEBDATA_AUTOFILL_WEBDATA_BACKEND_H_
Any chance you could convince the source control tools that this file is
branched from autofill_webdata_service.h rather than it being a brand new file?

[Cait (Slow), 2013-04-30 22:24:39]

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
File components/autofill/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/autocomplete_history_manager_unittest.cc:118:
db_thread_.Stop();
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> Hmm, can you just use content::RunAllPendingInMessageLoop() from
> //contents/public/test/test_utils.h?

Done.

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

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:22: namespace
autofill {
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.cc:316: }
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> nit: "  // namespace autofill"

Done.

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

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:20: class
AutofillWebDataBackend
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> Please add documentation describing the purpose of this class.

Done.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:22:
content::BrowserThread::DeleteOnDBThread> {
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> Why is this class reference counted, rather than simply always accessed on the
> DB thread?
It needs to be ref-counted as AutofillWebDataService posts tasks to it
asynchronously (from the UI thread).

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:26:
WebDatabase::State AddFormElementsImpl(
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> nit: Could we drop the "Impl" suffix for all of these methods?

Done.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:27: const
std::vector<FormFieldData>& fields, WebDatabase* db);
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> nit: Please leave a blank line after each method declaration, and please add
> documentation for the methods.

Done.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:27: const
std::vector<FormFieldData>& fields, WebDatabase* db);
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> nit: If the entire method declaration doesn't fit on a single line, please
wrap
> things so that there is exactly one parameter per line.

Done.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:65: // We have to
friend RCTS<> so WIN shared-lib build is happy (crbug/112250).
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> nit: Please prepend "http://" to the URL so that it is hyperlinked.

Done.

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:81: #endif  //
COMPONENTS_AUTOFILL_BROWSER_WEBDATA_AUTOFILL_WEBDATA_BACKEND_H_
On 2013/04/30 00:14:08, Ilya Sherman wrote:
> Any chance you could convince the source control tools that this file is
> branched from autofill_webdata_service.h rather than it being a brand new
file?

Done.

[Ilya Sherman, 2013-04-30 23:52:10]

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

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:22:
content::BrowserThread::DeleteOnDBThread> {
On 2013/04/30 22:24:40, caitkp wrote:
> On 2013/04/30 00:14:08, Ilya Sherman wrote:
> > Why is this class reference counted, rather than simply always accessed on
the
> > DB thread?
> It needs to be ref-counted as AutofillWebDataService posts tasks to it
> asynchronously (from the UI thread).

Seems like the AutofillWebDataService could just hold a WeakPtr to it; or if it
owns the lifetime, it could post asynchronous tasks to the DB thread to create
and destroy this object.

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

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.cc:317: } //
namespace autofill
nit: Please add one more space before the "//"

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

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:34: WebDatabase*
db);
nit: Wrap this as

  WebDatabase::State AddFormElements(const std::vector<FormFieldData>& fields,
                                     WebDatabase* db);

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:37: // form input
fields named |name|.
nit: Re-wrap to 80-col?

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:57: WebDatabase*
db);
nit: Wrap this as

  WebDatabase::State RemoveFormValueForElementName(const base::string16& name,
                                                   const base::string16& value,
                                                   WebDatabase* db);

Likewise for several of the methods below.

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

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.cc:142:
autofill_backend_, guid));
nit: Looks like this fits on the previous line

[Cait (Slow), 2013-05-01 23:14:19]

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

https://codereview.chromium.org/14348036/diff/3001/components/autofill/browse...
components/autofill/browser/webdata/autofill_webdata_backend.h:22:
content::BrowserThread::DeleteOnDBThread> {
On 2013/04/30 23:52:11, Ilya Sherman wrote:
> On 2013/04/30 22:24:40, caitkp wrote:
> > On 2013/04/30 00:14:08, Ilya Sherman wrote:
> > > Why is this class reference counted, rather than simply always accessed on
> the
> > > DB thread?
> > It needs to be ref-counted as AutofillWebDataService posts tasks to it
> > asynchronously (from the UI thread).
> 
> Seems like the AutofillWebDataService could just hold a WeakPtr to it; or if
it
> owns the lifetime, it could post asynchronous tasks to the DB thread to create
> and destroy this object.

The async tasks have non-void return types (either the state of the DB, or the
data being requested), so a WeakPtr won't work here. 

The other option is to let AutofillWebDataService own the lifetime, any not
execute any DB requests that were made to AutofillWebDataService after
ShutdownOnUIThread() had been called (since the backend could disappear anytime
thereafter). Not sure if this would break any of the  clients or not though...

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

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.cc:317: } //
namespace autofill
On 2013/04/30 23:52:11, Ilya Sherman wrote:
> nit: Please add one more space before the "//"

Done.

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

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:34: WebDatabase*
db);
On 2013/04/30 23:52:11, Ilya Sherman wrote:
> nit: Wrap this as
> 
>   WebDatabase::State AddFormElements(const std::vector<FormFieldData>& fields,
>                                      WebDatabase* db);

Done.

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:37: // form input
fields named |name|.
On 2013/04/30 23:52:11, Ilya Sherman wrote:
> nit: Re-wrap to 80-col?

Done.

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:57: WebDatabase*
db);
On 2013/04/30 23:52:11, Ilya Sherman wrote:
> nit: Wrap this as
> 
>   WebDatabase::State RemoveFormValueForElementName(const base::string16& name,
>                                                    const base::string16&
value,
>                                                    WebDatabase* db);
> 
> Likewise for several of the methods below.

Done.

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_backend.h:57: WebDatabase*
db);
On 2013/04/30 23:52:11, Ilya Sherman wrote:
> nit: Wrap this as
> 
>   WebDatabase::State RemoveFormValueForElementName(const base::string16& name,
>                                                    const base::string16&
value,
>                                                    WebDatabase* db);
> 
> Likewise for several of the methods below.

Done.

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

https://codereview.chromium.org/14348036/diff/10001/components/autofill/brows...
components/autofill/browser/webdata/autofill_webdata_service.cc:142:
autofill_backend_, guid));
On 2013/04/30 23:52:11, Ilya Sherman wrote:
> nit: Looks like this fits on the previous line

Done.

[Ilya Sherman, 2013-05-02 06:45:30]

Ok, since it sounds like untangling the reference counting will require a fair
bit of yak shaving, this LGTM for now.  Could you please file a bug to make the
backend no longer be reference counted?  Thanks :)

[Cait (Slow), 2013-05-02 16:05:09]

Filed http://crbug.com/237535.

Thanks!

[commit-bot: I haz the power, 2013-05-02 16:05:51]

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

[commit-bot: I haz the power, 2013-05-02 16:10:53]

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

[commit-bot: I haz the power, 2013-05-02 19:11:10]

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

[commit-bot: I haz the power, 2013-05-02 20:57:40]

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

[commit-bot: I haz the power, 2013-05-02 22:31:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
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-03 00:21:09]

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

[commit-bot: I haz the power, 2013-05-03 10:56:21]

Change committed as 198080