Make back forward menu model a MenuModel....

Make back forward menu model a MenuModel.

On Linux, implement just enough MenuModel support in GtkMenu. On Linux + windows, remove wrapper classes. On Mac, just change a few function calls around.

BUG=none
TEST=unit test + manually clicking around on it in debug mode


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

[Evan Stade, 2009-12-29 22:39:43]

thakis/trungl for mac side
erg/evan for the rest

(lots of reviewers because I don't know who all is in the office)

[Evan Martin, 2009-12-29 22:52:30]

This seems fine, but it's not my area; Elliot might be better.

Are you doing this to unify the page actions menu stuff?  I briefly glanced at
that and thought we ought to be sharing more code...

http://codereview.chromium.org/501168/diff/2001/2002
File chrome/browser/back_forward_menu_model.cc (right):

http://codereview.chromium.org/501168/diff/2001/2002#newcode108
chrome/browser/back_forward_menu_model.cc:108: TabContents* contents =
GetTabContents();
It appears this variable isn't needed except in the below expression

http://codereview.chromium.org/501168/diff/2001/2008
File chrome/browser/gtk/menu_gtk.h (right):

http://codereview.chromium.org/501168/diff/2001/2008#newcode58
chrome/browser/gtk/menu_gtk.h:58: // TYPE_COMMAND are currently implemented.
Do you DCHECK elsewhere to verify this?

http://codereview.chromium.org/501168/diff/2001/2008#newcode148
chrome/browser/gtk/menu_gtk.h:148: // FIXME
fix me :)

[viettrungluu, 2009-12-29 23:04:43]

Some quick corrections; I'll take a closer look in a bit.

http://codereview.chromium.org/501168/diff/2001/2018
File chrome/browser/cocoa/back_forward_menu_controller.mm (right):

http://codereview.chromium.org/501168/diff/2001/2018#newcode63
chrome/browser/cocoa/back_forward_menu_controller.mm:63: atIndex:menuID];
You need to start inserting at index 1, i.e., |atIndex:(menuID + 1)| (see
comment about 0-th item above).

http://codereview.chromium.org/501168/diff/2001/2018#newcode85
chrome/browser/cocoa/back_forward_menu_controller.mm:85: atIndex:menuID];
Ditto.

[Evan Stade, 2009-12-29 23:16:36]

http://codereview.chromium.org/501168/diff/2001/2002
File chrome/browser/back_forward_menu_model.cc (right):

http://codereview.chromium.org/501168/diff/2001/2002#newcode108
chrome/browser/back_forward_menu_model.cc:108: TabContents* contents =
GetTabContents();
On 2009/12/29 22:52:31, Evan Martin wrote:
> It appears this variable isn't needed except in the below expression

Done.

http://codereview.chromium.org/501168/diff/2001/2018
File chrome/browser/cocoa/back_forward_menu_controller.mm (right):

http://codereview.chromium.org/501168/diff/2001/2018#newcode63
chrome/browser/cocoa/back_forward_menu_controller.mm:63: atIndex:menuID];
On 2009/12/29 23:04:43, viettrungluu wrote:
> You need to start inserting at index 1, i.e., |atIndex:(menuID + 1)| (see
> comment about 0-th item above).

ah, thanks for pointing this out. Fixed. Note that this patch changes the
indexing on the BackForwardMenuModel to start at 0, not 1 (as with other
MenuModels). So there is now an offset of 1 between the index in the NSMenu and
the menuID.

http://codereview.chromium.org/501168/diff/2001/2008
File chrome/browser/gtk/menu_gtk.h (right):

http://codereview.chromium.org/501168/diff/2001/2008#newcode58
chrome/browser/gtk/menu_gtk.h:58: // TYPE_COMMAND are currently implemented.
On 2009/12/29 22:52:31, Evan Martin wrote:
> Do you DCHECK elsewhere to verify this? 

added dchecks.

http://codereview.chromium.org/501168/diff/2001/2008#newcode148
chrome/browser/gtk/menu_gtk.h:148: // FIXME
On 2009/12/29 22:52:31, Evan Martin wrote:
> fix me :)

Done.

[Evan Stade, 2009-12-29 23:25:39]

> Are you doing this to unify the page actions menu stuff?  I briefly glanced at
> that and thought we ought to be sharing more code...

yes

[viettrungluu, 2009-12-30 00:54:26]

LGTM with comments addressed on the model and the Mac part.

http://codereview.chromium.org/501168/diff/3004/2019
File chrome/browser/back_forward_menu_model.cc (right):

http://codereview.chromium.org/501168/diff/3004/2019#newcode97
chrome/browser/back_forward_menu_model.cc:97: return index < GetTotalItemCount()
&& !IsSeparator(index);
It seems to me that |index < GetTotalItemCount()| should always be true, in
which case maybe this condition should just be DCHECK()ed? (If we're paranoid,
we might want DCHECK()s on other instances of |index| as well.)

http://codereview.chromium.org/501168/diff/3004/2019#newcode101
chrome/browser/back_forward_menu_model.cc:101: return NULL;
NOTREACHED()?

http://codereview.chromium.org/501168/diff/3004/2019#newcode138
chrome/browser/back_forward_menu_model.cc:138:
UserMetrics::RecordComputedAction(BuildActionName("Popup", -1),
Really? (I mean this in the sense that "Popup" doesn't seem very descriptive,
and also that I'm not sure this is something we want to record.)

http://codereview.chromium.org/501168/diff/3004/2019#newcode290
chrome/browser/back_forward_menu_model.cc:290: return !IsSeparator(index) &&
index < GetTotalItemCount();
What I previously said about |index < GetTotalItemCount()|....

http://codereview.chromium.org/501168/diff/3004/2019#newcode294
chrome/browser/back_forward_menu_model.cc:294: return index <
GetTotalItemCount() && !IsSeparator(index);
Ditto (and if you keep it this way, could you make the order of the checks
consistent?).

http://codereview.chromium.org/501168/diff/3004/2027
File chrome/browser/back_forward_menu_model.h (right):

http://codereview.chromium.org/501168/diff/3004/2027#newcode87
chrome/browser/back_forward_menu_model.h:87: int GetTotalItemCount() const;
Is there a reason why this shouldn't just be replaced with |GetItemCount()|
(whose implementation just calls this)?

[Evan Stade, 2009-12-30 01:12:10]

http://codereview.chromium.org/501168/diff/3004/2019
File chrome/browser/back_forward_menu_model.cc (right):

http://codereview.chromium.org/501168/diff/3004/2019#newcode97
chrome/browser/back_forward_menu_model.cc:97: return index < GetTotalItemCount()
&& !IsSeparator(index);
On 2009/12/30 00:54:26, viettrungluu wrote:
> It seems to me that |index < GetTotalItemCount()| should always be true, in
> which case maybe this condition should just be DCHECK()ed? (If we're paranoid,
> we might want DCHECK()s on other instances of |index| as well.)

just copying the existing code.

http://codereview.chromium.org/501168/diff/3004/2019#newcode101
chrome/browser/back_forward_menu_model.cc:101: return NULL;
On 2009/12/30 00:54:26, viettrungluu wrote:
> NOTREACHED()?

this is not new code; it is taken from BackForwardMenuModelViews

also it appears to be the standard to leave it without a NOTREACHED()---perhaps
some code somewhere calls it without checking the type of the menu item.

http://codereview.chromium.org/501168/diff/3004/2019#newcode138
chrome/browser/back_forward_menu_model.cc:138:
UserMetrics::RecordComputedAction(BuildActionName("Popup", -1),
On 2009/12/30 00:54:26, viettrungluu wrote:
> Really? (I mean this in the sense that "Popup" doesn't seem very descriptive,
> and also that I'm not sure this is something we want to record.)

this is not new code; it is taken from BackForwardMenuModelViews

http://codereview.chromium.org/501168/diff/3004/2019#newcode290
chrome/browser/back_forward_menu_model.cc:290: return !IsSeparator(index) &&
index < GetTotalItemCount();
On 2009/12/30 00:54:26, viettrungluu wrote:
> What I previously said about |index < GetTotalItemCount()|....

ditto

http://codereview.chromium.org/501168/diff/3004/2019#newcode294
chrome/browser/back_forward_menu_model.cc:294: return index <
GetTotalItemCount() && !IsSeparator(index);
On 2009/12/30 00:54:26, viettrungluu wrote:
> Ditto (and if you keep it this way, could you make the order of the checks
> consistent?).

Done.

http://codereview.chromium.org/501168/diff/3004/2027
File chrome/browser/back_forward_menu_model.h (right):

http://codereview.chromium.org/501168/diff/3004/2027#newcode87
chrome/browser/back_forward_menu_model.h:87: int GetTotalItemCount() const;
On 2009/12/30 00:54:26, viettrungluu wrote:
> Is there a reason why this shouldn't just be replaced with |GetItemCount()|
> (whose implementation just calls this)?

yes.

1. I was trying to reduce the number of lines changed in this changelist, and
between the unit test and the implementation renaming this function required
changing about 20 lines of code where the logic didn't change at all
2. The comment explaining its implementation makes a lot more sense here, among
related functions, than it does above, among unrelated functions.

but I see that it could cause confusion, so I went ahead and made the change

[viettrungluu, 2009-12-30 01:20:33]

LGTM (on the aforementioned parts). After all, one shouldn't be expected to
fix/clean-up/re-think all the code that one moves around.

http://codereview.chromium.org/501168/diff/3004/2019
File chrome/browser/back_forward_menu_model.cc (right):

http://codereview.chromium.org/501168/diff/3004/2019#newcode101
chrome/browser/back_forward_menu_model.cc:101: return NULL;
On 2009/12/30 01:12:10, Evan Stade wrote:
> On 2009/12/30 00:54:26, viettrungluu wrote:
> > NOTREACHED()?
> 
> this is not new code; it is taken from BackForwardMenuModelViews

To be clear, I'm not blaming you for the code. I just like shooting messengers,
so to speak. ;-)

> also it appears to be the standard to leave it without a
NOTREACHED()---perhaps
> some code somewhere calls it without checking the type of the menu item.

Fair enough, though some methods get a NOTREACHED() (e.g., |IsItemCheckedAt()|),
hence my confusion.

[Evan Stade, 2009-12-30 01:32:49]

On Tue, Dec 29, 2009 at 5:20 PM, <viettrungluu@chromium.org> wrote:

> LGTM (on the aforementioned parts). After all, one shouldn't be expected to
> fix/clean-up/re-think all the code that one moves around.
>
>
>
> http://codereview.chromium.org/501168/diff/3004/2019
> File chrome/browser/back_forward_menu_model.cc (right):
>
> http://codereview.chromium.org/501168/diff/3004/2019#newcode101
> chrome/browser/back_forward_menu_model.cc:101: return NULL;
> On 2009/12/30 01:12:10, Evan Stade wrote:
>
>> On 2009/12/30 00:54:26, viettrungluu wrote:
>> > NOTREACHED()?
>>
>
>  this is not new code; it is taken from BackForwardMenuModelViews
>>
>
> To be clear, I'm not blaming you for the code. I just like shooting
> messengers, so to speak. ;-)
>
>
>  also it appears to be the standard to leave it without a
>>
> NOTREACHED()---perhaps
>
>> some code somewhere calls it without checking the type of the menu
>>
> item.
>
> Fair enough, though some methods get a NOTREACHED() (e.g.,
> |IsItemCheckedAt()|), hence my confusion.


oh yeah, good point, I'll remove those


>
>
> http://codereview.chromium.org/501168
>

[Evan Stade, 2009-12-30 18:18:40]

Evan, can you continue with the review as it seems Elliot is gone? thanks

[Evan Martin, 2009-12-30 19:29:30]

LGTM

http://codereview.chromium.org/501168/diff/3004/2019
File chrome/browser/back_forward_menu_model.cc (right):

http://codereview.chromium.org/501168/diff/3004/2019#newcode307
chrome/browser/back_forward_menu_model.cc:307: int
BackForwardMenuModel::MenuIdToNavEntryIndex(int index) const {
This is a bit weird, that the function name says it takes an id and you renamed
it to "index".

[Evan Stade, 2009-12-30 20:32:30]

http://codereview.chromium.org/501168/diff/3004/2019
File chrome/browser/back_forward_menu_model.cc (right):

http://codereview.chromium.org/501168/diff/3004/2019#newcode307
chrome/browser/back_forward_menu_model.cc:307: int
BackForwardMenuModel::MenuIdToNavEntryIndex(int index) const {
On 2009/12/30 19:29:30, Evan Martin wrote:
> This is a bit weird, that the function name says it takes an id and you
renamed
> it to "index".

will rename the function