Refactor history menu tests for clearer setup

chrome/browser/ui/cocoa/history_menu_bridge_unittest.mm

 // 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/cocoa/history_menu_bridge.h"
 
 #import <Cocoa/Cocoa.h>
 
+#include <initializer_list>
 #include <memory>
 #include <vector>
 
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted_memory.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/sessions/chrome_tab_restore_service_client.h"
@@ skipping 70 matching lines @@
   }
 
   HistoryMenuBridge::HistoryItem* CreateItem(const base::string16& title) {
     HistoryMenuBridge::HistoryItem* item =
         new HistoryMenuBridge::HistoryItem();
     item->title = title;
     item->url = GURL(title);
     return item;
   }
 
-  MockTRS::Tab CreateSessionTab(const std::string& url,
-                                const std::string& title) {
-    MockTRS::Tab tab;
-    tab.current_navigation_index = 0;
-    tab.navigations.push_back(
-        sessions::SerializedNavigationEntryTestHelper::CreateNavigation(
-            url, title));
+  MockTRS::Entries CreateSessionEntries(

[Robert Sesek, 2016/08/02 18:47:38]

Rather than this being a "Create" function, this could be called with a
vector<unique_ptr<MockTRS::Entry>> and return the unwrapped version instead.
That removes the weird ownership dance.

Something like this in code:

std::vector<std::unique_ptr<MockTRS::Entry>> owned_entries{
  CreateSessionTab(…),
  CreateSessionWindow(…),
};
MockTRS::Entry entries = UnwrapEntries(owned_entries);

+      std::vector<std::unique_ptr<MockTRS::Entry>>* out,
+      std::initializer_list<MockTRS::Entry*> l) {

[Robert Sesek, 2016/08/02 18:47:38]

naming: something more descriptive than |l|

[Sidney San Martín, 2016/08/02 19:47:34]

Sure. I picked `l` because I remember seeing it as a convention in example code
but, now that I look again, it may not be.

+    MockTRS::Entries ret;
+    out->reserve(out->size() + l.size());
+    for (auto* entry : l) {
+      ret.emplace_back(entry);
+      out->emplace_back(entry);
+    }
+    return ret;
+  }
+
+  MockTRS::Tab* CreateSessionTab(SessionID::id_type id,

[Robert Sesek, 2016/08/02 18:47:38]

This could return a unique_ptr<MockTRS::Tab>.

+                                 const std::string& url,
+                                 const std::string& title) {
+    auto tab = new MockTRS::Tab;
+    tab->id = id;
+    tab->current_navigation_index = 0;
+    tab->navigations.push_back(
+        sessions::SerializedNavigationEntryTestHelper::CreateNavigation(url,
+                                                                        title));
     return tab;
   }
 
+  MockTRS::Window* CreateSessionWindow(
+      SessionID::id_type id,
+      std::initializer_list<MockTRS::Tab*> tabs) {
+    auto window = new MockTRS::Window;
+    window->id = id;
+    window->tabs.reserve(tabs.size());
+    for (auto* tab : tabs) {
+      window->tabs.emplace_back(std::move(*tab));

[Robert Sesek, 2016/08/02 18:47:38]

This line is very hard to scrutinize (I've read it for five minutes). I guess
you're moving the underlying Tab, but does that mean |tab| is then inconsistent
after this, but that's OK because you immediately delete it?

+      delete tab;

[Robert Sesek, 2016/08/02 18:47:38]

... so that you don't need to manually delete here.

+    }
+    return window;
+  }
+
   void GetFaviconForHistoryItem(HistoryMenuBridge::HistoryItem* item) {
     bridge_->GetFaviconForHistoryItem(item);
   }
 
   void GotFaviconData(HistoryMenuBridge::HistoryItem* item,
                       const favicon_base::FaviconImageResult& image_result) {
     bridge_->GotFaviconData(item, image_result);
   }
 
   std::unique_ptr<MockBridge> bridge_;
@@ skipping 87 matching lines @@
   // Confirm tooltips and confirm they are not trimmed (like the item
   // name might be).  Add tolerance for URL fixer-upping;
   // e.g. http://foo becomes http://foo/)
   EXPECT_GE([[[menu itemAtIndex:0] toolTip] length], (2*short_url.length()-5));
   EXPECT_GE([[[menu itemAtIndex:1] toolTip] length], (2*long_url.length()-5));
 }
 
 // Test that the menu is created for a set of simple tabs.
 TEST_F(HistoryMenuBridgeTest, RecentlyClosedTabs) {
   std::unique_ptr<MockTRS> trs(new MockTRS(profile()));
-  MockTRS::Entries entries;
-
-  MockTRS::Tab tab1 = CreateSessionTab("http://google.com", "Google");
-  tab1.id = 24;
-  entries.push_back(&tab1);
-
-  MockTRS::Tab tab2 = CreateSessionTab("http://apple.com", "Apple");
-  tab2.id = 42;
-  entries.push_back(&tab2);
+  std::vector<std::unique_ptr<MockTRS::Entry>> holdEntries;
+  auto entries{CreateSessionEntries(&holdEntries, {
+    CreateSessionTab(24, "http://google.com", "Google"),
+    CreateSessionTab(42, "http://apple.com", "Apple"),
+  })};
 
   using ::testing::ReturnRef;
   EXPECT_CALL(*trs.get(), entries()).WillOnce(ReturnRef(entries));
 
   bridge_->TabRestoreServiceChanged(trs.get());
 
   NSMenu* menu = bridge_->HistoryMenu();
   ASSERT_EQ(2U, [[menu itemArray] count]);
 
   NSMenuItem* item1 = [menu itemAtIndex:0];
   MockBridge::HistoryItem* hist1 = bridge_->HistoryItemForMenuItem(item1);
   EXPECT_TRUE(hist1);
   EXPECT_EQ(24, hist1->session_id);
   EXPECT_NSEQ(@"Google", [item1 title]);
 
   NSMenuItem* item2 = [menu itemAtIndex:1];
   MockBridge::HistoryItem* hist2 = bridge_->HistoryItemForMenuItem(item2);
   EXPECT_TRUE(hist2);
   EXPECT_EQ(42, hist2->session_id);
   EXPECT_NSEQ(@"Apple", [item2 title]);
 }
 
 // Test that the menu is created for a mix of windows and tabs.
 TEST_F(HistoryMenuBridgeTest, RecentlyClosedTabsAndWindows) {
   std::unique_ptr<MockTRS> trs(new MockTRS(profile()));
-  MockTRS::Entries entries;
-
-  MockTRS::Tab tab1 = CreateSessionTab("http://google.com", "Google");
-  tab1.id = 24;
-  entries.push_back(&tab1);
-
-  // TODO(sdy): The tab ids below *must* be set after all of them have been
-  // pushed onto window.tabs. Otherwise, the ids will change when push_back
-  // reallocates the vector's storage and calls each tabs' copy ctor. Ugh.
-
-  MockTRS::Window win1;
-  win1.id = 30;
-  win1.tabs.push_back(CreateSessionTab("http://foo.com", "foo"));
-  win1.tabs.push_back(CreateSessionTab("http://bar.com", "bar"));
-  win1.tabs[0].id = 31;
-  win1.tabs[1].id = 32;
-  entries.push_back(&win1);
-
-  MockTRS::Tab tab2 = CreateSessionTab("http://apple.com", "Apple");
-  tab2.id = 42;
-  entries.push_back(&tab2);
-
-  MockTRS::Window win2;
-  win2.id = 50;
-  win2.tabs.push_back(CreateSessionTab("http://magic.com", "magic"));
-  win2.tabs.push_back(CreateSessionTab("http://goats.com", "goats"));
-  win2.tabs.push_back(CreateSessionTab("http://teleporter.com", "teleporter"));
-  win2.tabs[0].id = 51;
-  win2.tabs[1].id = 52;
-  win2.tabs[2].id = 53;
-  entries.push_back(&win2);
+  std::vector<std::unique_ptr<MockTRS::Entry>> holdEntries;

[Robert Sesek, 2016/08/02 18:47:38]

naming: hold_entries

[Sidney San Martín, 2016/08/02 19:47:34]

Done.

+  auto entries{CreateSessionEntries(&holdEntries, {
+    CreateSessionTab(24, "http://google.com", "Google"),
+    CreateSessionWindow(30, {
+      CreateSessionTab(31, "http://foo.com", "foo"),
+      CreateSessionTab(32, "http://bar.com", "bar"),
+    }),
+    CreateSessionTab(42, "http://apple.com", "Apple"),
+    CreateSessionWindow(50, {
+      CreateSessionTab(51, "http://magic.com", "magic"),
+      CreateSessionTab(52, "http://goats.com", "goats"),
+      CreateSessionTab(53, "http://teleporter.com", "teleporter"),
+    }),
+  })};
 
   using ::testing::ReturnRef;
   EXPECT_CALL(*trs.get(), entries()).WillOnce(ReturnRef(entries));
 
   bridge_->TabRestoreServiceChanged(trs.get());
 
   NSMenu* menu = bridge_->HistoryMenu();
   ASSERT_EQ(4U, [[menu itemArray] count]);
 
   NSMenuItem* item1 = [menu itemAtIndex:0];
@@ skipping 72 matching lines @@
   image_result.image = gfx::Image::CreateFrom1xBitmap(bitmap);
   GotFaviconData(&item, image_result);
 
   // Make sure the callback works.
   EXPECT_FALSE(item.icon_requested);
   EXPECT_TRUE(item.icon.get());
   EXPECT_TRUE([item.menu_item image]);
 }
 
 }  // namespace