Use Observer to notify of WebDB load instead of callbacks

Use Observer to notify of WebDB load instead of callbacks. We were already partially doing this for Autofill, this CL just makes it so that all WebDB clients can use an observer interface, instead of passing callbacks.

Also:
-Split WebDatabaseService and WebDataServiceBackend into separate files

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

[Cait (Slow), 2013-04-16 16:54:50]

Kai and Joi,

PTAL and let me know what you think of this approach. I refactored the
"WebDatabaseLoaded()" method out of The AutofillWebDataObserverOnUIThread, and
into an observer class in Webdata/common (WebDatabaseObserver).

This allows us to use a unified approach for notifying clients of the DB
(including WebDataService) that the DB has loaded, and avoid passing (and
binding) callbacks all over the place.

I wish there was a way to avoid having 3 different observer types, but since 2
are Autofill-specific, and one is meant to serve all WebData clients, I'm not
sure if this is feasible.

Thanks,
Cait

[Jói, 2013-04-16 19:40:39]

I'm traveling right now, probably won't be able to look until
tomorrow, but if you feel that switching to observer interfaces
instead of callback objects makes things simpler, I think I'll be fine
with that.

Cheers,
Jói


On Tue, Apr 16, 2013 at 5:54 PM,  <caitkp@chromium.org> wrote:
> Reviewers: kaiwang, Jói,
>
> Message:
> Kai and Joi,
>
> PTAL and let me know what you think of this approach. I refactored the
> "WebDatabaseLoaded()" method out of The AutofillWebDataObserverOnUIThread,
> and
> into an observer class in Webdata/common (WebDatabaseObserver).
>
> This allows us to use a unified approach for notifying clients of the DB
> (including WebDataService) that the DB has loaded, and avoid passing (and
> binding) callbacks all over the place.
>
> I wish there was a way to avoid having 3 different observer types, but since
> 2
> are Autofill-specific, and one is meant to serve all WebData clients, I'm
> not
> sure if this is feasible.
>
> Thanks,
> Cait
>
> Description:
> Use Observer to notify of WebDB load instead of callbacks. We were already
> partially doing this for Autofill, this CL just makes it so that all WebDB
> clients can use an observer interface, instead of passing callbacks.
>
> Also:
> -Split WebDatabaseService and WebDataServiceBackend into separate files
>
> BUG=230920
>
> Please review this at https://codereview.chromium.org/14103021/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M chrome/browser/password_manager/password_store_win_unittest.cc
>   M chrome/browser/sync/glue/autofill_data_type_controller.h
>   M chrome/browser/sync/glue/autofill_data_type_controller.cc
>   M chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc
>   M chrome/browser/sync/glue/autofill_profile_data_type_controller.h
>   M chrome/browser/sync/glue/autofill_profile_data_type_controller.cc
>   M chrome/browser/webdata/web_data_service_factory.cc
>   M components/autofill/browser/webdata/autofill_webdata_service.h
>   M components/autofill/browser/webdata/autofill_webdata_service.cc
>   M components/autofill/browser/webdata/autofill_webdata_service_observer.h
>   M components/autofill/browser/webdata/web_data_service_unittest.cc
>   M components/webdata.gypi
>   A components/webdata/common/web_data_service_backend.h
>   A components/webdata/common/web_data_service_backend.cc
>   M components/webdata/common/web_data_service_base.h
>   M components/webdata/common/web_data_service_base.cc
>   A components/webdata/common/web_database_observer.h
>   M components/webdata/common/web_database_service.h
>   M components/webdata/common/web_database_service.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-04-17 10:52:03]

LGTM with a few nits.

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

https://codereview.chromium.org/14103021/diff/10001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:75: virtual
void AddDBObserver(WebDatabaseObserver* observer) OVERRIDE {
Might be worth a comment saying this implementation is the simplest thing that
works for the unit test, although in violation of the contract for AddDBObserver
(which is that multiple observers are allowed).

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

https://codereview.chromium.org/14103021/diff/10001/components/webdata/common...
components/webdata/common/web_data_service_backend.h:28:
////////////////////////////////////////////////////////////////////////////////
Suggest getting rid of this weird formatting of the comment.

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

https://codereview.chromium.org/14103021/diff/10001/components/webdata/common...
components/webdata/common/web_database_service.h:119: // All vended weak
pointers are invalidated in Cleanup().
Cleanup -> ShutdownDatabase

[kaiwang, 2013-04-17 23:53:50]

LGTM

Sorry for my late response

[Cait (Slow), 2013-04-18 15:07:47]

dhollowa@: PTAL at c/b/webdata/, components/webdata/, and
components/autofill/browser/webdata
akalin@ PTAL at c/b/sync/glue

Thanks!

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

https://codereview.chromium.org/14103021/diff/10001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:75: virtual
void AddDBObserver(WebDatabaseObserver* observer) OVERRIDE {
On 2013/04/17 10:52:04, Jói wrote:
> Might be worth a comment saying this implementation is the simplest thing that
> works for the unit test, although in violation of the contract for
AddDBObserver
> (which is that multiple observers are allowed).

Done.

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

https://codereview.chromium.org/14103021/diff/10001/components/webdata/common...
components/webdata/common/web_data_service_backend.h:28:
////////////////////////////////////////////////////////////////////////////////
On 2013/04/17 10:52:04, Jói wrote:
> Suggest getting rid of this weird formatting of the comment.

Done.

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

https://codereview.chromium.org/14103021/diff/10001/components/webdata/common...
components/webdata/common/web_database_service.h:119: // All vended weak
pointers are invalidated in Cleanup().
On 2013/04/17 10:52:04, Jói wrote:
> Cleanup -> ShutdownDatabase

Done.

[dhollowa, 2013-04-18 16:21:17]

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

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:79: virtual
void AddDBObserver(WebDatabaseObserver* observer) OVERRIDE {
How about a DCHECK(!observer_) to enforce this.

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:84: if
(observer_ == observer) {
DCHECK_EQ (observer_, observer)

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/autofill_profile_data_type_controller.cc (right):

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:44: if
(web_data_service_) {
nit: no {} needed

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:104: if
(web_data_service_) {
nit: no {} needed

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
File components/webdata/common/web_data_service_base.cc (right):

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
components/webdata/common/web_data_service_base.cc:42: if
(!profile_error_callback_.is_null()) {
nit: no {} needed

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
File components/webdata/common/web_database_observer.h (right):

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
components/webdata/common/web_database_observer.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: "Copyright 2013"

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
components/webdata/common/web_database_observer.h:16: virtual void
WebDatabaseLoaded() {};
Could you explain why these are not "= 0"?  I'm ok with it, but I would have
thought it desirable to have compiler support alerting you to classes that say
they're an observer but do nothing.

[Cait (Slow), 2013-04-19 19:06:21]

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

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:79: virtual
void AddDBObserver(WebDatabaseObserver* observer) OVERRIDE {
On 2013/04/18 16:21:17, dhollowa wrote:
> How about a DCHECK(!observer_) to enforce this.

Done.

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:84: if
(observer_ == observer) {
On 2013/04/18 16:21:17, dhollowa wrote:
> DCHECK_EQ (observer_, observer)

Done.

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/autofill_profile_data_type_controller.cc (right):

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:44: if
(web_data_service_) {
On 2013/04/18 16:21:17, dhollowa wrote:
> nit: no {} needed

Done.

https://codereview.chromium.org/14103021/diff/20001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/autofill_profile_data_type_controller.cc:104: if
(web_data_service_) {
On 2013/04/18 16:21:17, dhollowa wrote:
> nit: no {} needed

Done.

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
File components/webdata/common/web_data_service_base.cc (right):

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
components/webdata/common/web_data_service_base.cc:42: if
(!profile_error_callback_.is_null()) {
On 2013/04/18 16:21:17, dhollowa wrote:
> nit: no {} needed

Done.

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
File components/webdata/common/web_database_observer.h (right):

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
components/webdata/common/web_database_observer.h:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/04/18 16:21:17, dhollowa wrote:
> nit: "Copyright 2013"

Done.

https://codereview.chromium.org/14103021/diff/20001/components/webdata/common...
components/webdata/common/web_database_observer.h:16: virtual void
WebDatabaseLoaded() {};
On 2013/04/18 16:21:17, dhollowa wrote:
> Could you explain why these are not "= 0"?  I'm ok with it, but I would have
> thought it desirable to have compiler support alerting you to classes that say
> they're an observer but do nothing.
Several observers want to be told that the DB loaded successfully but don't care
if it failed. We could make the WebDatabaseLoaded() pure virtual, but keep the
WebDatabaseLoadFailed() as is.

[dhollowa, 2013-04-19 19:30:58]

lgtm

[Cait (Slow), 2013-04-24 15:42:15]

-akalin, +atwilson for sync changes.

Andrew, PTAL. This change cleans up the DB load notifications for WebData.

[commit-bot: I haz the power, 2013-04-26 23:05:52]

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

[commit-bot: I haz the power, 2013-04-26 23:08:25]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-04-29 14:59:16]

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

[commit-bot: I haz the power, 2013-04-29 17:55:52]

Change committed as 197078