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

chrome/browser/ui/bookmarks/bookmark_utils_desktop.h

Index: chrome/browser/ui/bookmarks/bookmark_utils_desktop.h
diff --git a/chrome/browser/ui/bookmarks/bookmark_utils_desktop.h b/chrome/browser/ui/bookmarks/bookmark_utils_desktop.h
index 062817bd9367af76de9b86e2116c5bb296f96392..846af962780ff3b3734f0a62e8cbccff2523df9f 100644
--- a/chrome/browser/ui/bookmarks/bookmark_utils_desktop.h
+++ b/chrome/browser/ui/bookmarks/bookmark_utils_desktop.h
@@ -47,6 +47,18 @@ void OpenAll(gfx::NativeWindow parent,
              WindowOpenDisposition initial_disposition,
              content::BrowserContext* browser_context);
 
+// Returns the count of bookmarks that would be opened by OpenAll

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

This needs to document the args, at least the ones that are non-obvious.

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

Done.

+int OpenCount(gfx::NativeWindow parent,

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

We're trying to kill the chrome:: namespace, and in general it's not clear to me
whether these sorts of "chrome-global" functions are necessary or desirable. 
These are only used by other functions in bookmark-related files, can they just
live in one of those files?  Might not make sense to implement this if the
implementations here are refactored to use a common functor with OpenAll() or
similar (see comments in .cc).

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

The OpenAll functions are used in both the C++ and Objective C implementations,
so I think it makes sense to keep them in a separate file, to save on code
duplication and to make behavior more consistent across OSs. 

+              const std::vector<const bookmarks::BookmarkNode*>& nodes,
+              bool offTheRecord = false,

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

Nit: I sort of feel like when half the callers want one behavior and half the
other, default args don't really make sense.

Also, do we really need both this bool and the next pointer when the pointer is
only valid when we're incognito?

Also, is this incognito or "off the record"?  The two mean different things (off
the record = incognito + guest).

Also, camel case -> underscores

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

I am treating incognito as a 'special case'. I don't think it will often have an
effect. 
The pointer is only valid when we are incognito, however, I think that it would
make little logical sense to assume that if the pointer has been set that we are
incognito, so I'd prefer to leave it in there to make sure things are clear.
... I think it's incognito.
Done.

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

But like I said, fully half the callers set it.  That doesn't seem like a
special case.  That said, I think this would make more sense if we only had one
default arg, instead of two; see below.
 
It seems like it might make sense if the argument was named |incognito_context|,
and the function comments clearly documented that if |incognito_context| is set,
then the URLs counted are only those that would still be opened when calling
OpenAll() with OFF_THE_RECORD as the disposition?  I dunno.

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

Done.

+              content::BrowserContext* browser_context = NULL);
+
+// Convenience for OpenCount() with a single BookmarkNode.
+int OpenCount(gfx::NativeWindow parent,
+              const bookmarks::BookmarkNode* node,
+              bool offTheRecord = false,
+              content::BrowserContext* browser_context = NULL);
+
 // Asks the user before deleting a non-empty bookmark folder.
 bool ConfirmDeleteBookmarkNode(const bookmarks::BookmarkNode* node,
                                gfx::NativeWindow window);