Makes bookmark menu lazily create menus and removes limits

Increases the limit of bookmarks shown in the menu to 10,000

The ids now start at 0xDFFF. Since the menus are lazily loaded there isn't a need to limit how much is shown.

R=pkasting@chromium.org
BUG=374312
TEST=none

Committed: https://crrev.com/80d99738f74feb34aed36c5ec1c750d3418680b0
Cr-Commit-Position: refs/heads/master@{#322935}

[Peter Kasting, 2015-03-25 21:31:29]

Did you test to ensure 10,000 doesn't cause noticeable perf issues?

Is there a point at which perf does start to be a problem?  I'm wondering if we
can remove the cap entirely, of make it a number that's based on performance
concerns instead of being somewhat arbitrary.

https://codereview.chromium.org/1005873012/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.h (right):

https://codereview.chromium.org/1005873012/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:135: // ids aren't exposed to the
system.
Is it possible to define these values in chrome/app/chrome_command_ids.h or a
similar file that defines our other command IDs?  The idea of doing that would
be to make it more obvious that this range is in use, in case someone else tries
to add IDs there later.

(I don't know how many different files exist to reserve these different ranges
and if there's a better place for this.  For the same reason, I don't know if
there's something preventing you from using 0xD000 and up instead of 0xF000.)

https://codereview.chromium.org/1005873012/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:137: static const int
kMaxBookmarkCommandId = 0xF000 + 10000;
Can this use kMinBookmarkCommandId instead of repeating 0xF000?

[sky, 2015-03-30 20:46:35]

I've changed things around substantially. Part of the performance concern
previously was we created menus for *all* your bookmarks. I've changed it so we
create menus as the user shows them. This means the likelihood of seeing than
many is much slimmer.

https://codereview.chromium.org/1005873012/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.h (right):

https://codereview.chromium.org/1005873012/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:135: // ids aren't exposed to the
system.
On 2015/03/25 21:31:29, Peter Kasting wrote:
> Is it possible to define these values in chrome/app/chrome_command_ids.h or a
> similar file that defines our other command IDs?  The idea of doing that would
> be to make it more obvious that this range is in use, in case someone else
tries
> to add IDs there later.
> 
> (I don't know how many different files exist to reserve these different ranges
> and if there's a better place for this.  For the same reason, I don't know if
> there's something preventing you from using 0xD000 and up instead of 0xF000.)

Done.

https://codereview.chromium.org/1005873012/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:137: static const int
kMaxBookmarkCommandId = 0xF000 + 10000;
On 2015/03/25 21:31:29, Peter Kasting wrote:
> Can this use kMinBookmarkCommandId instead of repeating 0xF000?

I've nuked the defines here entirely.

[Peter Kasting, 2015-03-30 21:35:25]

Nice!  LGTM

https://codereview.chromium.org/1005873012/diff/60001/chrome/app/chrome_comma...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:365: // These values are > 0xDFFF as the ids are
not exposed to the native system.
Nit: Maybe this second sentence: "While command IDs passed to Windows functions
must not be higher than 0xDFFF, these IDs are not exposed to the native system
and thus can be in this otherwise-reserved range."

https://codereview.chromium.org/1005873012/diff/60001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:366: #define IDC_FIRST_BOOKMARK_MENU 0xDFFF + 1
Nit: "0xE000" seems a bit less obtuse.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.h (right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:131: // Range of command ID's to
use for the items in the recent tabs submenu.
Nit: ID's -> IDs (while you're here)

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (left):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:481: BuildMenu(node,
0, submenu, next_menu_id);
Just so I'm sure, the removal of this BuildMenu() call is what makes us not
build the whole nested tree at creation time, right?

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:350: if
(menu_id_to_node_map_.find(command) == menu_id_to_node_map_.end())
Nit: Shorter and slightly more efficient:

  auto iter = menu_id_to_node_map_.find(command);
  if ((iter != menu_id_to_node_map_.end()) && iter->second->child_count() &&
      !menu->GetSubmenu()->GetMenuItemCount())
    BuildMenu(iter->second, 0, menu);

I would also be fine with pulling "iter->second" out into a named temp; I mostly
just figured there was no reason to effectively do a find() twice (one
explicitly and once implicitly).

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:505: const
BookmarkNode* node = parent->GetChild(i);
Nit: Shorter, avoids violating the "don't handle DCHECK failure" rule, avoids
any possible "uninitialized variable" warnings:

    const BookmarkNode* node = parent->GetChild(i);
    DCHECK(node->is_folder() || node->is_url());
    const gfx::ImageSkia* icon = rb->GetImageSkiaNamed(IDR_BOOKMARK_BAR_FOLDER);
    if (node->is_url()) {
      const gfx::Image& image = GetBookmarkModel()->GetFavicon(node);
      icon = image.IsEmpty() ? rb->GetImageSkiaNamed(IDR_DEFAULT_FAVICON)
                             : image.ToImageSkia();
    }
    AddMenuToMaps(
        menu->AppendSubMenuWithIcon(next_menu_id_++, node->GetTitle(), *icon),
        node);

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc
(right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc:70: :
nullptr;
Nit: Shorter and more efficient (not that it matters):

    const auto& node_map = bookmark_menu_delegate_->menu_id_to_node_map_;
    auto iter = node_map.find(menu->GetCommand());
    return (iter == node_map.end()) ? nullptr : iter->second;

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc:146: // f1
should have loaded it's children.
Nit: its

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc:165: // F11
should have loaded it's single child (f11a).
Nit: its

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:94: return command_id >=
IDC_FIRST_BOOKMARK_MENU;
You should probably add a comment to where this value is defined saying no
Wrench menu command IDs may ever be larger.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1051:
bookmark_menu_delegate_.get()->WillShowMenu(menu);
Nit: .get() probably unnecessary

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1241: LOG(WARNING) <<
model->bookmark_bar_node()->GetTotalNodeCount();
Don't commit this

[sky, 2015-03-30 23:20:31]

The CQ bit was checked by sky@chromium.org

[sky, 2015-03-30 23:20:32]

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/1005873012/#ps80001
(title: "review feedback")

[commit-bot: I haz the power, 2015-03-30 23:20:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1005873012/80001

[commit-bot: I haz the power, 2015-03-31 01:03:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-31 01:04:50]

Patchset 5 (id:??) landed as
https://crrev.com/80d99738f74feb34aed36c5ec1c750d3418680b0
Cr-Commit-Position: refs/heads/master@{#322935}

[sky, 2015-03-31 15:17:39]

I forgot to push my responses before landing yesterday. Sorry.

https://codereview.chromium.org/1005873012/diff/60001/chrome/app/chrome_comma...
File chrome/app/chrome_command_ids.h (right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:365: // These values are > 0xDFFF as the ids are
not exposed to the native system.
On 2015/03/30 21:35:24, Peter Kasting wrote:
> Nit: Maybe this second sentence: "While command IDs passed to Windows
functions
> must not be higher than 0xDFFF, these IDs are not exposed to the native system
> and thus can be in this otherwise-reserved range."

Done.

https://codereview.chromium.org/1005873012/diff/60001/chrome/app/chrome_comma...
chrome/app/chrome_command_ids.h:366: #define IDC_FIRST_BOOKMARK_MENU 0xDFFF + 1
On 2015/03/30 21:35:24, Peter Kasting wrote:
> Nit: "0xE000" seems a bit less obtuse.

Done.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/wrench_menu_model.h (right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/wrench_menu_model.h:131: // Range of command ID's to
use for the items in the recent tabs submenu.
On 2015/03/30 21:35:24, Peter Kasting wrote:
> Nit: ID's -> IDs (while you're here)

Done.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (left):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:481: BuildMenu(node,
0, submenu, next_menu_id);
On 2015/03/30 21:35:24, Peter Kasting wrote:
> Just so I'm sure, the removal of this BuildMenu() call is what makes us not
> build the whole nested tree at creation time, right?

That's right.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:350: if
(menu_id_to_node_map_.find(command) == menu_id_to_node_map_.end())
On 2015/03/30 21:35:24, Peter Kasting wrote:
> Nit: Shorter and slightly more efficient:
> 
>   auto iter = menu_id_to_node_map_.find(command);
>   if ((iter != menu_id_to_node_map_.end()) && iter->second->child_count() &&
>       !menu->GetSubmenu()->GetMenuItemCount())
>     BuildMenu(iter->second, 0, menu);
> 
> I would also be fine with pulling "iter->second" out into a named temp; I
mostly
> just figured there was no reason to effectively do a find() twice (one
> explicitly and once implicitly).

Done.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:505: const
BookmarkNode* node = parent->GetChild(i);
On 2015/03/30 21:35:24, Peter Kasting wrote:
> Nit: Shorter, avoids violating the "don't handle DCHECK failure" rule, avoids
> any possible "uninitialized variable" warnings:
> 
>     const BookmarkNode* node = parent->GetChild(i);
>     DCHECK(node->is_folder() || node->is_url());
>     const gfx::ImageSkia* icon =
rb->GetImageSkiaNamed(IDR_BOOKMARK_BAR_FOLDER);
>     if (node->is_url()) {
>       const gfx::Image& image = GetBookmarkModel()->GetFavicon(node);
>       icon = image.IsEmpty() ? rb->GetImageSkiaNamed(IDR_DEFAULT_FAVICON)
>                              : image.ToImageSkia();
>     }
>     AddMenuToMaps(
>         menu->AppendSubMenuWithIcon(next_menu_id_++, node->GetTitle(), *icon),
>         node);

The calls to add the menus are slightly different, so this doesn't work. I
removed the else NOTREACHED though and went with the DCHECK.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc
(right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc:70: :
nullptr;
On 2015/03/30 21:35:24, Peter Kasting wrote:
> Nit: Shorter and more efficient (not that it matters):
> 
>     const auto& node_map = bookmark_menu_delegate_->menu_id_to_node_map_;
>     auto iter = node_map.find(menu->GetCommand());
>     return (iter == node_map.end()) ? nullptr : iter->second;

Done.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc:146: // f1
should have loaded it's children.
On 2015/03/30 21:35:24, Peter Kasting wrote:
> Nit: its

Done.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate_unittest.cc:165: // F11
should have loaded it's single child (f11a).
On 2015/03/30 21:35:25, Peter Kasting wrote:
> Nit: its

Done.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/wrench_menu.cc (right):

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:94: return command_id >=
IDC_FIRST_BOOKMARK_MENU;
On 2015/03/30 21:35:25, Peter Kasting wrote:
> You should probably add a comment to where this value is defined saying no
> Wrench menu command IDs may ever be larger.

Done.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1051:
bookmark_menu_delegate_.get()->WillShowMenu(menu);
On 2015/03/30 21:35:25, Peter Kasting wrote:
> Nit: .get() probably unnecessary

Done.

https://codereview.chromium.org/1005873012/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/wrench_menu.cc:1241: LOG(WARNING) <<
model->bookmark_bar_node()->GetTotalNodeCount();
On 2015/03/30 21:35:25, Peter Kasting wrote:
> Don't commit this

Done.