Broke out layout metric information from ViewsDelegate to LayoutProvider

chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc

Index: chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
diff --git a/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc b/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
index ef959af3e7003bdefa91940f54e241dc5d3fb8d1..8a90c0668a75ab2ccc4a3d3f6c0b7489735d072e 100644
--- a/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
+++ b/chrome/browser/ui/views/bookmarks/bookmark_editor_view_unittest.cc
@@ -10,6 +10,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/views/harmony/chrome_views_layout_delegate.h"
 #include "chrome/test/base/testing_profile.h"
 #include "components/bookmarks/browser/bookmark_model.h"
 #include "components/bookmarks/test/bookmark_test_helpers.h"
@@ -36,6 +37,10 @@ class BookmarkEditorViewTest : public testing::Test {
     profile_.reset(new TestingProfile());
     profile_->CreateBookmarkModel(true);
 
+    views_delegate_.set_layout_delegate(nullptr);

[Peter Kasting, 2017/03/24 22:51:13]

Why is this line needed?  Same comment elsewhere this pattern appears.

[kylix_rd, 2017/03/27 18:18:27]

Since these delegates are singletons and they set/clear the one global variable,
constructing a new instance while there may be one already constructed, (ie. the
default ViewsLayoutDelegate constructed by the views_delegate_ instance) would
cause the global to be set back to when the old delegate is destroyed. This
would lose the global delegate.

[Peter Kasting, 2017/03/27 20:06:48]

This sounds like the API is easy to subtly use wrong.

It seems like either we should remove setters, and if you want to set a new
delegate, you do it by constructing one, so the code here would just construct a
ChromeViewsLayoutDelegate and that would auto-set the global; or, we should not
have constructing these delegates auto-set any globals, and everywhere that
makes one should do that manually if they want to update the global.  The first
route would be harder to misuse.

[kylix_rd, 2017/03/29 18:56:04]

Short of imposing more conditions on its use, I'm not sure of a better solution.
This is really only a problem with unit tests. In this instance, the
TestViewsDelegate only references ui/views, so constructing a corresponding
ViewsLayoutDelegate is the only one visible. Some tests don't need a
ChromeViewsLayoutDelegate to be present since c/b/ui/views aren't exercised or
referenced. Other tests do directly or indirectly reference views from
c/b/ui/views, which may require the presence of a ChromeViewsLayoutDelgate
instance.
 
One approach may be to implement a "last constructed wins" model. Change the
constructor(s) to blindly set the global and declare the global(s) as a
std::weak_ptr<>. In all the cases I've seen so far there is always at least a
std::unique_ptr<> reference or it is constructed in-place within another object
instance that is explicitly life-time managed. There would still need to be a
setter in this case, but there would no longer be any interleaved setting of the
global.

+    views_delegate_.set_layout_delegate(
+        base::MakeUnique<ChromeViewsLayoutDelegate>());
+
     model_ = BookmarkModelFactory::GetForBrowserContext(profile_.get());
     bookmarks::test::WaitForBookmarkModelToLoad(model_);