Making bookmark folder context menu display the number of bookmarks that will be opened by Open All

chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc

Index: chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc
diff --git a/chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc b/chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc
index e80f31904d26a0f5088d37c087fe9d0875d19f63..1f0244bae2039bdd523a1d6cccd894566c530b88 100644
--- a/chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc
+++ b/chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc
@@ -58,91 +58,53 @@ using bookmarks::BookmarkNode;
 
 namespace chrome {
 
-int num_bookmark_urls_before_prompting = 15;
-
 namespace {
 
-// Iterator that iterates through a set of BookmarkNodes returning the URLs
-// for nodes that are urls, or the URLs for the children of non-url urls.
-// This does not recurse through all descendants, only immediate children.
-// The following illustrates
-// typical usage:
-// OpenURLIterator iterator(nodes);
-// while (iterator.has_next()) {
-//   const GURL* url = iterator.NextURL();
-//   // do something with |urll|.
-// }
-class OpenURLIterator {
- public:
-  explicit OpenURLIterator(const std::vector<const BookmarkNode*>& nodes)
-      : child_index_(0),
-        next_(NULL),
-        parent_(nodes.begin()),
-        end_(nodes.end()) {
-    FindNext();
-  }
-
-  bool has_next() { return next_ != NULL;}
-
-  const GURL* NextURL() {
-    if (!has_next()) {
-      NOTREACHED();
-      return NULL;
-    }
-
-    const GURL* next = next_;
-    FindNext();
-    return next;
-  }
-
- private:
-  // Seach next node which has URL.
-  void FindNext() {
-    for (; parent_ < end_; ++parent_, child_index_ = 0) {
-      if ((*parent_)->is_url()) {
-        next_ = &(*parent_)->url();
-        ++parent_;
-        child_index_ = 0;
-        return;
-      } else {
-        for (; child_index_ < (*parent_)->child_count(); ++child_index_) {
-          const BookmarkNode* child = (*parent_)->GetChild(child_index_);
-          if (child->is_url()) {
-            next_ = &child->url();
-            ++child_index_;
-            return;
-          }
+// Returns a vector of all URLs in |nodes| and their immediate children.  Only
+// recurses one level deep, not infinitely.  TODO(pkasting): It's not clear why
+// this shouldn't just recurse infinitely.
+std::vector<GURL> GetURLsToOpen(
+    const std::vector<const BookmarkNode*>& nodes,
+    content::BrowserContext* browser_context = nullptr,
+    bool incognito_urls_only = false) {
+  std::vector<GURL> urls;
+
+  const auto AddUrlIfLegal = [&](const GURL url) {
+    if (!incognito_urls_only || IsURLAllowedInIncognito(url, browser_context))
+      urls.push_back(url);
+  };
+
+  for (const BookmarkNode* node : nodes) {
+    if (node->is_url()) {
+      AddUrlIfLegal(node->url());
+    } else {
+      // If the node is not a URL, it is a folder. We want to add those of its
+      // children which are URLs.
+      for (int child_index = 0; child_index < node->child_count();
+           ++child_index) {
+        const BookmarkNode* child = node->GetChild(child_index);
+        if (child->is_url()) {

[Peter Kasting, 2017/04/14 20:41:53]

Nit: No {}

[Paezagon, 2017/04/14 23:32:56]

Done.

+          AddUrlIfLegal(child->url());
         }
       }
     }
-    next_ = NULL;
   }
-
-  int child_index_;
-  const GURL* next_;
-  std::vector<const BookmarkNode*>::const_iterator parent_;
-  const std::vector<const BookmarkNode*>::const_iterator end_;
-
-  DISALLOW_COPY_AND_ASSIGN(OpenURLIterator);
-};
+  return urls;
+}
 
 #if !defined(OS_ANDROID)
 bool ShouldOpenAll(gfx::NativeWindow parent,
                    const std::vector<const BookmarkNode*>& nodes) {
-  int child_count = 0;
-  OpenURLIterator iterator(nodes);
-  while (iterator.has_next()) {
-    iterator.NextURL();
-    child_count++;
-  }
+  constexpr size_t kNumBookmarkUrlsBeforePrompting = 15;
 
-  if (child_count < num_bookmark_urls_before_prompting)
+  size_t child_count = GetURLsToOpen(nodes).size();
+  if (child_count < kNumBookmarkUrlsBeforePrompting)
     return true;
 
   return ShowQuestionMessageBox(
              parent, l10n_util::GetStringUTF16(IDS_PRODUCT_NAME),
              l10n_util::GetStringFUTF16(IDS_BOOKMARK_BAR_SHOULD_OPEN_ALL,
-                                        base::IntToString16(child_count))) ==
+                                        base::SizeTToString16(child_count))) ==
          MESSAGE_BOX_RESULT_YES;
 }
 #endif
@@ -185,34 +147,27 @@ void OpenAll(gfx::NativeWindow parent,
 
   // Opens all |nodes| of type URL and any children of |nodes| that are of type
   // URL. |navigator| is the PageNavigator used to open URLs. After the first
-  // url is opened |opened_first_url| is set to true and |navigator| is set to
-  // the PageNavigator of the last active tab. This is done to handle a window
-  // disposition of new window, in which case we want subsequent tabs to open in
-  // that window.
-  bool opened_first_url = false;
-  WindowOpenDisposition disposition = initial_disposition;
-  OpenURLIterator iterator(nodes);
-  while (iterator.has_next()) {
-    const GURL* url = iterator.NextURL();
-    // When |initial_disposition| is OFF_THE_RECORD, a node which can't be
-    // opened in incognito window, it is detected using |browser_context|, is
-    // not opened.
-    if (initial_disposition == WindowOpenDisposition::OFF_THE_RECORD &&
-        !IsURLAllowedInIncognito(*url, browser_context))
-      continue;
+  // url is opened |navigator| is set to the PageNavigator of the last active
+  // tab. This is done to handle a window disposition of new window, in which
+  // case we want subsequent tabs to open in that window.
 
+  std::vector<GURL> urls = GetURLsToOpen(
+      nodes, browser_context,
+      initial_disposition == WindowOpenDisposition::OFF_THE_RECORD);
+
+  WindowOpenDisposition disposition = initial_disposition;
+  for (std::vector<GURL>::const_iterator url_it = urls.begin();
+       url_it != urls.end(); ++url_it) {
     content::WebContents* opened_tab = navigator->OpenURL(
-        content::OpenURLParams(*url, content::Referrer(), disposition,
+        content::OpenURLParams(*url_it, content::Referrer(), disposition,
                                ui::PAGE_TRANSITION_AUTO_BOOKMARK, false));
-
-    if (!opened_first_url) {
-      opened_first_url = true;
-      disposition = WindowOpenDisposition::NEW_BACKGROUND_TAB;
+    if (url_it == urls.begin()) {
       // We opened the first URL which may have opened a new window or clobbered
       // the current page, reset the navigator just to be sure. |opened_tab| may
-      // be NULL in tests.
+      // be nullptr in tests.

[Peter Kasting, 2017/04/14 20:41:53]

Nit: Just "null" in comments

[Paezagon, 2017/04/14 23:32:57]

Done.

       if (opened_tab)
         navigator = opened_tab;
+      disposition = WindowOpenDisposition::NEW_BACKGROUND_TAB;
     }
   }
 }
@@ -224,7 +179,24 @@ void OpenAll(gfx::NativeWindow parent,
              content::BrowserContext* browser_context) {
   std::vector<const BookmarkNode*> nodes;
   nodes.push_back(node);
-  OpenAll(parent, navigator, nodes, initial_disposition, browser_context);
+  OpenAll(parent, navigator, std::vector<const bookmarks::BookmarkNode*>{node},
+          initial_disposition, browser_context);
+}
+
+int OpenCount(gfx::NativeWindow parent,
+              const std::vector<const bookmarks::BookmarkNode*>& nodes,
+              content::BrowserContext* incognito_context) {
+  return GetURLsToOpen(nodes, incognito_context, incognito_context != nullptr)
+      .size();
+}
+
+int OpenCount(gfx::NativeWindow parent,
+              const BookmarkNode* node,
+              content::BrowserContext* incognito_context) {
+  std::vector<const BookmarkNode*> nodes;
+  nodes.push_back(node);
+  return OpenCount(parent, std::vector<const bookmarks::BookmarkNode*>{node},
+                   incognito_context);
 }
 
 bool ConfirmDeleteBookmarkNode(const BookmarkNode* node,
@@ -254,20 +226,13 @@ void ShowBookmarkAllTabsDialog(Browser* browser) {
 }
 
 bool HasBookmarkURLs(const std::vector<const BookmarkNode*>& selection) {
-  OpenURLIterator iterator(selection);
-  return iterator.has_next();
+  return !GetURLsToOpen(selection).empty();
 }
 
 bool HasBookmarkURLsAllowedInIncognitoMode(
     const std::vector<const BookmarkNode*>& selection,
     content::BrowserContext* browser_context) {
-  OpenURLIterator iterator(selection);
-  while (iterator.has_next()) {
-    const GURL* url = iterator.NextURL();
-    if (IsURLAllowedInIncognito(*url, browser_context))
-      return true;
-  }
-  return false;
+  return !GetURLsToOpen(selection, browser_context, true).empty();
 }
 #endif  // !defined(OS_ANDROID)