145802 - Implement session history menu (Go)

Status: VERIFIED FIXED

(Camino Graveyard :: History, defect)

Description

[Winnie Lam]

Implement session history menu (Go).

Comment 1

[Joe Hewitt (gone)]

Attachment: patch

Comment 2

[Mike Pinkerton (not reading bugmail)]

some comments:

remove the webProgressListener and ProgressDlgController fixes. I'm going to
land those on the tip tonight.

+// the tag of the separator after which to insert history menu items
+#define INSERTION_TAG 4000
+// the maximum number of history entry menuitems to display
+#define MAX_MENUITEMS 15

#defines are bad c++. Use enums or const int's.

+  // get controller for current window
+  BrowserWindowController *controller;
+  controller = (BrowserWindowController*)[[[NSApplication sharedApplication]
mainWindow] windowController];
+  if (!controller) return nsnull;

two things about this:
- remember this is c++, merge the declaration and the init onto 1 line.
- you're creating a new NSApplication here. you really want to use the global
|NSApp| instead:

BrowserWindowController* controller = [[NSApp mainWindow] windowController];
if (!controller) return nsnull;

+  int i = 0;
+  for (i = [self numberOfItems]-1; i > insertionIndex ; --i) {

don't need to worry about win32, so let's do this all on one line:
  for ( int i = [self ...] ... )

+    NSMenuItem *newItem = [self addItemWithTitle:title
action:@selector(historyItemAction:) keyEquivalent:@""];

what if the title is really long? You should proably middle-truncate it at 15-20
chars, or something reasonable, so the menu doesn't get too wide.

It also seems to me that your numbering might be off. You delete everything
after |INSERTION_TAG|, implying that it's always left in the menus. Did you give
the divider item that tag in interface builder? You then add things with tag:
+    [newItem setTag:INSERTION_TAG+i];

problem is, |i| can be 0, so you end up adding another tag of |INSERTION_TAG|.
Will cocoa freak out with two tags when you search for it in emptyHistoryItems:?
I'd guess not, but why take that chance? Or am i misreading?

Comment 3

[Joe Hewitt (gone)]

fixed

Comment 4

[Winnie Lam]

Test cases for this feature:
http://www.mozilla.org/quality/browser/front-end/testcases/history/

Use only session history test cases and not global history test cases.

Comment 5

[Chris Petersen]

Verified in the 2002-06-04-05 trunk