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

 // Copyright 2015 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/bookmarks/bookmark_utils_desktop.h"
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "build/build_config.h"
@@ skipping 40 matching lines @@
 #include "chrome/grit/theme_resources.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle.h"
 #endif
 
 using bookmarks::BookmarkModel;
 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;
+  for (const BookmarkNode* node : nodes) {
+    if (node->is_url() and

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

Hey now, this is C++, not Pascal :)

[Paezagon, 2017/04/14 19:52:35]

Eheh. I tend to get confused when switching langues... Weee Python + Javascript
+ Java => C++

+        (!incognito_urls_only or
+         IsURLAllowedInIncognito(node->url(), browser_context))) {

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

Nit: You could define a lambda locally:

  const auto AddNodeIfLegalUrl = [&](const BookmarkNode* node) {
      if (node->is_url() &&
          (!incognito_urls_only ||
           IsURLAllowedInIncognito(node->url(), browser_context)))
        urls->push_back(node->url());
  }

(Not sure the indenting here is right)

Then you can use this to avoid the duplication here/below.

[Paezagon, 2017/04/14 19:52:35]

Done with couple alterations.

+      urls.push_back(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() and
+            (!incognito_urls_only or
+             IsURLAllowedInIncognito(child->url(), browser_context))) {
+          urls.push_back(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++;
-  }
+  const size_t kNumBookmarkUrlsBeforePrompting = 15;

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

Nit: Prefer constexpr to const for compile-time constants

[Paezagon, 2017/04/14 19:52:35]

Done.

 
-  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
 
 // Returns the total number of descendants nodes.
 int ChildURLCountTotal(const BookmarkNode* node) {
   int result = 0;
   for (int i = 0; i < node->child_count(); ++i) {
     const BookmarkNode* child = node->GetChild(i);
     result++;
@@ skipping 22 matching lines @@
 void OpenAll(gfx::NativeWindow parent,
              content::PageNavigator* navigator,
              const std::vector<const BookmarkNode*>& nodes,
              WindowOpenDisposition initial_disposition,
              content::BrowserContext* browser_context) {
   if (!ShouldOpenAll(parent, nodes))
     return;
 
   // 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;
+  // 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;
-  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;
-
+  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.
       if (opened_tab)
         navigator = opened_tab;
+      disposition = WindowOpenDisposition::NEW_BACKGROUND_TAB;
     }
   }
 }
 
 void OpenAll(gfx::NativeWindow parent,
              content::PageNavigator* navigator,
              const BookmarkNode* node,
              WindowOpenDisposition initial_disposition,
              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,
                                gfx::NativeWindow window) {
   DCHECK(node && node->is_folder() && !node->empty());
   return ShowQuestionMessageBox(
              window, l10n_util::GetStringUTF16(IDS_PRODUCT_NAME),
              l10n_util::GetPluralStringFUTF16(
                  IDS_BOOKMARK_EDITOR_CONFIRM_DELETE,
                  ChildURLCountTotal(node))) ==
          MESSAGE_BOX_RESULT_YES;
 }
 
 void ShowBookmarkAllTabsDialog(Browser* browser) {
   Profile* profile = browser->profile();
   BookmarkModel* model = BookmarkModelFactory::GetForBrowserContext(profile);
   DCHECK(model && model->loaded());
 
   const BookmarkNode* parent = GetParentForNewNodes(model);
   BookmarkEditor::EditDetails details =
       BookmarkEditor::EditDetails::AddFolder(parent, parent->child_count());
   GetURLsForOpenTabs(browser, &(details.urls));
   DCHECK(!details.urls.empty());
 
   BookmarkEditor::Show(browser->window()->GetNativeWindow(), profile, details,
                        BookmarkEditor::SHOW_TREE);
 }
 
 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)
 
 }  // namespace chrome