341926 - Cleanup in MainController.mm -validateMenuItem

Status: VERIFIED FIXED

(Camino Graveyard :: General, defect)

Description

[froodian (Ian Leue)]

The validateMenuItem code is inconsistent, and fails to follow the coding style guidelines (spaces vs. tabs, ![foo bar] vs [foo bar] == nil, no else after returns).  Also, the viewSource selector has its own if statement, even though it could be placed under one of the group ifs.  Patch forthcoming.

Comment 1

[froodian (Ian Leue)]

Attachment: Patch

This changes no behavior (hopefully) - It just cleans up.

Comment 2

[Bruce Davidson]

Comment on attachment 226053 [details] [diff] [review]
Patch

One nit,

>+      /* action == @selector(goHome:) || */           // always enabled
>+      /* action == @selector(doSearch:) || */         // always enabled

No need to leave these two lines in, the comment above the if adequately describes what we want to enable/disable, and its pretty clear these should be enabled even when the bookmark manager is visible.

With that fixed r+ = me.

Comment 3

[froodian (Ian Leue)]

Attachment: r=bruced patch

Comment 4

[Chris Lawson (gone)]

Comment on attachment 226187 [details] [diff] [review]
r=bruced patch

second-r=me.

Can we get this checked in ASAP, since Ian and I are about to tackle a huge pile of code in this area and we'd prefer this not bitrot?

Requesting sr from pink.

cl

Comment 5

[froodian (Ian Leue)]

Attachment: Unbitrots it

> Can we get this checked in ASAP, since Ian and I are about to tackle a huge
> pile of code in this area and we'd prefer this not bitrot?

Too late. ;)  This brings the patch up to date with the current tree.  The above comment still applies. :)

Comment 6

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 226597 [details] [diff] [review]
Unbitrots it

sr=pink

Comment 7

[Nick Kreeger]

   // check what the state of the personal toolbar should be, but only if there is a browser
   // window open. Popup windows that have the personal toolbar removed should always gray
   // out this menu.
   if (action == @selector(toggleBookmarksToolbar:)) {
     if (browserController) {
       NSView* bookmarkToolbar = [browserController bookmarkToolbar];
       if ( bookmarkToolbar ) {
         float height = [bookmarkToolbar frame].size.height;
         BOOL toolbarShowing = (height > 0);
         if (toolbarShowing)
           [mBookmarksToolbarMenuItem setTitle: NSLocalizedString(@"Hide Bookmarks Toolbar",@"")];
         else
           [mBookmarksToolbarMenuItem setTitle: NSLocalizedString(@"Show Bookmarks Toolbar",@"")];
         return YES;
       }
-      else
-        return NO;
-    }
-    else
       return NO;
+    }
+    return NO;
   }

Why is there a need for these two return NO calls?

Comment 8

[Nick Kreeger]

   // check what the state of the personal toolbar should be, but only if there is a browser
   // window open. Popup windows that have the personal toolbar removed should always gray
   // out this menu.
   if (action == @selector(toggleBookmarksToolbar:)) {
     if (browserController) {
       NSView* bookmarkToolbar = [browserController bookmarkToolbar];
       if ( bookmarkToolbar ) {
         float height = [bookmarkToolbar frame].size.height;
         BOOL toolbarShowing = (height > 0);
         if (toolbarShowing)
           [mBookmarksToolbarMenuItem setTitle: NSLocalizedString(@"Hide Bookmarks Toolbar",@"")];
         else
           [mBookmarksToolbarMenuItem setTitle: NSLocalizedString(@"Show Bookmarks Toolbar",@"")];
         return YES;
       }
-      else
-        return NO;
-    }
-    else
       return NO;
+    }
+    return NO;
   }

Why is there a need for these two return NO calls?

Comment 9

[froodian (Ian Leue)]

Attachment: Even Better

Gets rid of the extra Return NO

Comment 10

[Nick Kreeger]

Fixed trunk and branch