bookmarks: Convert ProfileImpl to use BaseBookmarkModelObserver.

bookmarks: Convert ProfileImpl to use BaseBookmarkModelObserver.

That way we can remove the usage of chrome::NOTIFICATION_BOOKMARK_MODEL_LOADED,
and just register it using BookmarkModel::AddObserver/RemoveObserver API.

BUG=144783
R=sky@chromium.org,sail@chromium.org


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

[sky, 2013-01-25 23:38:50]

I think I would move this off to a standalone object, but I'l leave that
decision to Sailesh since he's the owner here.

[sail, 2013-01-26 00:15:11]

Hi Thiago, what's the motivation of using a model observer instead of a
notification? Reading bug 144783 didn't seem to help.

As for using a standalone object, this would be great if the object would live
in the sync code base. Making it a member of ProfileImpl wouldn't work though
because you're not allowed to add any new member variables to ProfileImpl.

[sky, 2013-01-26 00:17:26]

On Fri, Jan 25, 2013 at 4:15 PM,  <sail@chromium.org> wrote:
> Hi Thiago, what's the motivation of using a model observer instead of a
> notification? Reading bug 144783 didn't seem to help.
>
> As for using a standalone object, this would be great if the object would
> live
> in the sync code base. Making it a member of ProfileImpl wouldn't work
> though
> because you're not allowed to add any new member variables to ProfileImpl.

It need not be a member of ProfileImpl, it can delete itself as appropriate.

  -Scott

[sail, 2013-01-26 00:23:28]

On 2013/01/26 00:17:26, sky wrote:
> On Fri, Jan 25, 2013 at 4:15 PM,  <mailto:sail@chromium.org> wrote:
> > Hi Thiago, what's the motivation of using a model observer instead of a
> > notification? Reading bug 144783 didn't seem to help.
> >
> > As for using a standalone object, this would be great if the object would
> > live
> > in the sync code base. Making it a member of ProfileImpl wouldn't work
> > though
> > because you're not allowed to add any new member variables to ProfileImpl.
> 
> It need not be a member of ProfileImpl, it can delete itself as appropriate.

Are there any situations where the bookmark bar model doesn't load? For example
in a test? Otherwise this sounds good to me.

>   -Scott

[tfarina, 2013-01-26 01:12:22]

For the motivation it is:

- For BookmarkModel to not have two ways of firing the same event (one
through ObserverList, the other through content::NotificationService)
- For the clients/consumers to not have two ways of listening (registering)
to this event.
- To not depend on content::NotificationService.
- see crbug.com/170921.

This is also a consensus between Joi, John and Ben, that
content::NotificationService is evil and should be used less not more.

So I'm moving bookmarks system towards that goal.

I think my previous patches in crbug.com/144783 shows that. If you do a
quick scan you will see ;)

And Pawel's work is also reinforcing what I'm doing here.

[sail, 2013-01-26 01:16:58]

On 2013/01/26 01:12:22, tfarina wrote:
> For the motivation it is:
> 
> - For BookmarkModel to not have two ways of firing the same event (one
> through ObserverList, the other through content::NotificationService)
> - For the clients/consumers to not have two ways of listening (registering)
> to this event.
> - To not depend on content::NotificationService.
> - see crbug.com/170921.
> 
> This is also a consensus between Joi, John and Ben, that
> content::NotificationService is evil and should be used less not more.
> 
> So I'm moving bookmarks system towards that goal.
> 
> I think my previous patches in crbug.com/144783 shows that. If you do a
> quick scan you will see ;)
> 
> And Pawel's work is also reinforcing what I'm doing here.

Makes sense. LGTM
Up to you if you want to move this into a separate object. I'm not 100%
convinced it's possible.

[tfarina, 2013-01-26 02:45:12]

Sail, mind take another look at patch set 2?

I implemented Scott's suggestion and moved it to a separate object.

[sail, 2013-01-26 03:05:09]

LGTM

[tfarina, 2013-01-26 13:20:48]

For the motivation it is:

- don't have to ways to register for that event.
- do not depend on content::NotificationService.
- see bug 170921.

There is a consensul between Jo, John, Ben, that this Notifcation API is
evil and should be used less.

So I'm moving towards this in the bookmarks system.

Hope that clarifies the things :)

Thanks,



-- 
Thiago

[tfarina, 2013-01-26 14:04:27]

Oh, please, ignore my last message. It was a draft I had on my iPhone.
It was sent today when I opened the Gmail application. :/

-- 
Thiago

[commit-bot: I haz the power, 2013-01-28 02:55:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tfarina@chromium.org/12077006/9001

[commit-bot: I haz the power, 2013-01-28 04:39:05]

Change committed as 179101

[sky, 2013-01-28 20:28:56]

https://codereview.chromium.org/12077006/diff/9001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_impl.cc (right):

https://codereview.chromium.org/12077006/diff/9001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_impl.cc:240: explicit
BookmarkModelLoadedObserver(Profile* profile);
You should also delete this if you get BookmarkModelBeingDeleted.