Introduce ChromeBookmarkClientFactory

Introduce ChromeBookmarkClientFactory

Fix BookmarkModelFactory to really build a BookmarkModel object and
introduce another factory ChromeBookmarkClientFactory to build the
ChromeBookmarkClient and manage the correct dependency between the
two objects.

Change BookmarkModelFactory::GetChromeBookmarkClientForProfile and
clients to instead use ChromeBookmarkClientFactory::GetForProfile.


BUG=364865
TBR=asargent,pavely,shess,pkasting,phajdan.jr

[sdefresne, 2014-06-04 15:55:20]

Please take a look.

This is based on https://codereview.chromium.org/306143003/ that has been sent
to the CQ.

[tfarina, 2014-06-04 16:02:56]

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

https://codereview.chromium.org/306293006/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:8: #include
"base/compiler_specific.h"
can you include macros.h instead?

https://codereview.chromium.org/306293006/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:11: #include
"components/keyed_service/core/keyed_service.h"
can you remove this now?

[tfarina, 2014-06-04 16:04:01]

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

https://codereview.chromium.org/306293006/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model_factory.h:11: template <typename T>
why this change? could you revert it? it puts an unnecessary blame ere.

[sdefresne, 2014-06-04 16:08:48]

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

https://codereview.chromium.org/306293006/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model_factory.h:11: template <typename T>
On 2014/06/04 16:04:02, tfarina wrote:
> why this change? could you revert it? it puts an unnecessary blame ere.

This was reformated by clang-format when I ran it over the whole CL. I can
revert it if really required, but I don't think that changing the blame on that
line is a problem.

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

https://codereview.chromium.org/306293006/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:8: #include
"base/compiler_specific.h"
On 2014/06/04 16:02:57, tfarina wrote:
> can you include macros.h instead?

Done.

https://codereview.chromium.org/306293006/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:11: #include
"components/keyed_service/core/keyed_service.h"
On 2014/06/04 16:02:57, tfarina wrote:
> can you remove this now?

Done.

[tfarina, 2014-06-04 16:09:18]

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

https://codereview.chromium.org/306293006/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:123:
BookmarkModelFactory::GetInstance()->SetTestingFactoryAndUse(
This code is now almost the same as TestingProfile::CreateBookmarkModel(). See
my comments there. If we could simplify it, then we could reuse it.

I'm glad we are moving forward and cleaning this up.

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

https://codereview.chromium.org/306293006/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:523: base::DeleteFile(path, false);
This was always a mistery to me. And I never understood fully why we do this
here. Do you understand this code?

https://codereview.chromium.org/306293006/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:526:
ChromeHistoryClientFactory::GetInstance()->SetTestingFactory(
Do you understand why we initialize History here? I think it would be great if
we could decouple this (in a follow up).

[tfarina, 2014-06-04 16:14:24]

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

https://codereview.chromium.org/306293006/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model_factory.h:11: template <typename T>
On 2014/06/04 16:08:48, sdefresne wrote:
> On 2014/06/04 16:04:02, tfarina wrote:
> > why this change? could you revert it? it puts an unnecessary blame ere.
> 
> This was reformated by clang-format when I ran it over the whole CL. I can
> revert it if really required, but I don't think that changing the blame on
that
> line is a problem.
I know we can go back in time and see who added this line but I think the
obfuscating for the sake of clang-format is not worth it.

The style guide
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...,
has no examples of how to format it. So it may be that both forms are
acceptable.

You can see in this search:
https://code.google.com/p/chromium/codesearch#search/&q=template%20%3Ctypenam...,
that a single line declaration is very common.

[blundell, 2014-06-05 09:48:30]

LGTM

I like this design better; let's definitely keep using this pattern going
forward when relevant

https://codereview.chromium.org/306293006/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/306293006/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:59: }
You should call BookmarkClient::Shutdown() here.

https://codereview.chromium.org/306293006/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/306293006/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:65: BookmarkModel* model_;
Document lifetime expectations.

[sdefresne, 2014-06-10 13:12:21]

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

https://codereview.chromium.org/306293006/diff/20001/chrome/browser/bookmarks...
chrome/browser/bookmarks/bookmark_model_factory.h:11: template <typename T>
On 2014/06/04 16:14:25, tfarina wrote:
> On 2014/06/04 16:08:48, sdefresne wrote:
> > On 2014/06/04 16:04:02, tfarina wrote:
> > > why this change? could you revert it? it puts an unnecessary blame ere.
> > 
> > This was reformated by clang-format when I ran it over the whole CL. I can
> > revert it if really required, but I don't think that changing the blame on
> that
> > line is a problem.
> I know we can go back in time and see who added this line but I think the
> obfuscating for the sake of clang-format is not worth it.
> 
> The style guide
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Forwar...,
> has no examples of how to format it. So it may be that both forms are
> acceptable.
> 
> You can see in this search:
>
https://code.google.com/p/chromium/codesearch#search/&q=template%20%3Ctypenam...,
> that a single line declaration is very common.

Done.

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

https://codereview.chromium.org/306293006/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:523: base::DeleteFile(path, false);
On 2014/06/04 16:09:19, tfarina wrote:
> This was always a mistery to me. And I never understood fully why we do this
> here. Do you understand this code?

I think this is so that we do not load saved bookmarks from a previous test.

https://codereview.chromium.org/306293006/diff/20001/chrome/test/base/testing...
chrome/test/base/testing_profile.cc:526:
ChromeHistoryClientFactory::GetInstance()->SetTestingFactory(
On 2014/06/04 16:09:19, tfarina wrote:
> Do you understand why we initialize History here? I think it would be great if
> we could decouple this (in a follow up).

This is for test that want to check interactions between bookmarks and history
(ie. adding a bookmark prevent the deletion of the corresponding favicon from
history for example).

https://codereview.chromium.org/306293006/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.cc (right):

https://codereview.chromium.org/306293006/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.cc:59: }
On 2014/06/05 09:48:30, blundell wrote:
> You should call BookmarkClient::Shutdown() here.

Done.

https://codereview.chromium.org/306293006/diff/40001/chrome/browser/bookmarks...
File chrome/browser/bookmarks/chrome_bookmark_client.h (right):

https://codereview.chromium.org/306293006/diff/40001/chrome/browser/bookmarks...
chrome/browser/bookmarks/chrome_bookmark_client.h:65: BookmarkModel* model_;
On 2014/06/05 09:48:30, blundell wrote:
> Document lifetime expectations.

Done.

[sdefresne, 2014-06-10 13:13:31]

The CL has quite changed after the rebase due to recent changes to
ChromeBookmarkClient. Can you take another look?

[blundell, 2014-06-10 14:29:57]

SLGTM

[sdefresne, 2014-06-11 17:13:59]

+joaodasilva FYI

TBR-ing OWNERS of refactored code (using the correct interface instead of
BookmarkModelFactory::GetChromeBookmarkClientForProfile).

TBR=asargent for //chrome/browser/extensions/api
TBR=pavely for //chrome/browser/sync
TBR=shess for //chrome/browser/ui/cocoa
TBR=pkasting for //chrome/browser/ui/views
TBR=phajdan.jr for //chrome/test

[sdefresne, 2014-06-11 17:23:59]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-11 17:25:34]

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

[Scott Hess - ex-Googler, 2014-06-11 18:31:11]

lgtm for cocoa/

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h (right):

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h:28: const
BookmarkNode* node_;  // weak
Pick your layout - either two spaces for all three, or line all three up!

[Peter Kasting, 2014-06-11 18:35:13]

LGTM

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:921: :
model_->bookmark_bar_node();
Nit: Please don't change the previous line wrapping (this is a clang-format
error)

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1341: client_ =
ChromeBookmarkClientFactory::GetForProfile(browser_->profile());
Nit: Move this below the remainder of the block that deals with |model_|

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1673: :
model_->bookmark_bar_node()->GetChild(location->index);
Nit: Also avoid changing the wrapping/indenting here

[asargent_no_longer_on_chrome, 2014-06-11 18:52:17]

extensions part lgtm

[pavely, 2014-06-11 19:40:28]

//chrome/browser/sync lgtm

[commit-bot: I haz the power, 2014-06-11 20:31:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-11 20:35:26]

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

[commit-bot: I haz the power, 2014-06-11 20:35:27]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[Joao da Silva, 2014-06-11 20:37:32]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-11 20:40:42]

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

[commit-bot: I haz the power, 2014-06-11 21:04:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[sdefresne, 2014-06-11 21:05:46]

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h (right):

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/bookmarks/bookmark_bubble_controller.h:28: const
BookmarkNode* node_;  // weak
On 2014/06/11 18:31:11, shess wrote:
> Pick your layout - either two spaces for all three, or line all three up!

This was formatted with clang-format. Probably a bug of the tool. Picked two
spaces.

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:921: :
model_->bookmark_bar_node();
On 2014/06/11 18:35:12, Peter Kasting wrote:
> Nit: Please don't change the previous line wrapping (this is a clang-format
> error)

Done.

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1341: client_ =
ChromeBookmarkClientFactory::GetForProfile(browser_->profile());
On 2014/06/11 18:35:12, Peter Kasting wrote:
> Nit: Move this below the remainder of the block that deals with |model_|

Done.

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1673: :
model_->bookmark_bar_node()->GetChild(location->index);
On 2014/06/11 18:35:12, Peter Kasting wrote:
> Nit: Also avoid changing the wrapping/indenting here

Done.

[sdefresne, 2014-06-11 21:05:56]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:07:59]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[sdefresne, 2014-06-11 21:08:25]

The CQ bit was unchecked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:08:30]

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

[sdefresne, 2014-06-11 21:08:35]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-11 21:11:51]

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

[sdefresne, 2014-06-11 21:46:04]

The CQ bit was unchecked by sdefresne@chromium.org

[sdefresne, 2014-06-11 21:49:39]

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/306293006/diff/140001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1341: client_ =
ChromeBookmarkClientFactory::GetForProfile(browser_->profile());
On 2014/06/11 21:05:46, sdefresne wrote:
> On 2014/06/11 18:35:12, Peter Kasting wrote:
> > Nit: Move this below the remainder of the block that deals with |model_|
> 
> Done.

After a second look, I cannot change the order.
ChromeBookmarkClientFactory::GetForProfile must be called after
BookmarkModelFactory::GetForProfile but BookmarkBarView::BookmarkModelLoaded
uses client_ member, so it must be initialized before the if block. Reverted.

[sdefresne, 2014-06-11 22:02:20]

The CQ bit was checked by sdefresne@chromium.org

[sdefresne, 2014-06-11 22:02:29]

The CQ bit was checked by sdefresne@chromium.org

[commit-bot: I haz the power, 2014-06-11 22:04:44]

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

[commit-bot: I haz the power, 2014-06-11 23:17:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15700)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18833)

[commit-bot: I haz the power, 2014-06-11 23:18:30]

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

[commit-bot: I haz the power, 2014-06-11 23:18:32]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/15766)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/18898)

[Joao da Silva, 2014-06-12 05:26:55]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-12 05:29:41]

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

[commit-bot: I haz the power, 2014-06-12 09:48:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-12 12:01:48]

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

[commit-bot: I haz the power, 2014-06-12 12:01:50]

Try jobs failed on following builders:
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)

[Joao da Silva, 2014-06-13 15:00:36]

The CQ bit was checked by joaodasilva@chromium.org

[commit-bot: I haz the power, 2014-06-13 15:01:47]

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

[Joao da Silva, 2014-06-13 17:06:21]

The CQ bit was unchecked by joaodasilva@chromium.org

[tfarina, 2014-06-14 18:18:16]

Closing this.

João landed it in https://codereview.chromium.org/330343002/ - crrev.com/277138

[Paweł Hajdan Jr., 2014-07-25 14:25:29]

chrome/test LGTM