Replace WebDatabaseObserver with callbacks

Replace WebDatabaseObserver with callbacks

TBR=tim@chromium.org (sync/glue changes)
BUG=230920
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204894

[Cait (Slow), 2013-06-04 00:33:11]

tim: PTAL at chrome/browser/sync/glue changes.
joi: PTAL at components/webdata/common changes.

FYI: the purpose of this change is to make it so that AutofillDataTypeController
and AutofillProfileDataTypeController don't need to hold a ref to the
WebDataService, making lifecycle management as bit simpler.

[Jói, 2013-06-04 22:17:25]

Mostly looks good, see comments below.

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
File components/webdata/common/web_data_service_base.h (right):

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_data_service_base.h:73: virtual void
RegisterDBLoadedCallback(
Can you register one or more? Can you unregister? Need to document.

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
File components/webdata/common/web_database_service.h (right):

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_database_service.h:128: // Types for managing DB
laoding callbacks.
laoding -> loading

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_database_service.h:130: typedef
std::vector<DBLoadedCallback> PendingCallbacks;
For consistency, suggest renaming to LoadedCallbacks.

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_database_service.h:136: PendingCallbacks
pending_callbacks_;
variable name too

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

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
File components/webdata/common/web_data_service_base.h (right):

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_data_service_base.h:73: virtual void
RegisterDBLoadedCallback(
On 2013/06/04 22:17:25, Jói wrote:
> Can you register one or more? Can you unregister? Need to document.

Done.

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
File components/webdata/common/web_database_service.cc (right):

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_database_service.cc:148:
pending_callbacks_.push_back(callback);
I'm wondering if I should handle callbacks added post-DB-load (i.e. call them
immediately). Doing so would require storing some info about the DB state (did
we try to load it and fail? why did it fail? etc) in this class which is already
stored in the backend, and this seems a bit redundant. 

Maybe it is sufficient to document that callbacks added after the loading is
finished won't get called (esp. since there is an IsDBLoaded() method)?

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
File components/webdata/common/web_database_service.h (right):

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_database_service.h:128: // Types for managing DB
laoding callbacks.
On 2013/06/04 22:17:25, Jói wrote:
> laoding -> loading

Done.

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_database_service.h:130: typedef
std::vector<DBLoadedCallback> PendingCallbacks;
On 2013/06/04 22:17:25, Jói wrote:
> For consistency, suggest renaming to LoadedCallbacks.

Done.

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_database_service.h:136: PendingCallbacks
pending_callbacks_;
On 2013/06/04 22:17:25, Jói wrote:
> variable name too

Done.

[Jói, 2013-06-06 11:37:17]

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
File components/webdata/common/web_database_service.cc (right):

https://codereview.chromium.org/15927029/diff/1/components/webdata/common/web...
components/webdata/common/web_database_service.cc:148:
pending_callbacks_.push_back(callback);
On 2013/06/05 19:14:48, caitkp wrote:
> I'm wondering if I should handle callbacks added post-DB-load (i.e. call them
> immediately). Doing so would require storing some info about the DB state (did
> we try to load it and fail? why did it fail? etc) in this class which is
already
> stored in the backend, and this seems a bit redundant. 
> 
> Maybe it is sufficient to document that callbacks added after the loading is
> finished won't get called (esp. since there is an IsDBLoaded() method)? 

I think just documenting is sufficient.

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

https://codereview.chromium.org/15927029/diff/10001/components/webdata/common...
components/webdata/common/web_data_service_base.h:76: // Note: if the database
load is already complete, then the callback will NOT
"will NOT be stored or called." (right?)

Here and the other header with similar documentation.

[Cait (Slow), 2013-06-06 15:57:34]

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

https://codereview.chromium.org/15927029/diff/10001/components/webdata/common...
components/webdata/common/web_data_service_base.h:76: // Note: if the database
load is already complete, then the callback will NOT
On 2013/06/06 11:37:17, Jói wrote:
> "will NOT be stored or called." (right?)
> 
> Here and the other header with similar documentation.

Done.

[Jói, 2013-06-06 22:11:27]

LGTM

[commit-bot: I haz the power, 2013-06-07 16:02:31]

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

[commit-bot: I haz the power, 2013-06-07 19:23:49]

Change committed as 204894

[tfarina, 2013-06-26 03:53:51]

https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
File components/webdata/common/web_database_service.h (right):

https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
components/webdata/common/web_database_service.h:101: void
RegisterDBLoadedCallback(const base::Callback<void(void)>& callback);
base::Callback<void(void)> is base::Closure.

https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
components/webdata/common/web_database_service.h:111: bool db_loaded() { return
db_loaded_; };
minorminornit: const

https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
components/webdata/common/web_database_service.h:137: typedef
base::Callback<void(void)> DBLoadedCallback;
if we move the callback typedefs to the public section, then we can reuse them
in RegisterDBLoadedCallback/RegisterDBErrorCallback and here as well.

https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
components/webdata/common/web_database_service.h:151: };
minornit: DISALLOW_COPY_AND_ASSING

[Jói, 2013-06-26 13:22:31]

FYI, tfarina@ has a change pending to fix these nits,
https://codereview.chromium.org/17760007/.

On Wed, Jun 26, 2013 at 3:53 AM,  <tfarina@chromium.org> wrote:
>
>
https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
> File components/webdata/common/web_database_service.h (right):
>
>
https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
> components/webdata/common/web_database_service.h:101: void
> RegisterDBLoadedCallback(const base::Callback<void(void)>& callback);
> base::Callback<void(void)> is base::Closure.
>
>
https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
> components/webdata/common/web_database_service.h:111: bool db_loaded() {
> return db_loaded_; };
> minorminornit: const
>
>
https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
> components/webdata/common/web_database_service.h:137: typedef
> base::Callback<void(void)> DBLoadedCallback;
> if we move the callback typedefs to the public section, then we can
> reuse them in RegisterDBLoadedCallback/RegisterDBErrorCallback and here
> as well.
>
>
https://chromiumcodereview.appspot.com/15927029/diff/19001/components/webdata...
> components/webdata/common/web_database_service.h:151: };
> minornit: DISALLOW_COPY_AND_ASSING
>
> https://chromiumcodereview.appspot.com/15927029/