413146 - Refactor GoMenu/HistoryMenu

Status: RESOLVED FIXED

(Camino Graveyard :: History, defect)

Description

[Stuart Morgan]

Attachment: refactoring

HistoryMenu has a lot of logic that's specific to GoMenu (which is a subclass) embedded in it directly. This pulls the GoMenu-specific pieces into the GoMenu class where they belong. (I'll split HistoryMenu and GoMenu apart and do the class renaming later, so it's not mixed with real code changes). I'm sure there's more cleanup to do in these classes, but this seemed like a good stopping point for now.

I also cleaned up the way the "Show More" item is added to fix a bug that we'd never noticed with the Earlier Today menu.

Comment 1

[Mark Mentovai]

Comment on attachment 297986 [details] [diff] [review]
refactoring

>+  if ([inItem respondsToSelector:@selector(isTodayCategory)] &&
>+      [(HistoryDateCategoryItem*)inItem isTodayCategory])
>+  {

After moving all of those existing opening braces up a line, you're going to introduce a new lonely one?  This happens in menuItemTitleForHistoryItem: and again in todayMenuItem.

>+  unsigned int maxItems = MIN(kMaxTodayItems, [[mTodayItem children] count]);

MIN?  Yeah, I know Apple gives us a fun definition of MIN, but that's only if their header is included before something that's got the more classic naive MIN definition.  And also...MIN?  I'd be more comfortable with a temporary variable for the child count, or, since this is an .mm and C++ is cool, #include <algorithm> and call std::min.  It'll be inlined anyway.  See, C++ isn't that bad after all!

>+- (void)historyChanged:(NSNotification*)inNotification

Seems like too many blank lines above this.

>   [inMenuItem setKeyEquivalentModifierMask:0]; // Needed since by default NSMenuItems have NSCommandKeyMask
>+  // Find the item we are adding alternates for. Since this is generally used

These two things aren't really all that related, can you borrow the blank line from the previous comment and stick it in here?

>+  [self insertItem:altMenuItem atIndex:(itemIndex +2)];

And a space in there?

When I see stuff like this, I wonder if we'd be better off with [altMenuItem copy] and then a [-NSMenuItem setKeyEquivalentModifierMask:] call to set the only attribute that changes between the two.

Comment 2

[Stuart Morgan]

(In reply to comment #1)
> (From update of attachment 297986 [details] [diff] [review])
> >+  if ([inItem respondsToSelector:@selector(isTodayCategory)] &&
> >+      [(HistoryDateCategoryItem*)inItem isTodayCategory])
> >+  {
> 
> After moving all of those existing opening braces up a line, you're going to
> introduce a new lonely one?

Yep, that's what our style rules say for multi-line conditionals (although I'm growing to like it less and less).

Landed on trunk and MOZILLA_1_8_BRANCH with the other changes (and a tiny fix for not showing an empty Earlier Today folder, which I noticed in final testing).