Restructure the wrench menu into an action based order

chrome/browser/ui/toolbar/wrench_menu_model_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 <stdio.h>

[Peter Kasting, 2015/03/30 19:37:02]

Why add this?

[edwardjung, 2015/03/31 11:44:06]

Removed, debug.

 
 #include "chrome/browser/ui/toolbar/wrench_menu_model.h"
 
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/prefs/browser_prefs.h"
 #include "chrome/browser/signin/profile_oauth2_token_service_factory.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/global_error/global_error.h"
 #include "chrome/browser/ui/global_error/global_error_service.h"
 #include "chrome/browser/ui/global_error/global_error_service_factory.h"
@@ skipping 110 matching lines @@
   // Verify it has items. The number varies by platform, so we don't check
   // the exact number.
   EXPECT_GT(itemCount, 10);
 
   // Execute a couple of the items and make sure it gets back to our delegate.
   // We can't use CountEnabledExecutable() here because the encoding menu's
   // delegate is internal, it doesn't use the one we pass in.
   // Note: The new menu has a spacing separator at the first slot.
   model.ActivatedAt(1);
   EXPECT_TRUE(model.IsEnabledAt(1));
   // Make sure to use the index that is not separator in all configurations.

[Peter Kasting, 2015/03/30 19:37:02]

Is there an index that is a non-separator in all platforms, like there was in
the old code, so you can avoid the next #ifdef?

[edwardjung, 2015/03/31 11:44:05]

Let me find something that works for all. i tried a number of options which
failed one platform or another. 

+#if defined(OS_MACOSX) or defined(CHROMEOS)

[Peter Kasting, 2015/03/30 19:37:02]

"or" needs to be ||, this won't work as you have it

[edwardjung, 2015/03/31 11:44:05]

Done.

   model.ActivatedAt(2);
   EXPECT_TRUE(model.IsEnabledAt(2));
+#else
+  model.ActivatedAt(3);
+  EXPECT_TRUE(model.IsEnabledAt(3));
+#endif
   EXPECT_EQ(model.execute_count_, 2);
   EXPECT_EQ(model.enable_count_, 2);
 
   model.execute_count_ = 0;
   model.enable_count_ = 0;
 
   // Choose something from the bookmark submenu and make sure it makes it back
   // to the delegate as well.
   int bookmarksModelIndex = -1;
   for (int i = 0; i < itemCount; ++i) {
     if (model.GetTypeAt(i) == ui::MenuModel::TYPE_SUBMENU) {
-      bookmarksModelIndex = i;
+      // Bookmarks is now under the tabs submenu item.

[Peter Kasting, 2015/03/30 19:37:02]

What does "now" mean?  This seems to be implicitly referring to some previous
state, which readers in the future won't be aware of.  Make sure your comments
are contextually self-contained.

[edwardjung, 2015/03/31 11:44:05]

Done.

+      bookmarksModelIndex = i + 1;
       break;
     }
   }
   EXPECT_GT(bookmarksModelIndex, -1);
   ui::MenuModel* bookmarksModel = model.GetSubmenuModelAt(bookmarksModelIndex);
   EXPECT_TRUE(bookmarksModel);
   // The bookmarks model may be empty until we tell it we're going to show it.
   bookmarksModel->MenuWillShow();
   EXPECT_GT(bookmarksModel->GetItemCount(), 1);
-  bookmarksModel->ActivatedAt(1);
-  EXPECT_TRUE(bookmarksModel->IsEnabledAt(1));
+
+  // Bookmark manager item.
+  bookmarksModel->ActivatedAt(4);
+  EXPECT_TRUE(bookmarksModel->IsEnabledAt(4));
   EXPECT_EQ(model.execute_count_, 1);
   EXPECT_EQ(model.enable_count_, 1);
 }
 
 // Tests global error menu items in the wrench menu.
 TEST_F(WrenchMenuModelTest, GlobalError) {
   // Make sure services required for tests are initialized.
   GlobalErrorService* service =
       GlobalErrorServiceFactory::GetForProfile(browser()->profile());
   ProfileOAuth2TokenServiceFactory::GetForProfile(browser()->profile());
@@ skipping 25 matching lines @@
 
 class EncodingMenuModelTest : public BrowserWithTestWindowTest,
                               public MenuModelTest {
 };
 
 TEST_F(EncodingMenuModelTest, IsCommandIdCheckedWithNoTabs) {
   EncodingMenuModel model(browser());
   ASSERT_EQ(NULL, browser()->tab_strip_model()->GetActiveWebContents());
   EXPECT_FALSE(model.IsCommandIdChecked(IDC_ENCODING_WINDOWS1252));
 }