From:  bugzilla-request-daemon@mozilla.org
Date:  18 Jan 2006 11:25:04 Hong Kong Time
Newsgroup:  news.mozilla.org/netscape.public.mozilla.reviewers
Subject:  

superreview denied: [Bug 322592] Software Update Menu Item : [Attachment 208791] Patch v3

NNTP-Posting-Host:  207.126.111.202

Simon Fraser  has denied H嶡an Waara
's request for superreview:
Bug 322592: Software Update Menu Item
https://bugzilla.mozilla.org/show_bug.cgi?id=322592

Attachment 208791: Patch v3
https://bugzilla.mozilla.org/attachment.cgi?id=208791&action=edit

------- Additional Comments from Simon Fraser 
>Index: application/UpdateChecker.h
>===================================================================

>+// NSURLHandler implementation
>+- (void)URLResourceDidFinishLoading:(NSURL *)sender;
>+- (void)URL:(NSURL *)sender resourceDidFailLoadingWithReason:(NSString
*)reason;
>+
>+- (void)bailOut;

None of these need to be in the header, and should not be.

>Index: application/UpdateChecker.mm
>===================================================================


>+  // decodes the hex we get back from |systemVersion:| into an int, (e.g.,
0x1043 -> "1043")
>+  NSString *thisSystemVersion = [NSString stringWithFormat:@"%x",
[NSWorkspace systemVersion]];

Decodes or encodes?

>+- (void)URLResourceDidFinishLoading:(NSURL *)sender
>+{
>+  NSData *data = [sender resourceDataUsingCache:YES];

This is going to re-fetch (read the docs).

What you should do is implement 

- (void)URL:(NSURL *)sender resourceDataDidBecomeAvailable:(NSData *)newBytes

and append the new data into an NSMutableData member var.

>+  // no keys means no new version
>+  if ([root count] > 0)
>+  {
>+    NSDate *releaseDate = [root valueForKey:@"release_date"];
>+    NSString *releaseURL = [root valueForKey:@"release_url"];
>+    NSString *releaseVersion = [root valueForKey:@"release_version"];
>+    int daysAgo = 0;
>+    
>+    NSCalendarDate *dateDiff = [[NSCalendarDate alloc] init];
>+    
>+    [dateDiff years:NULL months:NULL days:&daysAgo 
>+		hours:NULL minutes:NULL seconds:NULL 
>+	    sinceDate:releaseDate];
>+    
>+    [dateDiff release];
>+    
>+    NSString *message = [NSString
stringWithFormat:NSLocalizedString(@"UpdateMsg", @""), releaseVersion,
daysAgo];
>+    
>+    // If there's a new version, offer the user to go to a webpage for
downloading the update.
>+    if (NSRunAlertPanel (NSLocalizedString (@"UpdateTitle", @""), message, 
>+			   NSLocalizedString (@"DownloadButtonText", @""), 
>+			   NSLocalizedString (@"CancelButtonText", @""), nil)
== NSAlertDefaultReturn)
>+    {
>+	MainController *mainController = (MainController*)[NSApp delegate];
>+	[mainController openNewWindowOrTabWithURL:releaseURL andReferrer:nil
alwaysInFront:YES];
>+    }
>+  }
>+  else
>+  {
>+    NSRunAlertPanel (NSLocalizedString (@"NoUpdateTitle", @""),
>+		       NSLocalizedString (@"NoUpdateMsg", @""),
>+		       NSLocalizedString (@"OKButtonText", @""), nil, nil);

Do we want a modal dialog in this case?

>Index: application/MainController.mm
>===================================================================

>+-(IBAction)checkForUpdates:(id)sender
>+{
>+  UpdateChecker *updater = [UpdateChecker alloc];


Always init your objects.

>+  [updater checkForUpdate];

Who releases this?
>+}