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..b1dd6c598673baeff25c41627d70fd20a397b0d5 100644
--- a/chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc
+++ b/chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc
@@ -62,80 +62,48 @@ 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;
-          }
+// Iterate through a set of nodes, adding all the URLs to a vector, and for the
+// nodes that are not URLs, add their child URLs.

[Peter Kasting, 2017/04/14 08:17:15]

Nit: How about "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."

[Paezagon, 2017/04/14 18:47:24]

Done.

+std::vector<const GURL*> GetOpenURLs(

[Peter Kasting, 2017/04/14 08:17:15]

Why does this vector contain GURL* instead of GURL?  That seems like it just
makes everything harder to deal with.

Don't worry about the cost of copying GURLs.

Nit: "Open" in the function name is misleading, because the URLs may or may not
be open.  GetURLsToOpen() would be better.

[Paezagon, 2017/04/14 18:47:24]

Done.

+    const std::vector<const BookmarkNode*>& nodes) {
+  std::vector<const GURL*> urls;
+  for (std::vector<const BookmarkNode*>::const_iterator nodes_it =
+           nodes.begin();
+       nodes_it != nodes.end(); ++nodes_it) {

[Peter Kasting, 2017/04/14 08:17:15]

Nit: Use range-based for, which will simplify both the loop declaration and body
syntax in this case

[Paezagon, 2017/04/14 18:47:24]

Done.

+    if ((*nodes_it)->is_url()) {
+      // if node is URL add it

[Peter Kasting, 2017/04/14 08:17:15]

Nit: Comments should be complete sentences, with capitalization, punctuation,
and good grammar.

In this case, the comment is unnecessary, because it adds no real information to
what the code already says.

[Paezagon, 2017/04/14 18:47:24]

Done.

+      urls.push_back(&(*nodes_it)->url());
+    } else {
+      // if node is not URL, it's a folder, add it's children that are URLs
+      for (int child_index = 0; child_index < (*nodes_it)->child_count();
+           ++child_index) {

[Peter Kasting, 2017/04/14 08:17:15]

Nit: Again, use range-based for

[Paezagon, 2017/04/14 18:47:24]

Can't use range for here because BookmarkNode's children are not iterable. We
could update BookmarkNode to make them iterable, but that would make more sense
to do in a separate CL. 

[Peter Kasting, 2017/04/14 19:04:40]

That might indeed make sense to do.  I'm fine with doing it separately.

+        const BookmarkNode* child = (*nodes_it)->GetChild(child_index);
+        if (child->is_url()) {
+          urls.push_back(&child->url());
         }
       }
     }
-    next_ = NULL;
   }
+  return urls;
+}
 
-  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);
-};
+std::vector<const GURL*> GetOpenURLsIncognito(
+    const std::vector<const BookmarkNode*>& nodes,
+    content::BrowserContext* browser_context) {
+  std::vector<const GURL*> urls = GetOpenURLs(nodes);
+  urls.erase(std::remove_if(urls.begin(), urls.end(),

[Peter Kasting, 2017/04/14 08:17:15]

Nit: Prefer base::EraseIf() to erase(remove_if())

[Paezagon, 2017/04/14 18:47:24]

Done.

+                            [&browser_context](const GURL* url) {
+                              return !IsURLAllowedInIncognito(*url,
+                                                              browser_context);
+                            }),
+             urls.end());
+  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++;
-  }
-
+  int child_count = GetOpenURLs(nodes).size();

[Peter Kasting, 2017/04/14 08:17:15]

Nit: Declare as size_t, change |num_bookmark_urls_before_prompting| to size_t
(and rename kNumBookmarkUrlsBeforePrompting, and move into this function),
change IntToString16() call below to SizeTToString16()

[Paezagon, 2017/04/14 18:47:24]

Done.

   if (child_count < num_bookmark_urls_before_prompting)
     return true;
 
@@ -189,31 +157,31 @@ void OpenAll(gfx::NativeWindow parent,
   // 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;
-
-    content::WebContents* opened_tab = navigator->OpenURL(
-        content::OpenURLParams(*url, content::Referrer(), disposition,
-                               ui::PAGE_TRANSITION_AUTO_BOOKMARK, false));
 
-    if (!opened_first_url) {
-      opened_first_url = true;
-      disposition = WindowOpenDisposition::NEW_BACKGROUND_TAB;
-      // 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.
-      if (opened_tab)
-        navigator = opened_tab;
-    }
+  std::vector<const GURL*> urls =
+      initial_disposition == WindowOpenDisposition::OFF_THE_RECORD
+          ? GetOpenURLsIncognito(nodes, browser_context)
+          : GetOpenURLs(nodes);

[Peter Kasting, 2017/04/14 08:17:15]

Nit: I suggest collapsing GetOpenURLs[InIncognito]() into one function that
takes args about whether to remove things not allowed in incognito.  That would
simplify multiple callers.

[Paezagon, 2017/04/14 18:47:24]

Done.

+
+  std::vector<const GURL*>::const_iterator url_it = urls.begin();
+
+  content::WebContents* opened_tab = navigator->OpenURL(
+      content::OpenURLParams(**url_it, content::Referrer(), initial_disposition,
+                             ui::PAGE_TRANSITION_AUTO_BOOKMARK, false));

[Peter Kasting, 2017/04/14 08:17:15]

Nit: The old loop-with-bool was ugly, but duplicating this OpenURL() call is
also ugly.  What about a single loop, and instead of |opened_first_url|, we just
check if the loop iterator == urls.begin()?

[Paezagon, 2017/04/14 18:47:24]

Done.

+
+  url_it++;

[Peter Kasting, 2017/04/14 08:17:15]

Nit: Preincrement iterators

[Paezagon, 2017/04/14 18:47:24]

Gone.

+
+  // 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.
+  if (opened_tab)
+    navigator = opened_tab;
+
+  WindowOpenDisposition disposition = WindowOpenDisposition::NEW_BACKGROUND_TAB;
+  for (; url_it != urls.end(); ++url_it) {
+    navigator->OpenURL(
+        content::OpenURLParams(**url_it, content::Referrer(), disposition,
+                               ui::PAGE_TRANSITION_AUTO_BOOKMARK, false));
   }
 }
 
@@ -227,6 +195,24 @@ void OpenAll(gfx::NativeWindow parent,
   OpenAll(parent, navigator, nodes, initial_disposition, browser_context);
 }
 
+int OpenCount(gfx::NativeWindow parent,
+              const std::vector<const bookmarks::BookmarkNode*>& nodes,
+              bool incognito,
+              content::BrowserContext* browser_context) {
+  return (incognito ? GetOpenURLsIncognito(nodes, browser_context)
+                    : GetOpenURLs(nodes))
+      .size();
+}
+
+int OpenCount(gfx::NativeWindow parent,
+              const BookmarkNode* node,
+              bool incognito,
+              content::BrowserContext* browser_context) {
+  std::vector<const BookmarkNode*> nodes;
+  nodes.push_back(node);
+  return OpenCount(parent, nodes, incognito, browser_context);
+}
+
 bool ConfirmDeleteBookmarkNode(const BookmarkNode* node,
                                gfx::NativeWindow window) {
   DCHECK(node && node->is_folder() && !node->empty());
@@ -254,20 +240,13 @@ void ShowBookmarkAllTabsDialog(Browser* browser) {
 }
 
 bool HasBookmarkURLs(const std::vector<const BookmarkNode*>& selection) {
-  OpenURLIterator iterator(selection);
-  return iterator.has_next();
+  return GetOpenURLs(selection).size() > 0;

[Peter Kasting, 2017/04/14 08:17:15]

Nit: Prefer !empty() to size() > 0 (2 places)

[Paezagon, 2017/04/14 18:47:24]

Done.

 }
 
 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 GetOpenURLsIncognito(selection, browser_context).size() > 0;
 }
 #endif  // !defined(OS_ANDROID)