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

 // 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.
 
 #ifndef CHROME_BROWSER_UI_BOOKMARKS_BOOKMARK_UTILS_DESKTOP_H_
 #define CHROME_BROWSER_UI_BOOKMARKS_BOOKMARK_UTILS_DESKTOP_H_
 
 #include <vector>
 
 #include "ui/base/window_open_disposition.h"
@@ skipping 29 matching lines @@
              WindowOpenDisposition initial_disposition,
              content::BrowserContext* browser_context);
 
 // Convenience for OpenAll() with a single BookmarkNode.
 void OpenAll(gfx::NativeWindow parent,
              content::PageNavigator* navigator,
              const bookmarks::BookmarkNode* node,
              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);
 
 // Shows the bookmark all tabs dialog.
 void ShowBookmarkAllTabsDialog(Browser* browser);
 
 // Returns true if OpenAll() can open at least one bookmark of type url
 // in |selection|.
 bool HasBookmarkURLs(
     const std::vector<const bookmarks::BookmarkNode*>& selection);
 
 // Returns true if OpenAll() can open at least one bookmark of type url
 // in |selection| with incognito mode.
 bool HasBookmarkURLsAllowedInIncognitoMode(
     const std::vector<const bookmarks::BookmarkNode*>& selection,
     content::BrowserContext* browser_context);
 
 }  // namespace chrome
 
 #endif  // CHROME_BROWSER_UI_BOOKMARKS_BOOKMARK_UTILS_DESKTOP_H_