Closed Bug 360345 Opened 18 years ago Closed 18 years ago

Bookmarks code style cleanup

Categories

(Camino Graveyard :: Bookmarks, defect)

PowerPC
macOS
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: stuart.morgan+bugzilla, Assigned: stuart.morgan+bugzilla)

References

Details

(Keywords: fixed1.8.1.1)

Attachments

(1 file, 1 obsolete file)

Bookmarks code is next on my style-compliance hit list.
Attached patch style fixes (obsolete) — Splinter Review
No behavioral changes, just lots of stylistic goodness.
Attachment #245319 - Flags: superreview?(mikepinkerton)
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
Attachment #245319 - Flags: review+
Comment on attachment 245319 [details] [diff] [review]
style fixes

rs=pink with froodian's fixes.
Attachment #245319 - Flags: superreview?(mikepinkerton) → superreview+
Attached patch v2Splinter Review
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.
Attachment #245319 - Attachment is obsolete: true
Attachment #245337 - Flags: superreview?(mikepinkerton)
Attachment #245337 - Flags: superreview?(mikepinkerton)
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.
Blocks: 325845
Blocks: 155956
Landed on MOZILLA_1_8_BRANCH
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Keywords: fixed1.8.1.1
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: