360345 - Bookmarks code style cleanup

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Stuart Morgan]

Bookmarks code is next on my style-compliance hit list.

Comment 1

[Stuart Morgan]

Attachment: style fixes

No behavioral changes, just lots of stylistic goodness.

Comment 2

[froodian (Ian Leue)]

Comment on attachment 245319 [details] [diff] [review]
style fixes

As always, I'm going to mention that I like (Foo *) better than (Foo*), but it's just a matter of taste - it seems there are instances of both in these files and you've just left them as they are, so let's stick with that.

>   NSDictionary *uriDict = [aDict objectForKey:SafariURIDictKey];

NSDictionary* uriDict.  In the context, I see that there are a bunch of other instances of Foo *bar which you left as is, so I guess it'd be "fix all or fix none."

>-    return [[[NSIndexSpecifier allocWithZone:[self zone]] initWithContainerClassDescription:aRef containerSpecifier:containerRef key:@"childArray" index:index] autorelease];
>-  } else
>+    return [[[NSIndexSpecifier allocWithZone:[self zone]] initWithContainerClassDescription:aRef
>+                                                                         containerSpecifier:containerRef
>+                                                                                        key:@"childArray"
>+                                                                                      index:index] autorelease];
>+  }
>+  else
>     return nil;
> }

By moz code style, no else after a return.

>   // If the drag results in the bookmark button being (re)moved, it could get
>   // deallocated too soon.  This occurs with SDK >= 10.3, but not earlier.
>   // Change in cleanup strategy?  Hold on tight.
>   [[self retain] autorelease];
>-  [self dragImage: [MainController createImageForDragging:[self image]
>-                                                    title:([item isSeparator] ? @"" : title)]
>-               at: NSMakePoint(0,NSHeight([self bounds])) offset: NSMakeSize(0,0)
>-            event: aEvent pasteboard: pboard source: self slideBack: YES];
>+  [self dragImage:[MainController createImageForDragging:[self image]
>+                                                   title:([item isSeparator] ? @"" : title)]
>+               at:NSMakePoint(0, NSHeight([self bounds])) offset:NSMakeSize(0, 0)
>+            event:aEvent pasteboard:pboard source:self slideBack:YES];
> }

This still looks really confusing.  Maybe offset, event, pasteboard, source, and slideback should get their own lines?  As it is, it's difficult to see the nesting of the message calls.

>       // need to get the appropriate class description
>-      return [[[NSIndexSpecifier allocWithZone:[self zone]] initWithContainerClassDescription:arrayScriptClassDesc containerSpecifier:containerRef key:@"childArray" index:index] autorelease];
>-    } else 
>+      return [[[NSIndexSpecifier allocWithZone:[self zone]] initWithContainerClassDescription:arrayScriptClassDesc
>+                                                                           containerSpecifier:containerRef
>+                                                                                          key:@"childArray"
>+                                                                                        index:index] autorelease];
>+    }
>+    else
>       return nil;
>-  } else  // root bookmark
>+  }
>+  else  // root bookmark
>     return [[[NSPropertySpecifier allocWithZone:[self zone]] initWithContainerSpecifier:containerRef key:@"rootArray"] autorelease];
>   return nil;
> }

No elses after return

>   [self showProgressView];
>-  NSDictionary *aDict = [NSDictionary dictionaryWithObjectsAndKeys: aPathArray, kBookmarkImportPathIndentifier, 
>+  NSDictionary *aDict = [NSDictionary dictionaryWithObjectsAndKeys: aPathArray, kBookmarkImportPathIndentifier,
>     aTitleArray, kBookmarkImportNewFolderNameIdentifier, nil];

dictionaryWithObjectsAndKeys:aPathArray

>@@ -1817,7 +1759,7 @@
>       else if ([tokenTag isEqualToString:@"<HR"]) {
>           currentItem = [currentArray addBookmark];
>           [(Bookmark *)currentItem setIsSeparator:YES];
>-          [fileScanner setScanLocation:(scanIndex+1)];          
>+          [fileScanner setScanLocation:(scanIndex+1)];
>       }

(scanIndex + 1)


>@@ -1982,14 +1924,13 @@
> - (void)bookmarkAdded:(NSNotification *)note
> {
>   BookmarkItem* addedItem = [note object];
>-  if ((addedItem == [[BookmarkManager sharedBookmarkManager] rootBookmarks]))
>-  {
>+  if ((addedItem == [[BookmarkManager sharedBookmarkManager] rootBookmarks])) {
>     [mContainersTableView reloadData];
>     BookmarkFolder* updatedFolder = [[note userInfo] objectForKey:BookmarkFolderChildKey];
>     [self selectContainerFolder:updatedFolder];
>     return;

>-  
>-  if ((removedItem == [[BookmarkManager sharedBookmarkManager] rootBookmarks]))
>-  {
>+
>+  if ((removedItem == [[BookmarkManager sharedBookmarkManager] rootBookmarks])) {
>     [mContainersTableView reloadData];
>     return;

Do the inner parens actually do anything in these cases?

r=me with or without theses changes

Comment 3

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 245319 [details] [diff] [review]
style fixes

rs=pink with froodian's fixes.

Comment 4

[Stuart Morgan]

Attachment: v2

Made those changes, and a handful of others I missed.

I didn't change Foo*/Foo * anywhere in any of the files, because I decided it's not worth trying to make consistent even per file when it's very likely to diverge again anyway.

Comment 5

[Stuart Morgan]

Branch needs to wait until bug 358620 lands (which is waiting for the branch SDK switch), but I landed this on the trunk so we can start making patches against bookmark code.

Comment 6

[Stuart Morgan]

Landed on MOZILLA_1_8_BRANCH