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 46 matching lines @@
 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.

[Peter Kasting, 2017/04/13 04:52:09]

Why this limitation?  Does this mean we won't count/"open all" bookmarks when
they're nested more than a level deep?  That seems wrong.

[Paezagon, 2017/04/14 01:00:29]

That's correct, when you use Open All on a bookmarks folder, it only opens the
bookmarks in the top level of the folder (this includes the bookmarks bar). It
might make sense to change that behavior, but not in this CL, as this CL is just
changing the display of the Open All

 // The following illustrates
 // typical usage:
 // OpenURLIterator iterator(nodes);
 // while (iterator.has_next()) {
 //   const GURL* url = iterator.NextURL();
 //   // do something with |urll|.
 // }
 class OpenURLIterator {

[Peter Kasting, 2017/04/13 04:52:09]

In general I wonder if this is really the right way to deal with this tree.  It
might be easier to have a method which returns a vector containing the URL
"leaves" of the tree, in order.  Then most callers can just traverse that vector
with standard algorithms, check if the vector is empty, etc.

[Paezagon, 2017/04/14 01:00:29]

I have done my best to replace the iterator with a function that returns a
vector. The code is a little shorter and tidier, but I am getting lost in the
pointers. The tests pass however, so I think it's working. 

  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;}
@@ skipping 134 matching lines @@
 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);
 }
 
+int OpenCount(gfx::NativeWindow parent,
+              const std::vector<const bookmarks::BookmarkNode*>& nodes,
+              bool offTheRecord,
+              content::BrowserContext* browser_context) {
+  int count = 0;
+  if (!ShouldOpenAll(parent, nodes))
+    return count;
+  // Counts all |nodes| of type URL and any children of |nodes| that are of type
+  // URL.
+  OpenURLIterator iterator(nodes);
+  while (iterator.has_next()) {

[Peter Kasting, 2017/04/13 04:52:09]

This feels very copy-and-pasted from OpenAll().  If you implement my suggestion
above, then write a lambda or functor that checks if a URL is safe to open in
incognito mode, it becomes easy to use methods like base::EraseIf() or
std::count_if() to transform the vector, check how many safe-to-open URLs there
are, etc.

[Paezagon, 2017/04/14 01:00:29]

I have added a second get vector function that returns URLs to use in incognito
mode. Seemed to make sense to do as that code is used in a couple places.

+    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.

[Peter Kasting, 2017/04/13 04:52:09]

This comment doesn't make much sense.  I realize you're copying it from above,
but please rewrite, or eliminate if the comment adds nothing to the code.

[Paezagon, 2017/04/14 01:00:29]

Done.

+    if (offTheRecord && !IsURLAllowedInIncognito(*url, browser_context))
+      continue;
+
+    count++;
+  }
+  return count;
+}
+
+int OpenCount(gfx::NativeWindow parent,
+              const BookmarkNode* node,
+              bool offTheRecord,
+              content::BrowserContext* browser_context) {
+  std::vector<const BookmarkNode*> nodes;
+  nodes.push_back(node);
+  return OpenCount(parent, nodes, offTheRecord, browser_context);

[Peter Kasting, 2017/04/13 04:52:09]

Nit: Simpler:

  return OpenCount(parent, {node}, offTheRecord, browser_context);

[Paezagon, 2017/04/14 01:00:29]

When I make the change, the compiler complains that it is recursive. 

c:\src\chromium\src\chrome\browser\ui\bookmarks\bookmark_utils_desktop.cc(258) :
warning C4717: 'chrome::OpenCount': recursive on all control paths, function
will cause runtime stack overflow

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

Might need to do something like:

  return OpenCount(parent, std::vector<const bookmarks::BookmarkNode*>{node},
...);

?  I'm not sure if that even compiles.

If not this should:

  std::vector<const BookmarkNode*> nodes = {node};
  return OpenCount(parent, nodes, offTheRecord, browser_context);

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

Done.

+}
+
 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;
 }
@@ skipping 25 matching lines @@
   while (iterator.has_next()) {
     const GURL* url = iterator.NextURL();
     if (IsURLAllowedInIncognito(*url, browser_context))
       return true;
   }
   return false;
 }
 #endif  // !defined(OS_ANDROID)
 
 }  // namespace chrome