Removed separator in bookmarks sub-menu if there are no bookmarks or folders to display (only fix...

Removed separator in bookmarks sub-menu if there are no bookmarks or folders to display (only fixed fo Windows)

Contributed by caitkp@chromium.org

BUG=89572
TEST= Open a browser, remove all bookmarks, go to wrench-menu-->bookmarks. No separator should appear at the end of the sub-menu. Now create a bookmark and repeat. A separator should appear, separating the bookmark items from the other menu items.

R=sky@chromium.org,rogerta@chromium.org


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93883

[Cait (Slow), 2011-07-21 19:52:32]

Hi Roger,

Here is the bookmark fix for you to review.

-Cait

[Roger Tawa OOO till Jul 10th, 2011-07-21 20:41:48]

Hi Cait,

Some comments below.  I have also started trybots for this change, you should
get an email response with the results.

http://codereview.chromium.org/7483003/diff/1/chrome/browser/ui/toolbar/wrenc...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

http://codereview.chromium.org/7483003/diff/1/chrome/browser/ui/toolbar/wrenc...
chrome/browser/ui/toolbar/wrench_menu_model.cc:203: #if !defined(OS_MACOSX) &&
!defined(TOOLKIT_VIEWS)
I suggested the wrong thing to you earlier, sorry.  Instead of reversing the
condition, keep the #ifdef defined(OS_MACOSX) part, since your change in the
other file only affects Views.

http://codereview.chromium.org/7483003/diff/1/chrome/browser/ui/views/bookmar...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

http://codereview.chromium.org/7483003/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:366: 
I think it would be better to check if the menu argument has a submenu and if
that submenu already has more than one child.  If both conditions are true, then
add the separator.   What do you think?

fyi, please take a look at the coding style at
http://www.chromium.org/developers/coding-style.  The style says that line 363
would need to be indented one space more, and line 364 would need to be indented
one space less.

[Roger Tawa OOO till Jul 10th, 2011-07-21 20:43:54]

http://codereview.chromium.org/7483003/diff/1/chrome/browser/ui/views/bookmar...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

http://codereview.chromium.org/7483003/diff/1/chrome/browser/ui/views/bookmar...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:366: 
On 2011/07/21 20:41:48, Roger Tawa wrote:
> I think it would be better to check if the menu argument has a submenu and if
> that submenu already has more than one child.  If both conditions are true,
then
> add the separator.   What do you think?
> 
> fyi, please take a look at the coding style at
> http://www.chromium.org/developers/coding-style.  The style says that line 363
> would need to be indented one space more, and line 364 would need to be
indented
> one space less.

Sorry, meant to say "one or more children", not "more than one child".

[Roger Tawa OOO till Jul 10th, 2011-07-22 19:32:42]

lgtm

Looks good Cait.

[Cait (Slow), 2011-07-22 20:34:01]

Hi Scott,

Can you take a quick look at this CL? I need to get it reviewed by an owner.

Thanks much,
Cait

[sky, 2011-07-22 21:09:09]

http://codereview.chromium.org/7483003/diff/6001/chrome/browser/ui/views/book...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

http://codereview.chromium.org/7483003/diff/6001/chrome/browser/ui/views/book...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:362: if
((start_child_index < parent->child_count()) &&
Couple of problems with this:
I don't like putting this check in a spot that is recursively called when all we
really care about is the first menu. It should be around line 67.
GetSubmenu can return null.
This doesn't deal with the case of there be no bookmarks on the bookmark bar,
but bookmarks in the other folder.

[Cait (Slow), 2011-07-22 22:12:44]

Thanks for the feedback. Please have a look at patch set 3, which addresses the
issues you brought up.

-Cait

On 2011/07/22 21:09:09, sky wrote:
>
http://codereview.chromium.org/7483003/diff/6001/chrome/browser/ui/views/book...
> File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):
> 
>
http://codereview.chromium.org/7483003/diff/6001/chrome/browser/ui/views/book...
> chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:362: if
> ((start_child_index < parent->child_count()) &&
> Couple of problems with this:
> I don't like putting this check in a spot that is recursively called when all
we
> really care about is the first menu. It should be around line 67.
> GetSubmenu can return null.
> This doesn't deal with the case of there be no bookmarks on the bookmark bar,
> but bookmarks in the other folder.

[tfarina, 2011-07-22 22:32:46]

http://codereview.chromium.org/7483003/diff/11001/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

http://codereview.chromium.org/7483003/diff/11001/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:75: if
(other_folder->child_count() > 0) {
while you are here, can you change this check to !other_folder->empty() ?

[sky, 2011-07-22 22:47:00]

LGTM

http://codereview.chromium.org/7483003/diff/11001/chrome/browser/ui/views/boo...
File chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc (right):

http://codereview.chromium.org/7483003/diff/11001/chrome/browser/ui/views/boo...
chrome/browser/ui/views/bookmarks/bookmark_menu_delegate.cc:362: 
remove this line.

[commit-bot: I haz the power, 2011-07-25 15:27:45]

Change committed as 93883