Broke out layout metric information from ViewsDelegate to LayoutProvider

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/bookmarks/bookmark_editor_view.h"
 
 #include <string>
 
 #include "base/strings/string_util.h"
 #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"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/views/controls/textfield/textfield.h"
 #include "ui/views/controls/tree/tree_view.h"
 #include "ui/views/test/test_views_delegate.h"
 
 using base::ASCIIToUTF16;
 using base::UTF8ToUTF16;
 using base::Time;
 using base::TimeDelta;
 using bookmarks::BookmarkModel;
 using bookmarks::BookmarkNode;
 
 // Base class for bookmark editor tests. Creates a BookmarkModel and populates
 // it with test data.
 class BookmarkEditorViewTest : public testing::Test {
  public:
   BookmarkEditorViewTest() : model_(nullptr) {}
 
   void SetUp() override {
     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_);
 
     AddTestData();
   }
 
  protected:
   std::string base_path() const { return "file:///c:/tmp/"; }
 
   const BookmarkNode* GetNode(const std::string& name) {
@@ skipping 396 matching lines @@
   ASSERT_TRUE(tree_view()->editor() != NULL);
   tree_view()->editor()->SetText(ASCIIToUTF16("modified"));
   ApplyEdits();
 
   // Verify the new folder was added and title set appropriately.
   ASSERT_EQ(1, parent->child_count());
   const BookmarkNode* new_folder = parent->GetChild(0);
   ASSERT_TRUE(new_folder->is_folder());
   EXPECT_EQ("modified", base::UTF16ToASCII(new_folder->GetTitle()));
 }