Create a common base class for all the webdatas

Create a common base class for all the webdatas

Depends on: https://codereview.chromium.org/12871006/ and 
https://codereview.chromium.org/12491017/

Unfortunately, until we fix ownership issues, this base has to be ref-counted, which means that AutofillWebDataService has to be refcounted too in order to inherit from it. Blah.

TBR=ben@chromium.org
COLLABORATOR=joi@chromium.org
TEST=unit_tests
BUG=181277
Patch originally by caitkp@chromium.org

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

[Jói, 2013-03-18 12:36:40]

Overall looks good. One question below.

https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
chrome/browser/api/webdata/web_data_service_base.h:67:
scoped_ptr<WebDatabaseService> wdbs_;
If I'm reading this right, the next change would introduce a top-level owning
class (e.g. WebDataManager) that would own WebDatabaseService and each of the
XyzWebDataService implementations, and would change AutofillWebDataService to
use the database directly rather than just delegate to WebDataService?

[Cait (Slow), 2013-03-18 13:52:00]

https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
chrome/browser/api/webdata/web_data_service_base.h:67:
scoped_ptr<WebDatabaseService> wdbs_;
That's correct. The top level class would handle init'ing and shutting down the
db, and pass it into each XYZWebdata for them to use. The one thing i wanted to
confirm with you was that (at least for now) this means we'll have to ref-count
the webdatabaseservice, correct?

On 2013/03/18 12:36:41, Jói wrote:
> If I'm reading this right, the next change would introduce a top-level owning
> class (e.g. WebDataManager) that would own WebDatabaseService and each of the
> XyzWebDataService implementations, and would change AutofillWebDataService to
> use the database directly rather than just delegate to WebDataService?

[Jói, 2013-03-18 13:53:50]

Correct, we'd have to refcount WebDatabaseService for now.

On Mon, Mar 18, 2013 at 1:52 PM,  <caitkp@chromium.org> wrote:
> Reviewers: Jói,
>
>
>
>
https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
> File chrome/browser/api/webdata/web_data_service_base.h (right):
>
>
https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
> chrome/browser/api/webdata/web_data_service_base.h:67:
> scoped_ptr<WebDatabaseService> wdbs_;
> That's correct. The top level class would handle init'ing and shutting
> down the db, and pass it into each XYZWebdata for them to use. The one
> thing i wanted to confirm with you was that (at least for now) this
> means we'll have to ref-count the webdatabaseservice, correct?
>
>
> On 2013/03/18 12:36:41, Jói wrote:
>>
>> If I'm reading this right, the next change would introduce a top-level
>
> owning
>>
>> class (e.g. WebDataManager) that would own WebDatabaseService and each
>
> of the
>>
>> XyzWebDataService implementations, and would change
>
> AutofillWebDataService to
>>
>> use the database directly rather than just delegate to WebDataService?
>
>
> Description:
> Create a common base class for all the webdatas
>
> Depends on: https://codereview.chromium.org/12871006/ and
> https://codereview.chromium.org/12491017/
>
> Unfortunately, until we fix ownership issues, this base has to be
> ref-counted,
> which means that AutofillWebDataService has to be refcounted too in order to
> inherit from it. Blah.
>
> TEST=unit_tests
> BUG=181277
>
>
> Please review this at https://codereview.chromium.org/12851008/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M chrome/browser/api/webdata/autofill_web_data_service.h
>   M chrome/browser/api/webdata/web_data_service_base.h
>   M chrome/browser/webdata/autofill_web_data_service_impl.h
>   M chrome/browser/webdata/autofill_web_data_service_impl.cc
>   M chrome/browser/webdata/web_data_service.h
>   M chrome/browser/webdata/web_data_service.cc
>   A chrome/browser/webdata/web_data_service_base.cc
>   M chrome/browser/webdata/web_data_service_factory.cc
>   M chrome/chrome_browser.gypi
>   M components/autofill/browser/autocomplete_history_manager.h
>   M components/autofill/browser/personal_data_manager.cc
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "browser-components-watch" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to browser-components-watch+unsubscribe@chromium.org.
> To post to this group, send email to browser-components-watch@chromium.org.
> For more options, visit
> https://groups.google.com/a/chromium.org/groups/opt_out.
>
>

[Jói, 2013-03-18 14:34:48]

https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
chrome/browser/api/webdata/web_data_service_base.h:45: virtual void Init(const
base::FilePath& path);
Only one subclass is calling Init, is that how we avoid constructing two
separate WebDatabaseService objects in this change?

[Cait (Slow), 2013-03-18 14:50:15]

https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12851008/diff/1/chrome/browser/api/webdata/we...
chrome/browser/api/webdata/web_data_service_base.h:45: virtual void Init(const
base::FilePath& path);
On 2013/03/18 14:34:48, Jói wrote:
> Only one subclass is calling Init, is that how we avoid constructing two
> separate WebDatabaseService objects in this change?
Yes. For now, AutofillWebData doesn't make much use of its base class, and
instead calls into the WebDataService received on  construction. 

This does mean that AutofillWebData still needs to override some base methods
though. Adding those now.

[Cait (Slow), 2013-03-18 19:37:13]

Hi David, PTAL at c/b/webdata and c/b/api/webdata. 

This change factors some common parts of WebDataService out into a base class
which can be used by all future WebDatas. 

Thanks!

[Jói, 2013-03-18 20:02:17]

Took another look at the full change. LGTM with a couple of nits.

Cheers,
Jói

https://codereview.chromium.org/12851008/diff/6001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autofill_web_data_service_impl.h (right):

https://codereview.chromium.org/12851008/diff/6001/chrome/browser/webdata/aut...
chrome/browser/webdata/autofill_web_data_service_impl.h:41: //
WebDataServiceBase overrides (temporary until we decouple
Perhaps make this a TODO so it's easier for us to spot as something remaining to
do as we finalize WebData as a component.

https://codereview.chromium.org/12851008/diff/6001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/12851008/diff/6001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.h:303: // TODO (caitkp): We probably
don't need these anymore.
I think the standard format is to not add a space before the opening
parenthesis.

[Cait (Slow), 2013-03-18 22:06:27]

https://codereview.chromium.org/12851008/diff/6001/chrome/browser/webdata/aut...
File chrome/browser/webdata/autofill_web_data_service_impl.h (right):

https://codereview.chromium.org/12851008/diff/6001/chrome/browser/webdata/aut...
chrome/browser/webdata/autofill_web_data_service_impl.h:41: //
WebDataServiceBase overrides (temporary until we decouple
On 2013/03/18 20:02:17, Jói wrote:
> Perhaps make this a TODO so it's easier for us to spot as something remaining
to
> do as we finalize WebData as a component.

Done.

https://codereview.chromium.org/12851008/diff/6001/chrome/browser/webdata/web...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/12851008/diff/6001/chrome/browser/webdata/web...
chrome/browser/webdata/web_data_service.h:303: // TODO (caitkp): We probably
don't need these anymore.
On 2013/03/18 20:02:17, Jói wrote:
> I think the standard format is to not add a space before the opening
> parenthesis.

Done.

[dhollowa, 2013-03-18 22:38:38]

There's a lot of repetition here from your other CL.  How am I to know what's
new and what I've reviewed before?  Maybe you can rebase and upload once that
lands or something?

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_service_base.h:30: WebDataServiceBase();
This should still be virtual.  I see inheritors and virtual methods.

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

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:128: class
MockWebDataServiceWrapper : public WebDataServiceWrapper {
Please move this class out into a common place.  Violation of the DRY principle
(Don't repeat yourself).

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:147:
fake_web_data_service_ = NULL;
not needed

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:152: 
nit: delete extra line

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:153: };
DISALLOW_COPY_AND_ASSIGN

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

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:263:
web_data_service_fake_ = NULL;
not needed

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:268: 
nit: delete extra line

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:270: 
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/webdata/au...
File chrome/browser/webdata/autofill_web_data_service_impl.cc (right):

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/webdata/au...
chrome/browser/webdata/autofill_web_data_service_impl.cc:81: return
service_->CancelRequest(h);
Is |return| needed?  Shouldn't be.

[Jói, 2013-03-18 23:08:07]

I think the repetition happened when Cait rebased on trunk; not sure exactly
why. If you look at patch set #2 you'll see the real change, minus the changes
she made to address my nits.

Cait, is it possible you changed the upstream branch (git cl upstream or git
branch --set-upstream) for this change to be origin/master before you uploaded?
That would cause this behavior. To fix, one way is to rebase the first change in
your pipeline to head, then pull to the next branch and so on.

Cheers,
Jói

[Cait (Slow), 2013-03-19 00:05:28]

Apologies for all the confusion here. I ended up having to change the upstream
branch in order to get the try-jobs to run, as they kept failing to apply the
patch when the change was based off of a previous CL in the pipeline. Is there a
better way to do this, for future reference?

In the meantime, I'll set upstream back to where it was before.

On 2013/03/18 23:08:07, Jói wrote:
> I think the repetition happened when Cait rebased on trunk; not sure exactly
> why. If you look at patch set #2 you'll see the real change, minus the changes
> she made to address my nits.
> 
> Cait, is it possible you changed the upstream branch (git cl upstream or git
> branch --set-upstream) for this change to be origin/master before you
uploaded?
> That would cause this behavior. To fix, one way is to rebase the first change
in
> your pipeline to head, then pull to the next branch and so on.
> 
> Cheers,
> Jói

[dhollowa, 2013-03-19 00:29:35]

No problem.  Please take my prior comments as TODO items if they don't apply to
the CL as it stands now.  Especially the DRY violations on the Mock.

Thanks.

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_service_base.h:30: WebDataServiceBase();
Should still be virtual.

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/webdata/au...
File chrome/browser/webdata/autofill_web_data_service_impl.cc (right):

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/webdata/au...
chrome/browser/webdata/autofill_web_data_service_impl.cc:81: return
service_->CancelRequest(h);
no need for the |return|.

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.h:303: // TODO(caitkp): We probably
don't need these anymore.
Instead of a TODO, how about just removing them?

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

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_service_base.h:30: WebDataServiceBase();
On 2013/03/18 22:38:38, dhollowa wrote:
> This should still be virtual.  I see inheritors and virtual methods.

Done.

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

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:147:
fake_web_data_service_ = NULL;
On 2013/03/18 22:38:38, dhollowa wrote:
> not needed

Done.

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:152: 
On 2013/03/18 22:38:38, dhollowa wrote:
> nit: delete extra line

Done.

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:153: };
On 2013/03/18 22:38:38, dhollowa wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/webdata/au...
File chrome/browser/webdata/autofill_web_data_service_impl.cc (right):

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/webdata/au...
chrome/browser/webdata/autofill_web_data_service_impl.cc:81: return
service_->CancelRequest(h);
On 2013/03/18 22:38:38, dhollowa wrote:
> Is |return| needed?  Shouldn't be.

Done.

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/api/webdat...
File chrome/browser/api/webdata/web_data_service_base.h (right):

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/api/webdat...
chrome/browser/api/webdata/web_data_service_base.h:30: WebDataServiceBase();
On 2013/03/19 00:29:36, dhollowa wrote:
> Should still be virtual.

Done.

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/webdata/au...
File chrome/browser/webdata/autofill_web_data_service_impl.cc (right):

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/webdata/au...
chrome/browser/webdata/autofill_web_data_service_impl.cc:81: return
service_->CancelRequest(h);
On 2013/03/19 00:29:36, dhollowa wrote:
> no need for the |return|.

Done.

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/webdata/we...
File chrome/browser/webdata/web_data_service.h (right):

https://codereview.chromium.org/12851008/diff/19001/chrome/browser/webdata/we...
chrome/browser/webdata/web_data_service.h:303: // TODO(caitkp): We probably
don't need these anymore.
On 2013/03/19 00:29:36, dhollowa wrote:
> Instead of a TODO, how about just removing them?

Done.

[dhollowa, 2013-03-19 15:30:52]

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

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:128: class
MockWebDataServiceWrapper : public WebDataServiceWrapper {
On 2013/03/18 22:38:38, dhollowa wrote:
> Please move this class out into a common place.  Violation of the DRY
principle
> (Don't repeat yourself).

What's happening here?

[Jói, 2013-03-19 15:33:42]

Hi David,

I'm working on landing this change. Let me take a look if I can fix
this reasonably quickly, if not I may ask to make it a TODO and do it
at the back of our current (fairly long) pipeline of changes.

Cheers,
Jói


On Tue, Mar 19, 2013 at 3:30 PM,  <dhollowa@chromium.org> wrote:
>
>
https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
> File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
> (right):
>
>
https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
> chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:128:
> class MockWebDataServiceWrapper : public WebDataServiceWrapper {
> On 2013/03/18 22:38:38, dhollowa wrote:
>>
>> Please move this class out into a common place.  Violation of the DRY
>
> principle
>>
>> (Don't repeat yourself).
>
>
> What's happening here?
>
> https://codereview.chromium.org/12851008/

[Jói, 2013-03-19 15:34:09]

(in case of confusion, Cait and I are using the COLLABORATOR= feature
in the change description to allow either of us to upload new patches
and to commit the change)

On Tue, Mar 19, 2013 at 3:32 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> Hi David,
>
> I'm working on landing this change. Let me take a look if I can fix
> this reasonably quickly, if not I may ask to make it a TODO and do it
> at the back of our current (fairly long) pipeline of changes.
>
> Cheers,
> Jói
>
>
> On Tue, Mar 19, 2013 at 3:30 PM,  <dhollowa@chromium.org> wrote:
>>
>>
https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
>> File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
>> (right):
>>
>>
https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
>> chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:128:
>> class MockWebDataServiceWrapper : public WebDataServiceWrapper {
>> On 2013/03/18 22:38:38, dhollowa wrote:
>>>
>>> Please move this class out into a common place.  Violation of the DRY
>>
>> principle
>>>
>>> (Don't repeat yourself).
>>
>>
>> What's happening here?
>>
>> https://codereview.chromium.org/12851008/

[Jói, 2013-03-19 15:37:47]

Looks easy enough, I'll upload a new version shortly.

Cheers,
Jói


On Tue, Mar 19, 2013 at 3:33 PM, Jói Sigurðsson <joi@chromium.org> wrote:
> (in case of confusion, Cait and I are using the COLLABORATOR= feature
> in the change description to allow either of us to upload new patches
> and to commit the change)
>
> On Tue, Mar 19, 2013 at 3:32 PM, Jói Sigurðsson <joi@chromium.org> wrote:
>> Hi David,
>>
>> I'm working on landing this change. Let me take a look if I can fix
>> this reasonably quickly, if not I may ask to make it a TODO and do it
>> at the back of our current (fairly long) pipeline of changes.
>>
>> Cheers,
>> Jói
>>
>>
>> On Tue, Mar 19, 2013 at 3:30 PM,  <dhollowa@chromium.org> wrote:
>>>
>>>
https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
>>> File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
>>> (right):
>>>
>>>
https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
>>> chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:128:
>>> class MockWebDataServiceWrapper : public WebDataServiceWrapper {
>>> On 2013/03/18 22:38:38, dhollowa wrote:
>>>>
>>>> Please move this class out into a common place.  Violation of the DRY
>>>
>>> principle
>>>>
>>>> (Don't repeat yourself).
>>>
>>>
>>> What's happening here?
>>>
>>> https://codereview.chromium.org/12851008/

[Jói, 2013-03-19 16:32:18]

David, DRY violations are fixed in patch set 7.  You can use the diff from patch
set 6 to patch set 7 if you want to see just what changed.

Cheers,
Jói

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

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:128: class
MockWebDataServiceWrapper : public WebDataServiceWrapper {
On 2013/03/18 22:38:38, dhollowa wrote:
> Please move this class out into a common place.  Violation of the DRY
principle
> (Don't repeat yourself).

Done.

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

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:263:
web_data_service_fake_ = NULL;
On 2013/03/18 22:38:38, dhollowa wrote:
> not needed

Done.

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:268: 
On 2013/03/18 22:38:38, dhollowa wrote:
> nit: delete extra line

Done.

https://codereview.chromium.org/12851008/diff/11001/chrome/browser/sync/profi...
chrome/browser/sync/profile_sync_service_autofill_unittest.cc:270: 
On 2013/03/18 22:38:38, dhollowa wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[dhollowa, 2013-03-19 17:28:03]

LGTM.  Thanks!

[commit-bot: I haz the power, 2013-03-20 10:16:05]

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

[commit-bot: I haz the power, 2013-03-20 11:15:17]

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&...

[Jói, 2013-03-20 11:59:49]

Committed patchset #9 manually as r189245 (presubmit successful).