gtk: Add context menu to Bookmark Editor.

gtk: Add context menu to Bookmark Editor.

BUG=63073
TEST=see bug

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

[tfarina, 2010-11-13 18:43:01]

Hi Evan, could you review this to me?

[evanm, 2010-11-13 18:50:12]

estade is probably your best bet for a good review, i just have a minor style
suggestion

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
File chrome/browser/gtk/bookmark_editor_gtk.h (right):

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.h:115: void ShowContextMenu();
Can you add docs to these new functions?

I guess from looking at this header it might be cleaner to put them above the
CHROMEGTK_CALLBACK lines (so around line 100).

[Evan Stade, 2010-11-15 04:01:13]

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
File chrome/browser/gtk/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:58: running_menu_for_root_ =
GetSelectedNode()->GetParent()->IsRoot();
GetSelectedNode() can return null. this segfaults

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:69: return (command_id != IDS_EDIT ||
!running_menu_for_root_) &&
clearer if you change this from !a || !b to !(a and b)

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:74: }
blank lines between functions

[tfarina, 2010-11-15 12:43:00]

PTAL!

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
File chrome/browser/gtk/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:58: running_menu_for_root_ =
GetSelectedNode()->GetParent()->IsRoot();
On 2010/11/15 04:01:13, Evan Stade wrote:
> GetSelectedNode() can return null. this segfaults

Done.

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:69: return (command_id != IDS_EDIT ||
!running_menu_for_root_) &&
On 2010/11/15 04:01:13, Evan Stade wrote:
> clearer if you change this from !a || !b to !(a and b)

Done.

http://codereview.chromium.org/4973001/diff/2001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:74: }
On 2010/11/15 04:01:13, Evan Stade wrote:
> blank lines between functions

Done.

[Evan Stade, 2010-11-15 18:57:03]

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
File chrome/browser/gtk/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:49:
menu_model_->AddItemWithStringId(IDS_EDIT, IDS_EDIT);
shouldn't one of these things be IDC_EDIT

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:51:
IDS_BOOMARK_EDITOR_NEW_FOLDER_MENU_ITEM,
ditto

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:87: return;
insert blank line

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:94: }
insert blank line

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:131: }
insert blank line

[tfarina, 2010-11-15 19:50:08]

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
File chrome/browser/gtk/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:49:
menu_model_->AddItemWithStringId(IDS_EDIT, IDS_EDIT);
On 2010/11/15 18:57:04, Evan Stade wrote:
> shouldn't one of these things be IDC_EDIT

Nope, we don't have IDC_EDIT.

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:51:
IDS_BOOMARK_EDITOR_NEW_FOLDER_MENU_ITEM,
On 2010/11/15 18:57:04, Evan Stade wrote:
> ditto

Same thing here.

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:87: return;
On 2010/11/15 18:57:04, Evan Stade wrote:
> insert blank line

Done.

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:94: }
On 2010/11/15 18:57:04, Evan Stade wrote:
> insert blank line

Done.

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:131: }
On 2010/11/15 18:57:04, Evan Stade wrote:
> insert blank line

Done.

[Evan Stade, 2010-11-15 20:41:42]

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
File chrome/browser/gtk/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:49:
menu_model_->AddItemWithStringId(IDS_EDIT, IDS_EDIT);
On 2010/11/15 19:50:08, tfarina wrote:
> On 2010/11/15 18:57:04, Evan Stade wrote:
> > shouldn't one of these things be IDC_EDIT
> 
> Nope, we don't have IDC_EDIT.

nevertheless, overloading the string ids seems like a path to pain. Please make
an enum (like in TabMenuModel)

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:88: if (command_id == IDS_EDIT) {
prefer switch/case here

[tfarina, 2010-11-15 21:09:26]

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
File chrome/browser/gtk/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:49:
menu_model_->AddItemWithStringId(IDS_EDIT, IDS_EDIT);
On 2010/11/15 20:41:42, Evan Stade wrote:
> On 2010/11/15 19:50:08, tfarina wrote:
> > On 2010/11/15 18:57:04, Evan Stade wrote:
> > > shouldn't one of these things be IDC_EDIT
> > 
> > Nope, we don't have IDC_EDIT.
> 
> nevertheless, overloading the string ids seems like a path to pain. Please
make
> an enum (like in TabMenuModel)

Done.

http://codereview.chromium.org/4973001/diff/8001/chrome/browser/gtk/bookmark_...
chrome/browser/gtk/bookmark_editor_gtk.cc:88: if (command_id == IDS_EDIT) {
On 2010/11/15 20:41:42, Evan Stade wrote:
> prefer switch/case here

Done.

[Evan Stade, 2010-11-15 21:13:35]

thank you, lgtm.

[Evan Stade, 2010-11-15 21:13:43]

http://codereview.chromium.org/4973001/diff/18001/chrome/browser/gtk/bookmark...
File chrome/browser/gtk/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/4973001/diff/18001/chrome/browser/gtk/bookmark...
chrome/browser/gtk/bookmark_editor_gtk.cc:45: enum ContextMenuCommand {
shoudl be private

[tfarina, 2010-11-15 23:45:07]

http://codereview.chromium.org/4973001/diff/18001/chrome/browser/gtk/bookmark...
File chrome/browser/gtk/bookmark_editor_gtk.cc (right):

http://codereview.chromium.org/4973001/diff/18001/chrome/browser/gtk/bookmark...
chrome/browser/gtk/bookmark_editor_gtk.cc:45: enum ContextMenuCommand {
On 2010/11/15 21:13:43, Evan Stade wrote:
> shoudl be private

Done.