Introduce BookmarkClient interface to abstract embedder

Introduce BookmarkClient interface to abstract embedder

The BookmarkClient interface abstracts the embedder. Once bookmarks is
componentized, the code will have to go through BookmarkClient methods
instead of accessing methods from chrome/browser or chrome/content (as
they are not accessible from iOS).

Wrapped methods from chrome/browser/:
- FaviconService::GetFaviconImageForURL
- HistoryService::InMemoryDatabase

BookmarkClient also wraps content::RecordAction, as it is a simple
wrapper around base::RecordAction and can easily be reimplemented in the
client without dependency on the content/ API.

Port all the client code to pass the BookmarkClient interface to the
method that need it (mostly bookmark_utils::AddIfNotBookmarked and
functions in bookmark_stats.cc, the rest are internal to the component).

BUG=364433
R=sky@chromium.org,tfarina@chromium.org,droger@chromium.org

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

[sdefresne, 2014-04-18 14:14:53]

Please have a look. Please note that I used clang-format on the code.

[sky, 2014-04-18 14:21:56]

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
File components/bookmarks/core/browser/bookmark_client.h (right):

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:33: virtual
~BookmarkClient() {};
nit: no ';'

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:54: typedef std::set<const
BookmarkNode*> NodeSet;
I would prefer to keep the typedefs at the top of the section (like the style
guide says).

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:60: virtual void
ExtractBookmarkNodePairs(
How about a better name for this, maybe GetTypedCountForNodes?

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:68: // Post a task in the UI
thread.
components can't depend upon the ability to get a ui thread?

[sdefresne, 2014-04-18 15:05:32]

Please take another look.

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
File components/bookmarks/core/browser/bookmark_client.h (right):

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:33: virtual
~BookmarkClient() {};
On 2014/04/18 14:21:56, sky wrote:
> nit: no ';'

Done.

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:54: typedef std::set<const
BookmarkNode*> NodeSet;
On 2014/04/18 14:21:56, sky wrote:
> I would prefer to keep the typedefs at the top of the section (like the style
> guide says).

Done.

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:60: virtual void
ExtractBookmarkNodePairs(
On 2014/04/18 14:21:56, sky wrote:
> How about a better name for this, maybe GetTypedCountForNodes?

Done.

https://codereview.chromium.org/242693003/diff/1/components/bookmarks/core/br...
components/bookmarks/core/browser/bookmark_client.h:68: // Post a task in the UI
thread.
On 2014/04/18 14:21:56, sky wrote:
> components can't depend upon the ability to get a ui thread?

Done.

[sky, 2014-04-18 17:05:04]

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index.cc (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.cc:139:
client->GetTypedCountForNodes(nodes, node_typed_counts);
If the intention is that some clients don't support GetTypedCountForNodes should
we have a SupportsTypedCountForNodes on the client interface?

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.cc (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.cc:969:
base::CancelableTaskTracker::TaskId BookmarkModel::GetFaviconImageForURL(
Style guide says declaration order and definition order should match. So, move
these to match position in header.

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:54: public BookmarkClient,
I'm confused as to why you're making BookmarkModel implement BookmarkClient? If
only temporary, I would prefer having it on a separate object that BookmarkModel
owns. I like this since you internally have BookmarkModel::LoadFavicon calling
GetFaviconImageForURL(), which should really by going through BookmarkClient,
and not BookmarkModel.

I'm fine with this code existing in bookmark_model.h/.cc for now, but would
prefer we start down the long term path that we want to get to. Unless I'm
missing something.

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_stats.h (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_stats.h:5: #ifndef
CHROME_BROWSER_BOOKMARKS_BOOKMARK_STATS_H_
Are you modifying this file in expectation of moving to components? I think it's
chrome specific and shouldn't need to move.

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_storage.cc (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_storage.cc:53: base::SequencedTaskRunner*
task_runner) {
Shouldn't this take a scoped_refptr?

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_storage.h (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_storage.h:118: const base::FilePath&
profile_path,
Document profile_path here and task_runner below.

https://codereview.chromium.org/242693003/diff/20001/components/bookmarks/cor...
File components/bookmarks/core/browser/bookmark_client.h (right):

https://codereview.chromium.org/242693003/diff/20001/components/bookmarks/cor...
components/bookmarks/core/browser/bookmark_client.h:35: // Callback for
GetFaviconImageForURL().
Actually, style guide says enums are before constructors/destructor.

[tfarina, 2014-04-18 19:35:08]

Sylvain, how Client relates to Service? You also plan to add a Driver, no?

If we could have a bigger picture of how all these classes will work together,
that would help me. May be a better and more detailed explanation in the
componentisation doc. Usually, users had to deal with only BookmarkModel,
history was the only one using BookmarkService. I'm afraid we are breaking this
into too many pieces.

[sdefresne, 2014-04-18 22:25:48]

The plan is to have in the component (I'll see if I can remove BookmarkService):
class BookmarkService {}
class BookmarkClient {}
class BookmarkModel : BookmarkService {}

and in the embedder:
class ChromeBookmarkClient : BookmarkClient {}
class BookmarkModelFactory {}

The ChromeBookmarkClient will own the BookmarkModel and pass itself as the
BookmarkClient. The BookmarkModelFactory will create the ChromeBookmarkClient,
and return a BookmarkModel, so the users of BookmarkModel will not have to be
updated.

With this CL (after patch 3, that is not ready yet), I'll introduce
ChromeBookmarkClient, but leave the BookmarkModel in the
chrome/browser/bookmarks directory. It will be moved to the component in a later
CL. I'll update the componentization document but not today, as it is already
quite late here.

I'm trying to avoid introducing a Driver for that component if possible.

Note: this third patch is not ready for review, as I know that unit tests are
not passing, but if you have time, could you have a quick look and tell me if it
is less confusing.

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index.cc (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.cc:139:
client->GetTypedCountForNodes(nodes, node_typed_counts);
On 2014/04/18 17:05:04, sky wrote:
> If the intention is that some clients don't support GetTypedCountForNodes
should
> we have a SupportsTypedCountForNodes on the client interface?

I think that exporting a SupportsTypedCountForNodes method on the client
interface make the code more complex. I've done it, but I think it would be
simpler to state in the interface that it should return a typed count of 0 if it
is not supported.

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.cc (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.cc:969:
base::CancelableTaskTracker::TaskId BookmarkModel::GetFaviconImageForURL(
On 2014/04/18 17:05:04, sky wrote:
> Style guide says declaration order and definition order should match. So, move
> these to match position in header.

Done.

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:54: public BookmarkClient,
On 2014/04/18 17:05:04, sky wrote:
> I'm confused as to why you're making BookmarkModel implement BookmarkClient?
If
> only temporary, I would prefer having it on a separate object that
BookmarkModel
> owns. I like this since you internally have BookmarkModel::LoadFavicon calling
> GetFaviconImageForURL(), which should really by going through BookmarkClient,
> and not BookmarkModel.
> 
> I'm fine with this code existing in bookmark_model.h/.cc for now, but would
> prefer we start down the long term path that we want to get to. Unless I'm
> missing something.

My plan was to have this inheritance temporarily, and then to split the class in
a second step as I feared that having the client a separate class would make the
CL extra large and I prefer working with small CL that do as few modification as
possible.

I've done extracted the ChromeBookmarkClient now, and as I feared this turned
the CL into a monster CL. I think it is going to be a pain to submit through the
CQ, but after that, the last steps of the componentization should be relatively
painless.

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_stats.h (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_stats.h:5: #ifndef
CHROME_BROWSER_BOOKMARKS_BOOKMARK_STATS_H_
On 2014/04/18 17:05:04, sky wrote:
> Are you modifying this file in expectation of moving to components? I think
it's
> chrome specific and shouldn't need to move.

The function RecordBookmarkLaunch is currently used on iOS (with
BOOKMARK_LAUNCH_LOCATION_OMNIBOX).

I modified the file because I tried to remove all dependencies on content/ from
files that were in chrome/browser/bookmarks and were used by iOS. If you think
it is better not to move this file into the component, and I can revert the
corresponding changes.

I've reverted the changes for now, but we may have to consider how to handle
that dependency later.

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_storage.cc (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_storage.cc:53: base::SequencedTaskRunner*
task_runner) {
On 2014/04/18 17:05:04, sky wrote:
> Shouldn't this take a scoped_refptr?

The object returned by Base::Bind store the task_runner in a scoped_refptr (as
the parameter to Base::Bind is a scoped_refptr const&), and when this callback
is invoked, the object is still alive. I think using a scoped_refptr here is not
required.

https://codereview.chromium.org/242693003/diff/20001/components/bookmarks/cor...
File components/bookmarks/core/browser/bookmark_client.h (right):

https://codereview.chromium.org/242693003/diff/20001/components/bookmarks/cor...
components/bookmarks/core/browser/bookmark_client.h:35: // Callback for
GetFaviconImageForURL().
On 2014/04/18 17:05:04, sky wrote:
> Actually, style guide says enums are before constructors/destructor.

Done.

[sdefresne, 2014-04-18 22:31:47]

https://codereview.chromium.org/242693003/diff/40001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/242693003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:60:
bookmark_model->Load(profile->GetIOTaskRunner(),
This won't compile.

https://codereview.chromium.org/242693003/diff/40001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:185:
bookmark_model_->Load(profile_.GetIOTaskRunner(),
This won't compile.

https://codereview.chromium.org/242693003/diff/40001/chrome/test/base/testing...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/242693003/diff/40001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:498:
bookmark_model->Load(profile->GetIOTaskRunner(),
This won't compile.

[sky, 2014-04-21 15:22:40]

On 2014/04/18 22:25:48, sdefresne wrote:
> The plan is to have in the component (I'll see if I can remove
BookmarkService):
> class BookmarkService {}
> class BookmarkClient {}
> class BookmarkModel : BookmarkService {}
> 
> and in the embedder:
> class ChromeBookmarkClient : BookmarkClient {}
> class BookmarkModelFactory {}
> 
> The ChromeBookmarkClient will own the BookmarkModel and pass itself as the
> BookmarkClient. The BookmarkModelFactory will create the ChromeBookmarkClient,
> and return a BookmarkModel, so the users of BookmarkModel will not have to be
> updated.
> 
> With this CL (after patch 3, that is not ready yet), I'll introduce
> ChromeBookmarkClient, but leave the BookmarkModel in the
> chrome/browser/bookmarks directory. It will be moved to the component in a
later
> CL. I'll update the componentization document but not today, as it is already
> quite late here.
> 
> I'm trying to avoid introducing a Driver for that component if possible.
> 
> Note: this third patch is not ready for review, as I know that unit tests are
> not passing, but if you have time, could you have a quick look and tell me if
it
> is less confusing.

This all sounds good to me.

>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> File chrome/browser/bookmarks/bookmark_index.cc (right):
> 
>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> chrome/browser/bookmarks/bookmark_index.cc:139:
> client->GetTypedCountForNodes(nodes, node_typed_counts);
> On 2014/04/18 17:05:04, sky wrote:
> > If the intention is that some clients don't support GetTypedCountForNodes
> should
> > we have a SupportsTypedCountForNodes on the client interface?
> 
> I think that exporting a SupportsTypedCountForNodes method on the client
> interface make the code more complex. I've done it, but I think it would be
> simpler to state in the interface that it should return a typed count of 0 if
it
> is not supported.
> 
>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> File chrome/browser/bookmarks/bookmark_model.cc (right):
> 
>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> chrome/browser/bookmarks/bookmark_model.cc:969:
> base::CancelableTaskTracker::TaskId BookmarkModel::GetFaviconImageForURL(
> On 2014/04/18 17:05:04, sky wrote:
> > Style guide says declaration order and definition order should match. So,
move
> > these to match position in header.
> 
> Done.
> 
>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> File chrome/browser/bookmarks/bookmark_model.h (right):
> 
>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> chrome/browser/bookmarks/bookmark_model.h:54: public BookmarkClient,
> On 2014/04/18 17:05:04, sky wrote:
> > I'm confused as to why you're making BookmarkModel implement BookmarkClient?
> If
> > only temporary, I would prefer having it on a separate object that
> BookmarkModel
> > owns. I like this since you internally have BookmarkModel::LoadFavicon
calling
> > GetFaviconImageForURL(), which should really by going through
BookmarkClient,
> > and not BookmarkModel.
> > 
> > I'm fine with this code existing in bookmark_model.h/.cc for now, but would
> > prefer we start down the long term path that we want to get to. Unless I'm
> > missing something.
> 
> My plan was to have this inheritance temporarily, and then to split the class
in
> a second step as I feared that having the client a separate class would make
the
> CL extra large and I prefer working with small CL that do as few modification
as
> possible.
> 
> I've done extracted the ChromeBookmarkClient now, and as I feared this turned
> the CL into a monster CL. I think it is going to be a pain to submit through
the
> CQ, but after that, the last steps of the componentization should be
relatively
> painless.
> 
>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> File chrome/browser/bookmarks/bookmark_stats.h (right):
> 
>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> chrome/browser/bookmarks/bookmark_stats.h:5: #ifndef
> CHROME_BROWSER_BOOKMARKS_BOOKMARK_STATS_H_
> On 2014/04/18 17:05:04, sky wrote:
> > Are you modifying this file in expectation of moving to components? I think
> it's
> > chrome specific and shouldn't need to move.
> 
> The function RecordBookmarkLaunch is currently used on iOS (with
> BOOKMARK_LAUNCH_LOCATION_OMNIBOX).
> 
> I modified the file because I tried to remove all dependencies on content/
from
> files that were in chrome/browser/bookmarks and were used by iOS. If you think
> it is better not to move this file into the component, and I can revert the
> corresponding changes.
> 
> I've reverted the changes for now, but we may have to consider how to handle
> that dependency later.
> 
>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> File chrome/browser/bookmarks/bookmark_storage.cc (right):
> 
>
https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
> chrome/browser/bookmarks/bookmark_storage.cc:53: base::SequencedTaskRunner*
> task_runner) {
> On 2014/04/18 17:05:04, sky wrote:
> > Shouldn't this take a scoped_refptr?
> 
> The object returned by Base::Bind store the task_runner in a scoped_refptr (as
> the parameter to Base::Bind is a scoped_refptr const&), and when this callback
> is invoked, the object is still alive. I think using a scoped_refptr here is
not
> required.
> 
>
https://codereview.chromium.org/242693003/diff/20001/components/bookmarks/cor...
> File components/bookmarks/core/browser/bookmark_client.h (right):
> 
>
https://codereview.chromium.org/242693003/diff/20001/components/bookmarks/cor...
> components/bookmarks/core/browser/bookmark_client.h:35: // Callback for
> GetFaviconImageForURL().
> On 2014/04/18 17:05:04, sky wrote:
> > Actually, style guide says enums are before constructors/destructor.
> 
> Done.

[sky, 2014-04-21 15:24:19]

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_index.cc (right):

https://codereview.chromium.org/242693003/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_index.cc:139:
client->GetTypedCountForNodes(nodes, node_typed_counts);
On 2014/04/18 22:25:49, sdefresne wrote:
> On 2014/04/18 17:05:04, sky wrote:
> > If the intention is that some clients don't support GetTypedCountForNodes
> should
> > we have a SupportsTypedCountForNodes on the client interface?
> 
> I think that exporting a SupportsTypedCountForNodes method on the client
> interface make the code more complex. I've done it, but I think it would be
> simpler to state in the interface that it should return a typed count of 0 if
it
> is not supported.

My thinking was that you can avoid this loop entirely if the client always
returns 0. But we can do this later.

[tfarina, 2014-04-22 17:22:34]

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:395: class ChromeBookmarkClient :
public content::NotificationObserver,
No, don't do that. Instead move this class declaration into
chrome_bookmark_client.h please.

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:396: public BookmarkClient,
Can you make BookmarkClient be the first in the inheritance list? I.e., swap its
place with NotificationObserver. I'd also put NotificationObserver as the third.

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:399: ChromeBookmarkClient(Profile*
profile);
explicit

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:409: virtual void Shutdown() OVERRIDE;
Please, make sure the overrides follow the same sequence of the inherited
classes.

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:418: 
Do not put blank lines between these overrides. We usually group methods
overridden from the same class together.

[sdefresne, 2014-04-22 20:25:48]

Please have a look.

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:395: class ChromeBookmarkClient :
public content::NotificationObserver,
On 2014/04/22 17:22:34, tfarina wrote:
> No, don't do that. Instead move this class declaration into
> chrome_bookmark_client.h please.

Done.

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:396: public BookmarkClient,
On 2014/04/22 17:22:34, tfarina wrote:
> Can you make BookmarkClient be the first in the inheritance list? I.e., swap
its
> place with NotificationObserver. I'd also put NotificationObserver as the
third.

Done.

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:399: ChromeBookmarkClient(Profile*
profile);
On 2014/04/22 17:22:34, tfarina wrote:
> explicit

Done.

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:409: virtual void Shutdown() OVERRIDE;
On 2014/04/22 17:22:34, tfarina wrote:
> Please, make sure the overrides follow the same sequence of the inherited
> classes.

Done.

https://codereview.chromium.org/242693003/diff/80001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model.h:418: 
On 2014/04/22 17:22:34, tfarina wrote:
> Do not put blank lines between these overrides. We usually group methods
> overridden from the same class together.

Done.

[tfarina, 2014-04-22 21:19:57]

More comments below. It would be good if it would be possible to do a change
like this with fewer changes and in fewer files. But I guess it can't be done
other way.

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_test_helpers.cc (right):

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_test_helpers.cc:101: scoped_ptr<BookmarkModel>
bookmark_model(new BookmarkModel(this));
I don't think using scoped_ptr + release here helps much. I would simply
allocate on the heap and return the raw pointer. Also make sure to document the
ownership in the declaration of CreateModel, saying to clients that
TestBookmarkClient does not own the pointer and the caller is responsible for
releasing the memory after it is done with it.

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_test_helpers.h (right):

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_test_helpers.h:18: class TestBookmarkClient :
public BookmarkClient {
Also, no. Leave this file for helper utility test functions. Please, move this
into test_bookmark_client.h

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_test_helpers.h:23: virtual
base::CancelableTaskTracker::TaskId GetFaviconImageForURL(
Group the overrides, can you also override everything in the private section?
And add a comment like: // BookmarkClient:

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_test_helpers.h:45: };
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
File components/bookmarks/core/browser/bookmark_client.h (right):

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
components/bookmarks/core/browser/bookmark_client.h:27: class Location;
Can you remove this? It looks unused.

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
components/bookmarks/core/browser/bookmark_client.h:70: private:
protected section should come before the private section.

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
components/bookmarks/core/browser/bookmark_client.h:71: friend class
BookmarkModel;
Why do they need to be friends?

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
components/bookmarks/core/browser/bookmark_client.h:75: virtual void
OnClearStore() = 0;
Could you add docs for this method? If, me, as an embedder, was going to
implement this, what I should know?

[tfarina, 2014-04-22 21:23:44]

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.h
(right):

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.h:12: #include
"chrome/browser/bookmarks/bookmark_model.h"
Can you remove this include?

[sdefresne, 2014-04-23 16:10:17]

OK, I've addressed the comments, and rebased on top of the recent changes to
BookmarkModel. Could you take another look?

Note: the try bots are currently failing to apply the patch, but I don't know
why. If I checkout chromium code and apply the patch, it does work. I've tried
to rebase another time to see if it helps.

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_test_helpers.cc (right):

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_test_helpers.cc:101: scoped_ptr<BookmarkModel>
bookmark_model(new BookmarkModel(this));
On 2014/04/22 21:19:58, tfarina wrote:
> I don't think using scoped_ptr + release here helps much. I would simply
> allocate on the heap and return the raw pointer. Also make sure to document
the
> ownership in the declaration of CreateModel, saying to clients that
> TestBookmarkClient does not own the pointer and the caller is responsible for
> releasing the memory after it is done with it.

Done.

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_test_helpers.h (right):

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_test_helpers.h:18: class TestBookmarkClient :
public BookmarkClient {
On 2014/04/22 21:19:58, tfarina wrote:
> Also, no. Leave this file for helper utility test functions. Please, move this
> into test_bookmark_client.h

Done.

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_test_helpers.h:23: virtual
base::CancelableTaskTracker::TaskId GetFaviconImageForURL(
On 2014/04/22 21:19:58, tfarina wrote:
> Group the overrides, can you also override everything in the private section?
> And add a comment like: // BookmarkClient:

Done.

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_test_helpers.h:45: };
On 2014/04/22 21:19:58, tfarina wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.h
(right):

https://codereview.chromium.org/242693003/diff/130001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_name_folder_controller.h:12: #include
"chrome/browser/bookmarks/bookmark_model.h"
On 2014/04/22 21:23:44, tfarina wrote:
> Can you remove this include?

Done.

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
File components/bookmarks/core/browser/bookmark_client.h (right):

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
components/bookmarks/core/browser/bookmark_client.h:27: class Location;
On 2014/04/22 21:19:58, tfarina wrote:
> Can you remove this? It looks unused.

Done.

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
components/bookmarks/core/browser/bookmark_client.h:70: private:
On 2014/04/22 21:19:58, tfarina wrote:
> protected section should come before the private section.

I removed the private section.

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
components/bookmarks/core/browser/bookmark_client.h:71: friend class
BookmarkModel;
On 2014/04/22 21:19:58, tfarina wrote:
> Why do they need to be friends?

They needed to be friend to call OnLoad / OnClearStore /
NotifyHistoryAboutRemovedBookmarks that were private. Since they have been
deleted / are no longer private, there is no need for friendship.

https://codereview.chromium.org/242693003/diff/130001/components/bookmarks/co...
components/bookmarks/core/browser/bookmark_client.h:75: virtual void
OnClearStore() = 0;
On 2014/04/22 21:19:58, tfarina wrote:
> Could you add docs for this method? If, me, as an embedder, was going to
> implement this, what I should know?

Removed it.

[sky, 2014-04-23 20:32:07]

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_index.cc (right):

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_index.cc:100: : client_(client),
languages_(languages), index_urls_(index_urls) {
nit: one member per line, like BookmarkModel.

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_model.cc (right):

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_model.cc:749:
client_->NotifyHistoryAboutRemovedBookmarks(removed_urls);
I don't think this needs to be on the client. The chrome side can add an
observer to detect this and cleanup history. If you want to do this later, add a
TODO for now.

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_storage.h (right):

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_storage.h:130: BookmarkLoadDetails* details,
Can you make this take a scoped_ptr?

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
File chrome/browser/bookmarks/test_bookmark_client.h (right):

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
chrome/browser/bookmarks/test_bookmark_client.h:21: BookmarkModel*
CreateModel(bool index_urls);
Return scoped_ptr

[sdefresne, 2014-04-24 12:30:14]

PTAL

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_index.cc (right):

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_index.cc:100: : client_(client),
languages_(languages), index_urls_(index_urls) {
On 2014/04/23 20:32:07, sky wrote:
> nit: one member per line, like BookmarkModel.

Done.

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_model.cc (right):

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_model.cc:749:
client_->NotifyHistoryAboutRemovedBookmarks(removed_urls);
On 2014/04/23 20:32:07, sky wrote:
> I don't think this needs to be on the client. The chrome side can add an
> observer to detect this and cleanup history. If you want to do this later, add
a
> TODO for now.

Make sense. I was also considering doing it. I'll do it in a followup CL, added
a TODO.

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
File chrome/browser/bookmarks/bookmark_storage.h (right):

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
chrome/browser/bookmarks/bookmark_storage.h:130: BookmarkLoadDetails* details,
On 2014/04/23 20:32:07, sky wrote:
> Can you make this take a scoped_ptr?

Done.

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
File chrome/browser/bookmarks/test_bookmark_client.h (right):

https://codereview.chromium.org/242693003/diff/190001/chrome/browser/bookmark...
chrome/browser/bookmarks/test_bookmark_client.h:21: BookmarkModel*
CreateModel(bool index_urls);
On 2014/04/23 20:32:07, sky wrote:
> Return scoped_ptr

Done.

[sky, 2014-04-24 16:22:27]

LGTM

[sdefresne, 2014-04-24 16:44:59]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-04-24 16:46:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sdefresne@chromium.org/242693003/210001

[commit-bot: I haz the power, 2014-04-24 21:07:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:07:23]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[sdefresne, 2014-04-25 10:57:16]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-04-25 12:52:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sdefresne@chromium.org/242693003/230001

[commit-bot: I haz the power, 2014-04-26 06:27:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 06:27:17]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[tfarina, 2014-04-26 13:44:00]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2014-04-26 13:44:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sdefresne@chromium.org/242693003/230001

[commit-bot: I haz the power, 2014-04-26 22:06:58]

Change committed as 266376