Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2809003002:
Making bookmark folder context menu display the number of bookmarks that will be opened by Open All (Closed)
|
Created:
3 years, 8 months ago by Paezagon Modified:
3 years, 8 months ago Reviewers:
Peter Kasting CC:
chromium-reviews, tfarina, mac-reviews_chromium.org, Wez Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWorking on making context menu display the number of bookmarks that will be opened by by Open All.
Modified the strings to change based on a passed in count. Modified bookmark_context_menu_controller and bookmark_menu_bridge to pass in the count of bookmarks that will be opened. Added a helper function in bookmark_utils_desktop to determine the count.
BUG=708815
Review-Url: https://codereview.chromium.org/2809003002
Cr-Commit-Position: refs/heads/master@{#464949}
Committed: https://chromium.googlesource.com/chromium/src/+/ae360640e5b0475e52f8cee25d755ce484faf6e4
Patch Set 1 : All changes to add count to context menu #
Total comments: 32
Patch Set 2 : Refactoring URL itteration function as per comments, addressing other formatting and style errors #
Total comments: 35
Patch Set 3 : Code modifications in response to comments #Patch Set 4 : removing boolean flag from open count #
Total comments: 12
Patch Set 5 : Last minor changes #
Total comments: 6
Patch Set 6 : Few more small fixes #Patch Set 7 : Fixing compilation issues #Messages
Total messages: 32 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Working on making context menu display the number of bookmarks that will be opened by by Open All BUG=708815 ========== to ========== Working on making context menu display the number of bookmarks that will be opened by by Open All BUG=708815 ==========
Description was changed from ========== Working on making context menu display the number of bookmarks that will be opened by by Open All BUG=708815 ========== to ========== Working on making context menu display the number of bookmarks that will be opened by by Open All. Modified the strings to change based on a passed in count. Modified bookmark_context_menu_controller and bookmark_menu_bridge to pass in the count of bookmarks that will be opened. Added a helper function in bookmark_utils_desktop to determine the count. BUG=708815 ==========
paezagon@chromium.org changed reviewers: + pkasting@chromium.org
Hello, I am doing this bug as a warmup project on chromium. As you were cc'ed in the bug, and an owner on the files, I was hoping you'd be able to review my changes. Thanks, Anna
For the style issues mentioned below, running git cl format before uploading may help a bit. https://codereview.chromium.org/2809003002/diff/60001/chrome/app/bookmarks_st... File chrome/app/bookmarks_strings.grdp (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/app/bookmarks_st... chrome/app/bookmarks_strings.grdp:66: <message name="IDS_BOOKMARK_BAR_OPEN_ALL" desc="Menu title for opening all urls in a bookmark folder"> Please set your editor to add spaces, not tabs, and fix this file. https://codereview.chromium.org/2809003002/diff/60001/chrome/app/bookmarks_st... chrome/app/bookmarks_strings.grdp:68: =0 {&Open all} Is this only displayed as a disabled menu item? https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc:81: int incognitoCount = Nit: Variable names in Chromium are spelled unix_hacker-style, not camelCase. Declare this variable as close to first use as possible, or inline into the statement below that uses it. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h:82: void AddItem(int id, base::string16 str); Nit: Pass by const ref https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:67: // This does not recurse through all descendants, only immediate children. 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. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:75: class OpenURLIterator { 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. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:240: while (iterator.has_next()) { 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. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:244: // not opened. 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. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:259: return OpenCount(parent, nodes, offTheRecord, browser_context); Nit: Simpler: return OpenCount(parent, {node}, offTheRecord, browser_context); https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.h (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:50: // Returns the count of bookmarks that would be opened by OpenAll This needs to document the args, at least the ones that are non-obvious. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:51: int OpenCount(gfx::NativeWindow parent, 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). https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:53: bool offTheRecord = false, 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 https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm:317: count = chrome::OpenCount(NULL, node, true, profile_); Nit: nullptr https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm:320: } Nit: Two potential simpler routes: int count = (message_id == IDS_BOOKMARK_BAR_OPEN_ALL_INCOGNITO) ? chrome::OpenCount(NULL, node, true, profile_); : chrome::OpenCount(NULL, node); bool incognito = (message_id == IDS_BOOKMARK_BAR_OPEN_ALL_INCOGNITO); content::BrowserContext* context = incognito ? profile_ : nullptr; int count = chrome::OpenCount(nullptr, node, incognito, context);
https://codereview.chromium.org/2809003002/diff/60001/chrome/app/bookmarks_st... File chrome/app/bookmarks_strings.grdp (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/app/bookmarks_st... chrome/app/bookmarks_strings.grdp:66: <message name="IDS_BOOKMARK_BAR_OPEN_ALL" desc="Menu title for opening all urls in a bookmark folder"> On 2017/04/13 04:52:09, Peter Kasting wrote: > Please set your editor to add spaces, not tabs, and fix this file. Missed that. The git cl format corrected all the other files. https://codereview.chromium.org/2809003002/diff/60001/chrome/app/bookmarks_st... chrome/app/bookmarks_strings.grdp:68: =0 {&Open all} On 2017/04/13 04:52:08, Peter Kasting wrote: > Is this only displayed as a disabled menu item? As far as I can tell, yes. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc:81: int incognitoCount = On 2017/04/13 04:52:09, Peter Kasting wrote: > Nit: Variable names in Chromium are spelled unix_hacker-style, not camelCase. > > Declare this variable as close to first use as possible, or inline into the > statement below that uses it. I am going to move it down, as I believe using the variable improves readability. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h:82: void AddItem(int id, base::string16 str); On 2017/04/13 04:52:09, Peter Kasting wrote: > Nit: Pass by const ref Done. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:67: // This does not recurse through all descendants, only immediate children. On 2017/04/13 04:52:09, Peter Kasting wrote: > 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. 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 https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:75: class OpenURLIterator { On 2017/04/13 04:52:09, Peter Kasting wrote: > 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. 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. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:240: while (iterator.has_next()) { On 2017/04/13 04:52:09, Peter Kasting wrote: > 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. 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. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:244: // not opened. On 2017/04/13 04:52:09, Peter Kasting wrote: > 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. Done. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:259: return OpenCount(parent, nodes, offTheRecord, browser_context); On 2017/04/13 04:52:09, Peter Kasting wrote: > Nit: Simpler: > > return OpenCount(parent, {node}, offTheRecord, browser_context); 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 https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.h (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:50: // Returns the count of bookmarks that would be opened by OpenAll On 2017/04/13 04:52:09, Peter Kasting wrote: > This needs to document the args, at least the ones that are non-obvious. Done. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:51: int OpenCount(gfx::NativeWindow parent, On 2017/04/13 04:52:09, Peter Kasting wrote: > 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). 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. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:53: bool offTheRecord = false, On 2017/04/13 04:52:09, Peter Kasting wrote: > Nit: I sort of feel like when half the callers want one behavior and half the > other, default args don't really make sense. I am treating incognito as a 'special case'. I don't think it will often have an effect. > > Also, do we really need both this bool and the next pointer when the pointer is > only valid when we're incognito? 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. > > Also, is this incognito or "off the record"? The two mean different things (off > the record = incognito + guest). ... I think it's incognito. > > Also, camel case -> underscores Done. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm:317: count = chrome::OpenCount(NULL, node, true, profile_); On 2017/04/13 04:52:10, Peter Kasting wrote: > Nit: nullptr Done. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm:320: } On 2017/04/13 04:52:09, Peter Kasting wrote: > Nit: Two potential simpler routes: > > int count = (message_id == IDS_BOOKMARK_BAR_OPEN_ALL_INCOGNITO) > ? chrome::OpenCount(NULL, node, true, profile_); > : chrome::OpenCount(NULL, node); > > bool incognito = (message_id == IDS_BOOKMARK_BAR_OPEN_ALL_INCOGNITO); > content::BrowserContext* context = incognito ? profile_ : nullptr; > int count = chrome::OpenCount(nullptr, node, incognito, context); Done.
https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:259: return OpenCount(parent, nodes, offTheRecord, browser_context); On 2017/04/14 01:00:29, Paezagon wrote: > On 2017/04/13 04:52:09, Peter Kasting wrote: > > Nit: Simpler: > > > > return OpenCount(parent, {node}, offTheRecord, browser_context); > > 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 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); https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.h (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:53: bool offTheRecord = false, On 2017/04/14 01:00:29, Paezagon wrote: > On 2017/04/13 04:52:09, Peter Kasting wrote: > > Nit: I sort of feel like when half the callers want one behavior and half the > > other, default args don't really make sense. > > I am treating incognito as a 'special case'. I don't think it will often have an > effect. 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. > > Also, do we really need both this bool and the next pointer when the pointer > is > > only valid when we're incognito? > > 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. 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. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:66: // nodes that are not URLs, add their child URLs. 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." https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:67: std::vector<const GURL*> GetOpenURLs( 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. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:72: nodes_it != nodes.end(); ++nodes_it) { Nit: Use range-based for, which will simplify both the loop declaration and body syntax in this case https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:74: // if node is URL add it 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. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:79: ++child_index) { Nit: Again, use range-based for https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:94: urls.erase(std::remove_if(urls.begin(), urls.end(), Nit: Prefer base::EraseIf() to erase(remove_if()) https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:106: int child_count = GetOpenURLs(nodes).size(); 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() https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:164: : GetOpenURLs(nodes); 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. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:170: ui::PAGE_TRANSITION_AUTO_BOOKMARK, false)); 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()? https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:172: url_it++; Nit: Preincrement iterators https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:243: return GetOpenURLs(selection).size() > 0; Nit: Prefer !empty() to size() > 0 (2 places) https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.h (right): https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:56: content::BrowserContext* browser_context = NULL); Nit: NULL -> nullptr (everywhere in CL) https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:58: // Convenience for OpenCount() with a single BookmarkNode. Nit: Maybe we shouldn't add this, and BookmarkMenuBridge::AddItemToMenu() should just create the nodes vector itself? https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm (right): https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm:323: // NSMenuItem setEnabled will not reflect the disabled state Nit: Why re-linebreak this? https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc (right): https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:160: // Should have navigated to F1's child but not F11's child. Nit: Should have navigated -> OpenAll() would navigate (2 places) https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:161: ASSERT_EQ(static_cast<size_t>(2), result); Nit: Can be EXPECT rather than ASSERT. Use "2U" instead of static casting. (2 places) https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:165: // incognito window. Nit: This is slightly unclear; maybe instead "Same as the above test, but for bookmarks openable in incognito mode."
https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:259: return OpenCount(parent, nodes, offTheRecord, browser_context); On 2017/04/14 08:17:14, Peter Kasting wrote: > On 2017/04/14 01:00:29, Paezagon wrote: > > On 2017/04/13 04:52:09, Peter Kasting wrote: > > > Nit: Simpler: > > > > > > return OpenCount(parent, {node}, offTheRecord, browser_context); > > > > 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 > > 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); Done. https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.h (right): https://codereview.chromium.org/2809003002/diff/60001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:53: bool offTheRecord = false, On 2017/04/14 08:17:14, Peter Kasting wrote: > On 2017/04/14 01:00:29, Paezagon wrote: > > On 2017/04/13 04:52:09, Peter Kasting wrote: > > > Nit: I sort of feel like when half the callers want one behavior and half > the > > > other, default args don't really make sense. > > > > I am treating incognito as a 'special case'. I don't think it will often have > an > > effect. > > 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. > > > > Also, do we really need both this bool and the next pointer when the pointer > > is > > > only valid when we're incognito? > > > > 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. > > 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. Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:66: // nodes that are not URLs, add their child URLs. On 2017/04/14 08:17:15, Peter Kasting wrote: > 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." Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:67: std::vector<const GURL*> GetOpenURLs( On 2017/04/14 08:17:15, Peter Kasting wrote: > 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. Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:72: nodes_it != nodes.end(); ++nodes_it) { On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: Use range-based for, which will simplify both the loop declaration and body > syntax in this case Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:74: // if node is URL add it On 2017/04/14 08:17:15, Peter Kasting wrote: > 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. Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:79: ++child_index) { On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: Again, use range-based for 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. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:94: urls.erase(std::remove_if(urls.begin(), urls.end(), On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: Prefer base::EraseIf() to erase(remove_if()) Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:106: int child_count = GetOpenURLs(nodes).size(); On 2017/04/14 08:17:15, Peter Kasting wrote: > 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() Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:164: : GetOpenURLs(nodes); On 2017/04/14 08:17:15, Peter Kasting wrote: > 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. Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:170: ui::PAGE_TRANSITION_AUTO_BOOKMARK, false)); On 2017/04/14 08:17:15, Peter Kasting wrote: > 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()? Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:172: url_it++; On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: Preincrement iterators Gone. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:243: return GetOpenURLs(selection).size() > 0; On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: Prefer !empty() to size() > 0 (2 places) Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.h (right): https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:56: content::BrowserContext* browser_context = NULL); On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: NULL -> nullptr (everywhere in CL) Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.h:58: // Convenience for OpenCount() with a single BookmarkNode. On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: Maybe we shouldn't add this, and BookmarkMenuBridge::AddItemToMenu() should > just create the nodes vector itself? I would rather keep it consistent with the OpenAll functions. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/cocoa... File chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm (right): https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/cocoa... chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm:323: // NSMenuItem setEnabled will not reflect the disabled state On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: Why re-linebreak this? No idea how that happened. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc (right): https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:160: // Should have navigated to F1's child but not F11's child. On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: Should have navigated -> OpenAll() would navigate (2 places) Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:161: ASSERT_EQ(static_cast<size_t>(2), result); On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: Can be EXPECT rather than ASSERT. Use "2U" instead of static casting. (2 > places) Done. https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:165: // incognito window. On 2017/04/14 08:17:15, Peter Kasting wrote: > Nit: This is slightly unclear; maybe instead "Same as the above test, but for > bookmarks openable in incognito mode." Done.
LGTM https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/80001/chrome/browser/ui/bookm... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:79: ++child_index) { On 2017/04/14 18:47:24, Paezagon wrote: > On 2017/04/14 08:17:15, Peter Kasting wrote: > > Nit: Again, use range-based for > > 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. That might indeed make sense to do. I'm fine with doing it separately. https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc (right): https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc:405: bookmarks::GetParentForNewNodes(parent_, selection_, nullptr) != Nit: Could just remove "!= nullptr" https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h (right): https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h:49: // |browser| is used to open the bookmark manager and is nullptr in tests. Nit: In comments, just say "null" in lowercase. https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:72: if (node->is_url() and Hey now, this is C++, not Pascal :) https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:74: IsURLAllowedInIncognito(node->url(), browser_context))) { 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. https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:96: const size_t kNumBookmarkUrlsBeforePrompting = 15; Nit: Prefer constexpr to const for compile-time constants https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc (right): https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:158: int result = chrome::OpenCount(nullptr, folder); Nit: You may inline this below, if you want (2 places)
https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc (right): https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc:405: bookmarks::GetParentForNewNodes(parent_, selection_, nullptr) != On 2017/04/14 19:04:40, Peter Kasting wrote: > Nit: Could just remove "!= nullptr" Done. https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h (right): https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h:49: // |browser| is used to open the bookmark manager and is nullptr in tests. On 2017/04/14 19:04:40, Peter Kasting wrote: > Nit: In comments, just say "null" in lowercase. Done. https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:72: if (node->is_url() and On 2017/04/14 19:04:40, Peter Kasting wrote: > Hey now, this is C++, not Pascal :) Eheh. I tend to get confused when switching langues... Weee Python + Javascript + Java => C++ https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:74: IsURLAllowedInIncognito(node->url(), browser_context))) { On 2017/04/14 19:04:40, Peter Kasting wrote: > 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. Done with couple alterations. https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:96: const size_t kNumBookmarkUrlsBeforePrompting = 15; On 2017/04/14 19:04:40, Peter Kasting wrote: > Nit: Prefer constexpr to const for compile-time constants Done. https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc (right): https://codereview.chromium.org/2809003002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:158: int result = chrome::OpenCount(nullptr, folder); On 2017/04/14 19:04:40, Peter Kasting wrote: > Nit: You may inline this below, if you want (2 places) Done.
Still LGTM Great work on this change! Thanks for being so thoughtful about how you implemented things, e.g. pushing back/modifying my suggestions when doing something else made more sense. https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:86: if (child->is_url()) { Nit: No {} https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:167: // be nullptr in tests. Nit: Just "null" in comments https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc (right): https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:159: // Should have counted F1's child but not F11's child, as that's what OpenAll Nit: have counted -> count, now that you have inlined things? (2 places)
The CQ bit was checked by paezagon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2809003002/#ps160001 (title: "Few more small fixes")
The CQ bit was unchecked by paezagon@chromium.org
https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/book... File chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc (right): https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:86: if (child->is_url()) { On 2017/04/14 20:41:53, Peter Kasting wrote: > Nit: No {} Done. https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/book... chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc:167: // be nullptr in tests. On 2017/04/14 20:41:53, Peter Kasting wrote: > Nit: Just "null" in comments Done. https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc (right): https://codereview.chromium.org/2809003002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc:159: // Should have counted F1's child but not F11's child, as that's what OpenAll On 2017/04/14 20:41:53, Peter Kasting wrote: > Nit: have counted -> count, now that you have inlined things? (2 places) Done.
The CQ bit was checked by paezagon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by paezagon@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by paezagon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2809003002/#ps180001 (title: "Fixing compilation issues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492451098850950,
"parent_rev": "e0122c897d6339f118cea81d8c0d196561892119", "commit_rev":
"ae360640e5b0475e52f8cee25d755ce484faf6e4"}
Message was sent while issue was closed.
Description was changed from ========== Working on making context menu display the number of bookmarks that will be opened by by Open All. Modified the strings to change based on a passed in count. Modified bookmark_context_menu_controller and bookmark_menu_bridge to pass in the count of bookmarks that will be opened. Added a helper function in bookmark_utils_desktop to determine the count. BUG=708815 ========== to ========== Working on making context menu display the number of bookmarks that will be opened by by Open All. Modified the strings to change based on a passed in count. Modified bookmark_context_menu_controller and bookmark_menu_bridge to pass in the count of bookmarks that will be opened. Added a helper function in bookmark_utils_desktop to determine the count. BUG=708815 Review-Url: https://codereview.chromium.org/2809003002 Cr-Commit-Position: refs/heads/master@{#464949} Committed: https://chromium.googlesource.com/chromium/src/+/ae360640e5b0475e52f8cee25d75... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/ae360640e5b0475e52f8cee25d75... |
