sync/glue: Convert BookmarkDataTypeController to BaseBookmarkModelObserver.

sync/glue: Convert BookmarkDataTypeController to BaseBookmarkModelObserver.

This removes the last consumer of chrome::NOTIFICATION_BOOKMARK_MODEL_LOADED.

BUG=144783
TEST=unit_tests --gtest_filter=SyncBookmarkDataTypeControllerTest*
R=zea@chromium.org

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

[tfarina, 2013-01-30 04:40:29]

Hi Nicolas, this what I'm working on. This is not ready to be landed, but I'd
like to get your eyes on it. You might spot something I'm missing. It's pretty
late here (2:40 AM), so I'm going to get some sleep now.

$ out/Debug/unit_tests --gtest_filter=SyncBookmarkDataTypeControllerTest*
Note: Google Test filter = SyncBookmarkDataTypeControllerTest*
[==========] Running 11 tests from 1 test case.                                 
                                                                                
                               
[----------] Global test environment set-up.
[----------] 11 tests from SyncBookmarkDataTypeControllerTest
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartDependentsReady

GMOCK WARNING:
Uninteresting mock function call - returning default value.
    Function call: IsLoaded()
          Returns: false
Stack trace:
[       OK ] SyncBookmarkDataTypeControllerTest.StartDependentsReady (69 ms)
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartBookmarkModelNotReady

GMOCK WARNING:
Uninteresting mock function call - returning default value.
    Function call: IsLoaded()
          Returns: false
Stack trace:
[4168:4168:0130/023853:277032247138:FATAL:observer_list.h(122)] Check failed:
false. Observers can only be added once!
 [0x7f5ca4e8893e] base::debug::StackTrace::StackTrace()
 [0x7f5ca4ec49ff] logging::LogMessage::~LogMessage()
 [0x0000023ffa8a] ObserverListBase<>::AddObserver()
 [0x0000023fa7ef] BookmarkModel::AddObserver()
 [0x000002b22a48] browser_sync::BookmarkDataTypeController::StartModels()
 [0x000002b381e4] browser_sync::FrontendDataTypeController::LoadModels()
 [0x0000017a3a27]
SyncBookmarkDataTypeControllerTest_StartBookmarkModelNotReady_Test::TestBody()
 [0x000003a07213] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x0000039ff08e] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x0000039f6ae5] testing::Test::Run()
 [0x0000039f71eb] testing::TestInfo::Run()
 [0x0000039f7787] testing::TestCase::Run()
 [0x0000039fbab5] testing::internal::UnitTestImpl::RunAllTests()
 [0x000003a04923] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x000003a004ee] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x0000039fb7a4] testing::UnitTest::Run()
 [0x00000471ae6f] base::TestSuite::Run()
 [0x0000046d486d] content::UnitTestTestSuite::Run()
 [0x0000044ee1b6] main
 [0x7f5c972eeeff] __libc_start_main
 [0x00000054c769] <unknown>

[4168:4168:0130/023853:277032247138:FATAL:observer_list.h(122)] Check failed:
false. Observers can only be added once!
 [0x7f5ca4e8893e] base::debug::StackTrace::StackTrace()
 [0x7f5ca4ec49ff] logging::LogMessage::~LogMessage()
 [0x0000023ffa8a] ObserverListBase<>::AddObserver()
 [0x0000023fa7ef] BookmarkModel::AddObserver()
 [0x000002b22a48] browser_sync::BookmarkDataTypeController::StartModels()
 [0x000002b381e4] browser_sync::FrontendDataTypeController::LoadModels()
 [0x0000017a3a27]
SyncBookmarkDataTypeControllerTest_StartBookmarkModelNotReady_Test::TestBody()
 [0x000003a07213] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x0000039ff08e] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x0000039f6ae5] testing::Test::Run()
 [0x0000039f71eb] testing::TestInfo::Run()
 [0x0000039f7787] testing::TestCase::Run()
 [0x0000039fbab5] testing::internal::UnitTestImpl::RunAllTests()
 [0x000003a04923] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x000003a004ee] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x0000039fb7a4] testing::UnitTest::Run()
 [0x00000471ae6f] base::TestSuite::Run()
 [0x0000046d486d] content::UnitTestTestSuite::Run()
 [0x0000044ee1b6] main
 [0x7f5c972eeeff] __libc_start_main
 [0x00000054c769] <unknown>

Trace/breakpoint trap

[Nicolas Zea, 2013-01-30 19:02:20]

https://codereview.chromium.org/12084058/diff/1/chrome/browser/sync/glue/book...
File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right):

https://codereview.chromium.org/12084058/diff/1/chrome/browser/sync/glue/book...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:85:
model->RemoveObserver(this);
maybe DCHECK(model->IsLoaded()) here

https://codereview.chromium.org/12084058/diff/1/chrome/browser/sync/glue/book...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (left):

https://codereview.chromium.org/12084058/diff/1/chrome/browser/sync/glue/book...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:173:
content::NotificationService::current()->Notify(
What is triggering the Loaded() method call now?

[tfarina, 2013-01-30 19:16:41]

I'm building locally to try to run this again.

https://codereview.chromium.org/12084058/diff/1/chrome/browser/sync/glue/book...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (left):

https://codereview.chromium.org/12084058/diff/1/chrome/browser/sync/glue/book...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:173:
content::NotificationService::current()->Notify(
On 2013/01/30 19:02:21, Nicolas Zea wrote:
> What is triggering the Loaded() method call now?
The BookmarkModel::Load method should be called if you pass a NULL Profile
pointer to BookmarkModel or through
BookmarkModelFactory::BuildServiceInstanceFor which is called from Profiles API
which I don't know very well in details
(ProfileKeyedServiceFactory::GetServiceForProfile).

[tfarina, 2013-01-30 22:34:17]

I added a hack, but didn't make the test pass yet.

It isn't still clear to me what to do :/

GMOCK WARNING:
Uninteresting mock function call - returning default value.
    Function call: IsLoaded()
          Returns: false
Stack trace:
../../chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:191:
Failure
Value of: bookmark_dtc_->state()
  Actual: 2
Expected: DataTypeController::MODEL_STARTING
Which is: 1
[25831:25831:0130/203301:341480175152:FATAL:bookmark_data_type_controller.cc(43)]
Check failed: state_ == MODEL_STARTING (2 vs. 1)

[tfarina, 2013-01-31 03:28:36]

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right):

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:42: DCHECK_EQ(state_,
MODEL_STARTING);
where do we set |state_|?

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (left):

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:173:
content::NotificationService::current()->Notify(
I'm not sure how we could emulate/trigger the Loaded notification now.

As as far I can see/understand, that only happens if you pass NULL to
BookmarkModel, but if you use BookmarkModelFactory then it calls Load() for you.

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:49:
MOCK_CONST_METHOD0(IsLoaded, bool(void));
this is the only place where we mock BookmarkModel. :/

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:85:
BookmarkModelFactory::GetInstance()->SetTestingFactoryAndUse(
does this code here makes BookmarkModelFactory::GetForProfile(profile_); in the
source file return the BookmarkModelMock?

[tfarina, 2013-01-31 03:42:36]

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:84:
bookmark_model_ = static_cast<BookmarkModelMock*>(
can we move away of this complicated mock?

And use it simply as: BookmarkModel model(NULL); ?

[Nicolas Zea, 2013-01-31 20:48:27]

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:84:
bookmark_model_ = static_cast<BookmarkModelMock*>(
On 2013/01/31 03:42:36, tfarina wrote:
> can we move away of this complicated mock?
> 
> And use it simply as: BookmarkModel model(NULL); ?

I'm fine with moving away from the mock, but in its place you'll need to
implement a fake. For example, the fake could return false for IsLoaded until
you call TriggerLoad(), at which point it invokes Loaded() and IsLoaded returns
true. That will allow the test to verify that the datatype controller blocks.

The tricky part there is that the AddObserver and RemoveObserver are not
virtual. Perhaps change BookmarkModel so that they are, and you can therefore
track the observers within the fake?

https://codereview.chromium.org/12084058/diff/8001/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:85:
BookmarkModelFactory::GetInstance()->SetTestingFactoryAndUse(
On 2013/01/31 03:28:36, tfarina wrote:
> does this code here makes BookmarkModelFactory::GetForProfile(profile_); in
the
> source file return the BookmarkModelMock?

correct.

[tfarina, 2013-01-31 21:05:42]

Nicolas, would you be envision something like the following?

class FakeBookmarkModel : public BookmarkModel {
 public:
  FakeBookmarkModel();

  virtual void AddObserver(BookmarkModelObserver* observer) OVERRIDE {
    observers_.AddObserver(observer);
  }

  virtual void RemoveObserver(BookmarkModelObserver* observer) OVERRIDE {
    observers_.RemoveObserver(observer);
  }

  virtual bool IsLoaded() OVERRIDE { return is_loaded_; }

  void TriggerLoad() {
    is_loaded_ = true;
    FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, Loaded(...));
  }

 private:
  ObserverList<BookmarkModelObserver> observers_;
};
-- 
Thiago

[Nicolas Zea, 2013-01-31 21:06:45]

On 2013/01/31 21:05:42, tfarina wrote:
> Nicolas, would you be envision something like the following?
> 
> class FakeBookmarkModel : public BookmarkModel {
>  public:
>   FakeBookmarkModel();
> 
>   virtual void AddObserver(BookmarkModelObserver* observer) OVERRIDE {
>     observers_.AddObserver(observer);
>   }
> 
>   virtual void RemoveObserver(BookmarkModelObserver* observer) OVERRIDE {
>     observers_.RemoveObserver(observer);
>   }
> 
>   virtual bool IsLoaded() OVERRIDE { return is_loaded_; }
> 
>   void TriggerLoad() {
>     is_loaded_ = true;
>     FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, Loaded(...));
>   }
> 
>  private:
>   ObserverList<BookmarkModelObserver> observers_;
> };
> -- 
> Thiago

Correct (with a local is_loaded_ value in the fake).

[tfarina, 2013-02-01 20:16:12]

Nicolas, I think you can take a preliminary look at FakeBookmarkModel and see
how I'm going.

I didn't hook up the BookmarkDataTypeController with BaseBookmarkModelObserver
yet, but I'm going to look into this right now.

[Nicolas Zea, 2013-02-01 21:42:31]

The FakeBookmarkModel looks good.

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

https://codereview.chromium.org/12084058/diff/17001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:118:
EXPECT_TRUE(bookmark_model_->IsLoaded());
This isn't really right. The previous version was simulating it being loaded,
now you're just expecting it to be without actually triggering the load. 

The question is where is the right place to trigger the load. One option might
be to pass a bool load_bookmark_model into this function and if true call
TriggerLoad. That way in the BookmarkModelNotReady test you can pass false in,
while in the other tests you pass true.

https://codereview.chromium.org/12084058/diff/17001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:190:
EXPECT_TRUE(bookmark_model_->IsLoaded());
This won't work here (or shouldn't anyways), and should be moved to after the
TriggerLoad call.

[Nicolas Zea, 2013-02-01 21:43:22]

On 2013/02/01 21:42:31, Nicolas Zea wrote:
> The FakeBookmarkModel looks good.
> 
>
https://codereview.chromium.org/12084058/diff/17001/chrome/browser/sync/glue/...
> File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc
(right):
> 
>
https://codereview.chromium.org/12084058/diff/17001/chrome/browser/sync/glue/...
> chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:118:
> EXPECT_TRUE(bookmark_model_->IsLoaded());
> This isn't really right. The previous version was simulating it being loaded,
> now you're just expecting it to be without actually triggering the load. 
> 
> The question is where is the right place to trigger the load. One option might
> be to pass a bool load_bookmark_model into this function and if true call
> TriggerLoad. That way in the BookmarkModelNotReady test you can pass false in,
> while in the other tests you pass true.
> 
>
https://codereview.chromium.org/12084058/diff/17001/chrome/browser/sync/glue/...
> chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:190:
> EXPECT_TRUE(bookmark_model_->IsLoaded());
> This won't work here (or shouldn't anyways), and should be moved to after the
> TriggerLoad call.

Also, once this is finished you'll need a bookmarks/ owner to review the
bookmark model change (I'm only a sync owner)

[tfarina, 2013-02-01 22:32:37]

Nicolas, thanks a lot for the review!

I made the suggested changes to SetStartExpectations() and added
load_bookmark_model to it.

We are more closer to get all tests passing now.

[==========] Running 11 tests from 1 test case.                                 
                                                                                
                               
[----------] Global test environment set-up.
[----------] 11 tests from SyncBookmarkDataTypeControllerTest
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartDependentsReady
[       OK ] SyncBookmarkDataTypeControllerTest.StartDependentsReady (60 ms)
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartBookmarkModelNotReady
../../chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:192:
Failure
Value of: bookmark_dtc_->state()
  Actual: 2
Expected: DataTypeController::MODEL_STARTING
Which is: 1
[  FAILED  ] SyncBookmarkDataTypeControllerTest.StartBookmarkModelNotReady (32
ms)
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartHistoryServiceNotReady
[       OK ] SyncBookmarkDataTypeControllerTest.StartHistoryServiceNotReady (31
ms)
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartFirstRun
[       OK ] SyncBookmarkDataTypeControllerTest.StartFirstRun (31 ms)
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartBusy
[11868:11868:0201/203039:157720711097:ERROR:frontend_data_type_controller.cc(43)]
Bookmarks, Sync Error: Model already running
[       OK ] SyncBookmarkDataTypeControllerTest.StartBusy (39 ms)
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartOk
[       OK ] SyncBookmarkDataTypeControllerTest.StartOk (38 ms)
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartAssociationFailed
[11868:11868:0201/203039:157720804358:ERROR:bookmark_data_type_controller_unittest.cc(267)]
Bookmarks, Sync Error: error
[       OK ] SyncBookmarkDataTypeControllerTest.StartAssociationFailed (44 ms)
[ RUN      ]
SyncBookmarkDataTypeControllerTest.StartAssociationTriggersUnrecoverableError
[11868:11868:0201/203039:157720835826:ERROR:frontend_data_type_controller.cc(173)]
Bookmarks, Sync Error: Failed to load sync nodes
[       OK ]
SyncBookmarkDataTypeControllerTest.StartAssociationTriggersUnrecoverableError
(32 ms)
[ RUN      ] SyncBookmarkDataTypeControllerTest.StartAborted
[11868:11868:0201/203039:157720877901:ERROR:frontend_data_type_controller.cc(226)]
Bookmarks, Sync Error: Aborted
[       OK ] SyncBookmarkDataTypeControllerTest.StartAborted (38 ms)
[ RUN      ] SyncBookmarkDataTypeControllerTest.Stop
[       OK ] SyncBookmarkDataTypeControllerTest.Stop (30 ms)
[ RUN      ]
SyncBookmarkDataTypeControllerTest.OnSingleDatatypeUnrecoverableError
[       OK ]
SyncBookmarkDataTypeControllerTest.OnSingleDatatypeUnrecoverableError (36 ms)
[----------] 11 tests from SyncBookmarkDataTypeControllerTest (426 ms total)

[----------] Global test environment tear-down
[==========] 11 tests from 1 test case ran. (426 ms total)
[  PASSED  ] 10 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] SyncBookmarkDataTypeControllerTest.StartBookmarkModelNotReady

[tfarina, 2013-02-01 22:33:29]

Oh, forgot to note that once this is finished I will add Scott (sky) as reviewer
for chrome/browser/bookmarks/ changes.

[tfarina, 2013-02-01 23:14:11]

Nicolas, I think this is finished! Yay!

All SyncBookmarkDataTypeControllerTest tests passed.

Please, could you take another look?

Thank you!

[Nicolas Zea, 2013-02-01 23:46:27]

https://codereview.chromium.org/12084058/diff/9006/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right):

https://codereview.chromium.org/12084058/diff/9006/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:87: OnModelLoaded();
This isn't quite right. I think you should keep the DependentsLoaded method and
call it here. The problem is that it's possible the HistoryService isn't loaded
at this point, so we don't want to call OnModelLoaded yet.

[tfarina, 2013-02-02 00:08:39]

https://codereview.chromium.org/12084058/diff/9006/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right):

https://codereview.chromium.org/12084058/diff/9006/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:87: OnModelLoaded();
On 2013/02/01 23:46:27, Nicolas Zea wrote:
> This isn't quite right. I think you should keep the DependentsLoaded method
and
> call it here. The problem is that it's possible the HistoryService isn't
loaded
> at this point, so we don't want to call OnModelLoaded yet.

Done.

[tfarina, 2013-02-04 18:53:14]

Ping?

[Nicolas Zea, 2013-02-04 18:58:27]

https://codereview.chromium.org/12084058/diff/3006/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/12084058/diff/3006/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_model.h:245: virtual void
AddObserver(BookmarkModelObserver* observer);
Why did you move these up?

https://codereview.chromium.org/12084058/diff/3006/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right):

https://codereview.chromium.org/12084058/diff/3006/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:78: OnModelLoaded();
You're not checking for the bookmark model loaded case here. I think you should
bring back the DependentsLoaded method and check it here/in the Loaded function
below. Both dependents need to be loaded before we start the datatype
controller.

[tfarina, 2013-02-04 20:44:28]

Nicolas, thanks for the review!

I reverted most of the style changes, and turns out the diff is now much easier
to read/understand.

Please, could you take another look?

https://codereview.chromium.org/12084058/diff/3006/chrome/browser/bookmarks/b...
File chrome/browser/bookmarks/bookmark_model.h (right):

https://codereview.chromium.org/12084058/diff/3006/chrome/browser/bookmarks/b...
chrome/browser/bookmarks/bookmark_model.h:245: virtual void
AddObserver(BookmarkModelObserver* observer);
On 2013/02/04 18:58:28, Nicolas Zea wrote:
> Why did you move these up?

It was an arbitrary change. To separate virtual functions from non-virtual
functions. Yeah, I know this file isn't too consistent about this. :/

https://codereview.chromium.org/12084058/diff/3006/chrome/browser/sync/glue/b...
File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right):

https://codereview.chromium.org/12084058/diff/3006/chrome/browser/sync/glue/b...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:78: OnModelLoaded();
On 2013/02/04 18:58:28, Nicolas Zea wrote:
> You're not checking for the bookmark model loaded case here. I think you
should
> bring back the DependentsLoaded method and check it here/in the Loaded
function
> below. Both dependents need to be loaded before we start the datatype
> controller.

Done.

[tfarina, 2013-02-05 15:00:19]

Hi Nicolas, does this L G T Y now?

Can and send this to Scott if you are happy with the current shape?

Thanks!

[Nicolas Zea, 2013-02-05 18:20:46]

LGTM once the changes are made

https://codereview.chromium.org/12084058/diff/28006/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right):

https://codereview.chromium.org/12084058/diff/28006/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:47:
registrar_.RemoveAll();
remove the datatype controller from the BookmarkModel's observer list here too
(before calling OnModelLoaded)

https://codereview.chromium.org/12084058/diff/28006/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:88: OnModelLoaded();
remove the dtc from the registrar before calling OnModelLoaded here as well

[tfarina, 2013-02-05 18:32:13]

https://codereview.chromium.org/12084058/diff/28006/chrome/browser/sync/glue/...
File chrome/browser/sync/glue/bookmark_data_type_controller.cc (right):

https://codereview.chromium.org/12084058/diff/28006/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:47:
registrar_.RemoveAll();
On 2013/02/05 18:20:47, Nicolas Zea wrote:
> remove the datatype controller from the BookmarkModel's observer list here too
> (before calling OnModelLoaded)

Done.

https://codereview.chromium.org/12084058/diff/28006/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller.cc:88: OnModelLoaded();
On 2013/02/05 18:20:47, Nicolas Zea wrote:
> remove the dtc from the registrar before calling OnModelLoaded here as well

Done.

[tfarina, 2013-02-05 20:09:06]

+sky for final approval!

Scott, could you review my changes to c/b/bookmarks? I think an overall look at
the API usage in sync/glue is fine too.

Thanks,

[sky, 2013-02-05 21:43:58]

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

https://codereview.chromium.org/12084058/diff/36001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:47: class
FakeBookmarkModel : public BookmarkModel {
BookmarkModel isn't intended to be subclassed. Please find another way to
accomplish what you are trying to do.

[tfarina, 2013-02-06 00:32:12]

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

https://codereview.chromium.org/12084058/diff/38002/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:87:
ui_test_utils::WaitForBookmarkModelToLoad(bookmark_model_);
I'm getting a crash here:

 [0x7f94407b4ede] base::debug::StackTrace::StackTrace()
 [0x7f94407b4a15] base::debug::(anonymous namespace)::StackDumpSignalHandler()
 [0x7f9435bbdc60] <unknown>
 [0x0000034a5213] ui_test_utils::WaitForBookmarkModelToLoad()
 [0x0000017f1729] SyncBookmarkDataTypeControllerTest::SetStartExpectations()
 [0x0000017edd3a]
SyncBookmarkDataTypeControllerTest_StartDependentsReady_Test::TestBody()
 [0x0000039500f3] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x000003947f6e] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x00000393fa35] testing::Test::Run()
 [0x00000394013b] testing::TestInfo::Run()
 [0x0000039406d7] testing::TestCase::Run()
 [0x000003944a05] testing::internal::UnitTestImpl::RunAllTests()
 [0x00000394d803] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x0000039493ce] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x0000039446f4] testing::UnitTest::Run()
 [0x00000452bfaf] base::TestSuite::Run()
 [0x0000044e61fd] content::UnitTestTestSuite::Run()
 [0x000004315266] main
 [0x7f9432b6aeff] __libc_start_main
 [0x0000005646f9] <unknown>
  r8: 00007fff8ecbff60  r9: 00007fff8ecbff40 r10: 00007fff8ecbff40 r11:
0000000000000246
 r12: 00000000005646d0 r13: 00007fff8ecc06b0 r14: 0000000000000000 r15:
0000000000000000
  di: 0000000000000000  si: 0000000000000001  bp: 00007fff8ecbff30  bx:
0000000000000000
  dx: 0000000000000020  ax: 000022026999a020  cx: 00000000017edd10  sp:
00007fff8ecbfe80
  ip: 00000000034a5213 efl: 0000000000010202 cgf: 0000000000000033 erf:
0000000000000004
 trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000000

[tfarina, 2013-02-06 00:53:34]

With patch set 18 I get this crash:

Received signal 11 SEGV_MAPERR 0000000001d0
 [0x7f2d82ef4ede] base::debug::StackTrace::StackTrace()
 [0x7f2d82ef4a15] base::debug::(anonymous namespace)::StackDumpSignalHandler()
 [0x7f2d782fdc60] <unknown>
 [0x0000023edf54] __gnu_cxx::__normal_iterator<>::__normal_iterator()
 [0x0000023ede1d] __gnu_cxx::__normal_iterator<>::__normal_iterator()
 [0x0000023edae4] std::vector<>::begin()
 [0x0000023e7b7d] ObserverListBase<>::AddObserver()
 [0x0000023e29cf] BookmarkModel::AddObserver()
 [0x000002b0df38] browser_sync::BookmarkDataTypeController::StartModels()
 [0x000002b20154] browser_sync::FrontendDataTypeController::LoadModels()
 [0x0000017f1d58] SyncBookmarkDataTypeControllerTest::Start()
 [0x0000017edef3]
SyncBookmarkDataTypeControllerTest_StartDependentsReady_Test::TestBody()
 [0x00000394ffd3] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x000003947e4e] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x00000393f915] testing::Test::Run()
 [0x00000394001b] testing::TestInfo::Run()
 [0x0000039405b7] testing::TestCase::Run()
 [0x0000039448e5] testing::internal::UnitTestImpl::RunAllTests()
 [0x00000394d6e3] testing::internal::HandleSehExceptionsInMethodIfSupported<>()
 [0x0000039492ae] testing::internal::HandleExceptionsInMethodIfSupported<>()
 [0x0000039445d4] testing::UnitTest::Run()
 [0x00000452bdaf] base::TestSuite::Run()
 [0x0000044e5ffd] content::UnitTestTestSuite::Run()
 [0x000004315076] main
 [0x7f2d752aaeff] __libc_start_main
 [0x0000005646f9] <unknown>
  r8: 00007fffcea67270  r9: 00007fffcea67250 r10: 00007fffcea672f0 r11:
0000000000000246
 r12: 00000000005646d0 r13: 00007fffcea68450 r14: 0000000000000000 r15:
0000000000000000
  di: 00000000000001d0  si: 00007fffcea67378  bp: 00007fffcea67350  bx:
0000000000000000
  dx: 00001908b9998230  ax: 00007fffcea67378  cx: 00007fffcea67538  sp:
00007fffcea67338
  ip: 00000000023edf54 efl: 0000000000010202 cgf: 0000000000000033 erf:
0000000000000004
 trp: 000000000000000e msk: 0000000000000000 cr2: 00000000000001d0

[tfarina, 2013-02-06 02:52:59]

If I just remove the lines of:

EXPECT_CALL(*bookmark_model_, IsLoaded()).WillRepeatedly(Return(true));

The test fails:

[ RUN      ] SyncBookmarkDataTypeControllerTest.StartDependentsReady

GMOCK WARNING:
Uninteresting mock function call - returning default value.
    Function call: IsLoaded()
          Returns: false
Stack trace:
[31561:31561:0206/005230:88707039950:FATAL:frontend_data_type_controller.cc(80)]
Check failed: state_ == MODEL_LOADED (1 vs. 2)
 [0x7f7e2660eede] base::debug::StackTrace::StackTrace()
 [0x7f7e2664aba2] logging::LogMessage::~LogMessage()
 [0x000002b2060b] browser_sync::FrontendDataTypeController::StartAssociating()
 [0x0000017f1e8d] SyncBookmarkDataTypeControllerTest::Start()
 [0x0000017edef3]
SyncBookmarkDataTypeControllerTest_StartDependentsReady_Test::TestBody()

wtf gmock is doing?!

[tfarina, 2013-02-06 03:23:36]

Patch set 19 does not crash, but...

[ RUN      ] SyncBookmarkDataTypeControllerTest.StartBookmarkModelNotReady
../../chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:158:
Failure
Value of: bookmark_dtc_->state()
  Actual: 2
Expected: DataTypeController::MODEL_STARTING
Which is: 1
[  FAILED  ] SyncBookmarkDataTypeControllerTest.StartBookmarkModelNotReady (43
ms)


And does not remove the notification. Since this is very complicated I think we
will need to focus on removing the inheritance first then the notification.

Nicolas, any ideas of how to fix StartBookmarkModelNotReady?

[tfarina, 2013-02-07 00:52:07]

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

https://codereview.chromium.org/12084058/diff/45005/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:56: return
new BookmarkModel(NULL);
this, of course, causes BookmarkModel to fire the Loaded event. Which for
StartBookmarkModelNotReady doesn't work. But works for all the other cases.

[tfarina, 2013-02-07 02:53:00]

Nicolas, crazy idea, is it too bad to just through away this
StartBookmarkModelNotReady? yeah, I mean, remove it! Wyt?

[Nicolas Zea, 2013-02-07 20:49:34]

On 2013/02/07 02:53:00, tfarina wrote:
> Nicolas, crazy idea, is it too bad to just through away this
> StartBookmarkModelNotReady? yeah, I mean, remove it! Wyt?

I'm not a fan of removing a valid test for the purposes of refactoring.

Scott, is subclassing BookmarkModel not supported because there are alternatives
to writing unit tests for other components that depend on the bookmark model? If
so, can you recommend any? 

Really, we just need a way to control when the BookmarkModel loads. It doesn't
appear that stubbing BookmarkStorage is viable, but perhaps there's something
else I'm not aware of?

[sky, 2013-02-11 17:26:43]

I don't like exposing a bunch of methods purely for subclassing and
use in a test. I would rather have a more direct route to exercise
what the test cares about.

  -Scott

On Thu, Feb 7, 2013 at 12:49 PM,  <zea@chromium.org> wrote:
> On 2013/02/07 02:53:00, tfarina wrote:
>>
>> Nicolas, crazy idea, is it too bad to just through away this
>> StartBookmarkModelNotReady? yeah, I mean, remove it! Wyt?
>
>
> I'm not a fan of removing a valid test for the purposes of refactoring.
>
> Scott, is subclassing BookmarkModel not supported because there are
> alternatives
> to writing unit tests for other components that depend on the bookmark
> model? If
> so, can you recommend any?
>
> Really, we just need a way to control when the BookmarkModel loads. It
> doesn't
> appear that stubbing BookmarkStorage is viable, but perhaps there's
> something
> else I'm not aware of?
>
> https://codereview.chromium.org/12084058/

[tfarina, 2013-02-15 13:42:42]

On 2013/02/06 03:23:36, tfarina wrote:
> Patch set 19 does not crash, but...
> 
> [ RUN      ] SyncBookmarkDataTypeControllerTest.StartBookmarkModelNotReady
> ../../chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:158:
> Failure
> Value of: bookmark_dtc_->state()
>   Actual: 2
> Expected: DataTypeController::MODEL_STARTING
> Which is: 1
> [  FAILED  ] 
SyncBookmarkDataTypeControllerTest.StartBookmarkModelNotReady (43
> ms)
> 
Looks like OnModelLoaded() is being called before LoadModels(). If
OnModelLoaded() is called before then it sets state to MODEL_LOADED, but we need
LoadModels() to be called first so state is equal to MODEL_STARTING.

Nicolas are you familiar with the control flow of this?

Not sure yet what is triggering OnModelLoaded untimely.

[tfarina, 2013-02-15 17:34:49]

Fred, are you familiar with the workflow of these DataTypeControllers, specially
between BookmarkDataTypeController and FrontendDataTypeController.

[tfarina, 2013-02-15 17:36:00]

On 2013/02/15 17:34:49, tfarina wrote:
> Fred, are you familiar with the workflow of these DataTypeControllers,
specially
> between BookmarkDataTypeController and FrontendDataTypeController.
?

If not, is there someone else that could help look at this?

[akalin, 2013-02-15 22:41:52]

I'm not too familiar with this code, sorry. Nicolas?

[Nicolas Zea, 2013-02-15 23:05:14]

Thiago, passing the NULL param to BookmarkModel defeats the purpose of this
test. The logic it's trying to verify is that the DataTypeController will wait
until the BookmarkModel is loaded. To do this, the BookmarkModel shouldn't have
loaded at the time the test begins.

One option to do this is to create our own fake BookmarkStorage implementation,
and pass that into the BookmarkModel somehow. That way we can directly control
when the bookmark model loads.

Another slightly hackier solution might be having a method that sets loaded_ to
false (either a new method or maybe repurposing ClearStore()), and then calling
DoneLoading() ourselves, via a friended class.

There might also be some alternatives I'm not thinking of too.

[tfarina, 2013-02-15 23:24:55]

On Fri, Feb 15, 2013 at 9:05 PM,  <zea@chromium.org> wrote:
> Thiago, passing the NULL param to BookmarkModel defeats the purpose of this
> test. The logic it's trying to verify is that the DataTypeController will
> wait
> until the BookmarkModel is loaded. To do this, the BookmarkModel shouldn't
> have
> loaded at the time the test begins.
>
I'll investigate more.

For a quick look there seems to be two ways of creating bookmark model for
test.

One:

BookmarkModel* bookmark_model = BookmarkModelFactory::GetForProfile(profile);
ui_test_utils::WaitForBookmarkModelToLoad(bookmark_model);

Other:
profile_->CreateBookmarkModel(true);
profile_->BlockUntilBookmarkModelLoaded();
BookmarkModel* bookmark_model =
BookmarkModelFactory::GetForProfile(profile_.get());

But so far I couldn't get neither to work.

> One option to do this is to create our own fake BookmarkStorage
> implementation,
> and pass that into the BookmarkModel somehow. That way we can directly
> control
> when the bookmark model loads.
>
Looks like this might be a solution, but I don't know if Scott will
like to inject that dependency on BookmarkModel.

> Another slightly hackier solution might be having a method that sets loaded_
> to
> false (either a new method or maybe repurposing ClearStore()), and then
> calling
> DoneLoading() ourselves, via a friended class.
>
I wouldn't be happy to make BookmarkDataTypeControllerTest or whatever
a friend of BookmarkModel just for the purpose of unittest. That
violates the encapsulation rule (IMO).

> There might also be some alternatives I'm not thinking of too.
>
> https://codereview.chromium.org/12084058/

-- 
Thiago

[tfarina, 2013-02-19 22:51:49]

Nicolas, I'm happy!

With the changes made in patch set 24 (ouch, sigh :/ ), all the tests PASSED!

I plan to go now and clean it up (without breaking it again!), but I'd like you
to look at it and see if it lgty.

Thanks for guiding me through this change!

Please, take a look!

[tfarina, 2013-02-19 22:52:35]

Oh also ignore the change I did in frontend_data_type_controller.cc, it was just
my debugging code.

[Nicolas Zea, 2013-02-19 22:58:09]

Looks pretty good, one style comment  mainly.

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

https://codereview.chromium.org/12084058/diff/65001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:79: void
Prepare() {
I think I'd prefer leaving this as SetUp, and just pulling the BookmarkModel
loading code into its own CreateBookmarkModel method, which you can call from
the tests. You can also get rid of load_model_ that way (have it be a param to
CreateBookmarkModel).

[tfarina, 2013-02-20 01:42:49]

Nicolas, I think this is ready, please, take another look.

Yeah, I notice I have to update the CL description, I plan to do this when I get
the green light ;)

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

https://codereview.chromium.org/12084058/diff/65001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:79: void
Prepare() {
On 2013/02/19 22:58:10, Nicolas Zea wrote:
> I think I'd prefer leaving this as SetUp, and just pulling the BookmarkModel
> loading code into its own CreateBookmarkModel method, which you can call from
> the tests.
Yeah, that was my first inclination but I was not sure if the order would
actually matters. It turns out, it does NOT.
:)

> You can also get rid of load_model_ that way (have it be a param to
> CreateBookmarkModel).
I was planning to do this as the clean up pass. I used an enum to make it clear
in the call sites, otherwise CreateBookmarkModel(false) will pass the idea that
we don't want to create the bookmark model.

[Nicolas Zea, 2013-02-20 23:37:16]

On 2013/02/20 01:42:49, tfarina wrote:
> Nicolas, I think this is ready, please, take another look.
> 
> Yeah, I notice I have to update the CL description, I plan to do this when I
get
> the green light ;)
> 
>
https://codereview.chromium.org/12084058/diff/65001/chrome/browser/sync/glue/...
> File chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc
(right):
> 
>
https://codereview.chromium.org/12084058/diff/65001/chrome/browser/sync/glue/...
> chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:79: void
> Prepare() {
> On 2013/02/19 22:58:10, Nicolas Zea wrote:
> > I think I'd prefer leaving this as SetUp, and just pulling the BookmarkModel
> > loading code into its own CreateBookmarkModel method, which you can call
from
> > the tests.
> Yeah, that was my first inclination but I was not sure if the order would
> actually matters. It turns out, it does NOT.
> :)
> 
> > You can also get rid of load_model_ that way (have it be a param to
> > CreateBookmarkModel).
> I was planning to do this as the clean up pass. I used an enum to make it
clear
> in the call sites, otherwise CreateBookmarkModel(false) will pass the idea
that
> we don't want to create the bookmark model.

Don't the bookmark_data_type_controller changes need to be here too?

[tfarina, 2013-02-24 22:02:10]

On 2013/02/20 23:37:16, Nicolas Zea wrote:
> Don't the bookmark_data_type_controller changes need to be here too?
Included bookmark_data_type_controller.* changes. Please, could you take another
look?

[Nicolas Zea, 2013-02-25 19:39:01]

LGTM with a naming nit. Thanks for sticking with it!

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

https://codereview.chromium.org/12084058/diff/74001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:93: enum
LoadModel {
nit: prefer calling this BookmarkLoadPolicy (LoadModel with value
DONT_LOAD_MODEL is a bit confusing)

https://codereview.chromium.org/12084058/diff/74001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:98: void
CreateBookmarkModel(LoadModel load_model) {
per above, load_model -> bookmark_load_policy

[tfarina, 2013-02-26 15:03:23]

On 2013/02/25 19:39:01, Nicolas Zea wrote:
> LGTM with a naming nit. Thanks for sticking with it!
> 
It was a long battle, but I was determined to win it! ;)

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

https://codereview.chromium.org/12084058/diff/74001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:93: enum
LoadModel {
On 2013/02/25 19:39:01, Nicolas Zea wrote:
> nit: prefer calling this BookmarkLoadPolicy (LoadModel with value
> DONT_LOAD_MODEL is a bit confusing)

Done.

https://codereview.chromium.org/12084058/diff/74001/chrome/browser/sync/glue/...
chrome/browser/sync/glue/bookmark_data_type_controller_unittest.cc:98: void
CreateBookmarkModel(LoadModel load_model) {
On 2013/02/25 19:39:01, Nicolas Zea wrote:
> per above, load_model -> bookmark_load_policy

Done.

[tfarina, 2013-02-26 15:04:02]

Committed manually as r184651 (presubmit successful).