Changes BookmarkBarView to only create buttons as needed

Changes BookmarkBarView to only create buttons as needed

Previously it created a button per child of the bookmark bar node. Now
it creates only as many buttons as are needed, lazily creating more if
the size changes.

BUG=416641
TEST=covered by tests.
R=msw@chromium.org

Committed: https://crrev.com/b8c86e2bed280803ec89ae1d9f4acf9b6b29aed6
Cr-Commit-Position: refs/heads/master@{#301551}

[msw, 2014-10-27 20:40:25]

Mostly nits, but this code could be clearer, eg. explaining the
GetBookmarkButtonCount return value better, etc.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:786: // If we have no
model we're being destroyed and there is no point in doing a
nit: "Skip layout during destruction, when no model exists."

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:862:
DCHECK_NE(button_count, GetBookmarkButtonCount());
nit: DCHECK_LT, if at all?

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:877: // Layout the right
side of the bar.
nit: Remove this comment.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:886:
(model_->bookmark_bar_node()->child_count() != GetBookmarkButtonCount() ||
nit: explicitly check for '>' instead of '!='.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1178: // The actual
bookmark buttons are added from Layout().
nit: combine with the comment above.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1649: void
BookmarkBarView::LoadNextBookmarkButton() {
This is only called in Layout, consider in-lining there.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:289: // Returns the number
of buttons corresponding to starred urls/folders. This
Consider revising this comment, it's no longer quite true.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:326: // does not
layout/paint. It's assumed the call site does that.
nit: I think you can remove "This does not layout/paint... does that."

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:51: for (int i =
0; i < test_helper_->GetBookmarkButtonCount(); ++i) {
Doesn't GetBookmarkButtonCount include invisible buttons that were added but
don't fit (like when shrinking the window, reordering nodes, etc?). I don't
think that nuance is obvious, and it may lead to incorrect testing (or worse).

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:64: for (int i =
0; i < 100 &&
nit: why not just do a while loop that increases bbv_->width() and nix |i|?

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:174:
EXPECT_FALSE(test_helper_->overflow_button()->visible());
nit: test shrinking the bookmark bar view here.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:196: // Verifies
buttons are added as the model and size change.
So this test is just like above, but doesn't call WaitForBookmarkModelToLoad()?
Can you verify that the model hasn't yet loaded before some operation, and has
indeed loaded afterward, as expected?

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:226:
EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
nit: is it worthwhile for multiple tests to expect 0/1 around SizeUntil?

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:232:
EXPECT_EQ(1, test_helper_->GetBookmarkButtonCount());
nit: EXPECT_EQ("a", GetStringForButtons()); instead?

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:266: // Even
though only one node fits, the second node isn't deleted.
nit: revise this comment to match the check.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:272: // Even
though only one node fits, the second node isn't deleted.
nit: revise this comment to match the check.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:276: //
Assertions for changing the title of a node.
Should we test that increasing the name length could cause a button to hide?

[sky, 2014-10-27 21:48:19]

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc (right):

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:786: // If we have no
model we're being destroyed and there is no point in doing a
On 2014/10/27 20:40:24, msw wrote:
> nit: "Skip layout during destruction, when no model exists."

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:862:
DCHECK_NE(button_count, GetBookmarkButtonCount());
On 2014/10/27 20:40:24, msw wrote:
> nit: DCHECK_LT, if at all?

I removed the DCHECK as LoadNextBookmarkButton has it.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:877: // Layout the right
side of the bar.
On 2014/10/27 20:40:24, msw wrote:
> nit: Remove this comment.

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:886:
(model_->bookmark_bar_node()->child_count() != GetBookmarkButtonCount() ||
On 2014/10/27 20:40:24, msw wrote:
> nit: explicitly check for '>' instead of '!='.

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1178: // The actual
bookmark buttons are added from Layout().
On 2014/10/27 20:40:24, msw wrote:
> nit: combine with the comment above.

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc:1649: void
BookmarkBarView::LoadNextBookmarkButton() {
On 2014/10/27 20:40:24, msw wrote:
> This is only called in Layout, consider in-lining there.

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:289: // Returns the number
of buttons corresponding to starred urls/folders. This
On 2014/10/27 20:40:24, msw wrote:
> Consider revising this comment, it's no longer quite true.

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:326: // does not
layout/paint. It's assumed the call site does that.
On 2014/10/27 20:40:24, msw wrote:
> nit: I think you can remove "This does not layout/paint... does that."

I nuked this function entirely.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:51: for (int i =
0; i < test_helper_->GetBookmarkButtonCount(); ++i) {
On 2014/10/27 20:40:25, msw wrote:
> Doesn't GetBookmarkButtonCount include invisible buttons that were added but
> don't fit (like when shrinking the window, reordering nodes, etc?). I don't
> think that nuance is obvious, and it may lead to incorrect testing (or worse).

Indeed. I changed things around and made the names better reflect what is
returned.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:64: for (int i =
0; i < 100 &&
On 2014/10/27 20:40:25, msw wrote:
> nit: why not just do a while loop that increases bbv_->width() and nix |i|?

If this gets to 100 something is likely very wrong, which is why I went with the
for loop.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:174:
EXPECT_FALSE(test_helper_->overflow_button()->visible());
On 2014/10/27 20:40:25, msw wrote:
> nit: test shrinking the bookmark bar view here.

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:196: // Verifies
buttons are added as the model and size change.
On 2014/10/27 20:40:25, msw wrote:
> So this test is just like above, but doesn't call
WaitForBookmarkModelToLoad()?
> Can you verify that the model hasn't yet loaded before some operation, and has
> indeed loaded afterward, as expected?

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:226:
EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
On 2014/10/27 20:40:25, msw wrote:
> nit: is it worthwhile for multiple tests to expect 0/1 around SizeUntil?

I view it like a DCHECK. If this fails then the rest of the test results should
be suspect. I think it's good to know when things went wrong.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:232:
EXPECT_EQ(1, test_helper_->GetBookmarkButtonCount());
On 2014/10/27 20:40:25, msw wrote:
> nit: EXPECT_EQ("a", GetStringForButtons()); instead?

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:266: // Even
though only one node fits, the second node isn't deleted.
On 2014/10/27 20:40:25, msw wrote:
> nit: revise this comment to match the check.

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:272: // Even
though only one node fits, the second node isn't deleted.
On 2014/10/27 20:40:25, msw wrote:
> nit: revise this comment to match the check.

Done.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:276: //
Assertions for changing the title of a node.
On 2014/10/27 20:40:25, msw wrote:
> Should we test that increasing the name length could cause a button to hide?

Done.

[msw, 2014-10-27 22:50:19]

Just a couple minor comments.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:64: for (int i =
0; i < 100 &&
On 2014/10/27 21:48:18, sky wrote:
> On 2014/10/27 20:40:25, msw wrote:
> > nit: why not just do a while loop that increases bbv_->width() and nix |i|?
> 
> If this gets to 100 something is likely very wrong, which is why I went with
the
> for loop.

Acknowledged.

https://codereview.chromium.org/677533003/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:226:
EXPECT_EQ(0, test_helper_->GetBookmarkButtonCount());
On 2014/10/27 21:48:18, sky wrote:
> On 2014/10/27 20:40:25, msw wrote:
> > nit: is it worthwhile for multiple tests to expect 0/1 around SizeUntil?
> 
> I view it like a DCHECK. If this fails then the rest of the test results
should
> be suspect. I think it's good to know when things went wrong.

Acknowledged.

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:288: //
I don't think these two comment blocks should be joined.

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:290: // Buttons
What? Fix this sentence of the comment. Overall, I'd also suggest something like
"Returns the number of bookmark bar url/folder buttons that have been created.
This does not necessarily represent the number of bookmark bar nodes, nor the
number of visible bookmark bar buttons. Buttons are created lazily to fill
available space, and may be hidden for ordering or sizing changes."

I'm trying to convey: #(visible buttons) <= #GetBookmarkButtonCount() <=
#(bookmark bar model buttons).

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:323:
EXPECT_LE(1, test_helper_->GetBookmarkButtonCount());
nit: EXPECT_EQ("a1", GetStringForVisibleButtons());

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:326: // use
GetStringForVisibleButtons() here as more buttons may have been
nit: Is this still necessary? The visibility check that you added to
GetStringForVisibleButtons() should make it okay to test that string:
EXPECT_EQ("a1 b1", GetStringForVisibleButtons());

[sky, 2014-10-27 23:00:50]

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:288: //
On 2014/10/27 22:50:18, msw wrote:
> I don't think these two comment blocks should be joined.

Done.

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:290: // Buttons
On 2014/10/27 22:50:18, msw wrote:
> What? Fix this sentence of the comment. Overall, I'd also suggest something
like
> "Returns the number of bookmark bar url/folder buttons that have been created.
> This does not necessarily represent the number of bookmark bar nodes, nor the
> number of visible bookmark bar buttons. Buttons are created lazily to fill
> available space, and may be hidden for ordering or sizing changes."
> 
> I'm trying to convey: #(visible buttons) <= #GetBookmarkButtonCount() <=
> #(bookmark bar model buttons).

Done.

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:323:
EXPECT_LE(1, test_helper_->GetBookmarkButtonCount());
On 2014/10/27 22:50:18, msw wrote:
> nit: EXPECT_EQ("a1", GetStringForVisibleButtons());

'a1' has changed to 'a_really_long_title', which is big enough that both buttons
are most likely not visible. This is why the assertion is now there should be
none or one button visible.

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:326: // use
GetStringForVisibleButtons() here as more buttons may have been
On 2014/10/27 22:50:19, msw wrote:
> nit: Is this still necessary? The visibility check that you added to
> GetStringForVisibleButtons() should make it okay to test that string:
> EXPECT_EQ("a1 b1", GetStringForVisibleButtons());

Not strictly, but it seemed nice and kept the rest of the assertions easier to
grok.

[msw, 2014-10-27 23:06:00]

lgtm

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:288: //
On 2014/10/27 23:00:49, sky wrote:
> On 2014/10/27 22:50:18, msw wrote:
> > I don't think these two comment blocks should be joined.
> 
> Done.

nit: (still joined)

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc (right):

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:323:
EXPECT_LE(1, test_helper_->GetBookmarkButtonCount());
On 2014/10/27 23:00:49, sky wrote:
> On 2014/10/27 22:50:18, msw wrote:
> > nit: EXPECT_EQ("a1", GetStringForVisibleButtons());
> 
> 'a1' has changed to 'a_really_long_title', which is big enough that both
buttons
> are most likely not visible. This is why the assertion is now there should be
> none or one button visible.

Acknowledged.

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view_unittest.cc:326: // use
GetStringForVisibleButtons() here as more buttons may have been
On 2014/10/27 23:00:49, sky wrote:
> On 2014/10/27 22:50:19, msw wrote:
> > nit: Is this still necessary? The visibility check that you added to
> > GetStringForVisibleButtons() should make it okay to test that string:
> > EXPECT_EQ("a1 b1", GetStringForVisibleButtons());
> 
> Not strictly, but it seemed nice and kept the rest of the assertions easier to
> grok.

Acknowledged.

[sky, 2014-10-27 23:13:58]

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_view.h (right):

https://codereview.chromium.org/677533003/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_view.h:288: //
On 2014/10/27 23:06:00, msw wrote:
> On 2014/10/27 23:00:49, sky wrote:
> > On 2014/10/27 22:50:18, msw wrote:
> > > I don't think these two comment blocks should be joined.
> > 
> > Done.
> 
> nit: (still joined)

GAH! Sorry, misunderstood your comment. fixed.

[sky, 2014-10-27 23:14:03]

The CQ bit was checked by sky@chromium.org

[commit-bot: I haz the power, 2014-10-27 23:20:29]

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

[commit-bot: I haz the power, 2014-10-28 02:18:27]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2014-10-28 02:19:34]

Patchset 8 (id:??) landed as
https://crrev.com/b8c86e2bed280803ec89ae1d9f4acf9b6b29aed6
Cr-Commit-Position: refs/heads/master@{#301551}