GTK: Implement popup favicon menu.

GTK: Implement popup favicon menu.

BUG=18181

[tony, 2009-09-28 23:00:41]

http://codereview.chromium.org/244029/diff/3001/3004
File chrome/browser/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/244029/diff/3001/3004#newcode153
Line 153: standard_page_menu_length += 2;
I don't understand this.  You add 2, but then both times you use it, you
subtract 2.  I think the second use of it, you are copying too much data.

http://codereview.chromium.org/244029/diff/3001/3004#newcode158
Line 158: memcpy(favicon_menu + arraysize(g_favicon_menu), standard_page + 2,
Can you add a comment that the +2 is to skip the create shortcut menu?  Also,
this seems fragile if we change the page menu order, but I don't have any great
suggestions.

[Evan Stade, 2009-09-28 23:15:39]

http://codereview.chromium.org/244029/diff/3001/3004
File chrome/browser/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/244029/diff/3001/3004#newcode153
Line 153: standard_page_menu_length += 2;
On 2009/09/28 23:00:42, tony wrote:
> I don't understand this.  You add 2, but then both times you use it, you
> subtract 2.  I think the second use of it, you are copying too much data.

yea, it's confusing. The +2 is because i is an index and we want a length (+1)
and because we broke out of the for loop before the last assignment, so
standard_page_menu is pointing at the second-to-last member of the array (+1).

I don't think the second use is copying too much.

http://codereview.chromium.org/244029/diff/3001/3004#newcode158
Line 158: memcpy(favicon_menu + arraysize(g_favicon_menu), standard_page + 2,
On 2009/09/28 23:00:42, tony wrote:
> Can you add a comment that the +2 is to skip the create shortcut menu?  Also,
> this seems fragile if we change the page menu order, but I don't have any
great
> suggestions.

yea, it's fragile, and it's something that's used infrequently enough that a
breakage could go a while before anyone notices. I'll make it more robust.

[tony, 2009-09-28 23:23:09]

http://codereview.chromium.org/244029/diff/3001/3004
File chrome/browser/gtk/browser_titlebar.cc (right):

http://codereview.chromium.org/244029/diff/3001/3004#newcode153
Line 153: standard_page_menu_length += 2;
Oh, now I understand.  Yes, this looks correct.  Can you add a comment? 
Alternately, maybe this would all be less confusing if you just copied each
element in a for loop that skipped IDC_CREATE_SHORTCUTS and the separator after
it.

[Evan Stade, 2009-09-30 01:27:30]

updated; more verbose but hopefully easier to understand and slightly less
likely to break/more likely to be discovered.

[Evan Stade, 2009-09-30 20:53:49]

ping

[tony, 2009-09-30 22:14:10]

LGTM.  Yes, I think this is easier to understand.