322592 - Software Update Menu Item

Status: VERIFIED WONTFIX

(Camino Graveyard :: General, defect)

Description

[Samuel Sidler (old account; do not CC)]

We need a software/application update menu item for 1.0. This item will simply check an XML file on m.o for updates. If there is an update, it will open a new tab/window (depending on the preference set) with a page on m.o/projects/camino which lists the latest update and a set of features in said update as well as a link for download.

The menu item should be in the Application menu, not the Help menu, for higher visibility as well as because it's not really a "help" function.

This differs from the other bug which is a more complete implementation that will update Camino automagically, if specified.

Comment 1

[Samuel Sidler (old account; do not CC)]

Also: If there isn't an update, the window will report back and say "No new releases found" or something similar.

If a user is using a nightly, we could send them to a page telling them that and giving them links to the latest release.

Comment 2

[Jasper]

If we could make it this simple, I hope we can indeed do this for 1.0.

Comment 3

[Samuel Sidler (old account; do not CC)]

CCing Chase for any questins we may have related to getting our XML file on AUS.

Comment 4

[Chase Phillips]

AUS is an application platform.  I'm not sure it should be used to store this XML file.  Perhaps another m.o property (eg, www.m.o/projects/camino/)?

Comment 5

[Benjamin Smedberg]

or caminobrowser.org

Can you set up "update.caminobrowser.org", so that if scaling becomes an issue it can be moved to a separate server?

Comment 6

[Samuel Sidler (old account; do not CC)]

(In reply to comment #5)
> or caminobrowser.org
> 
> Can you set up "update.caminobrowser.org", so that if scaling becomes an issue
> it can be moved to a separate server?
> 

We'd prefer to keep it on mozilla.org. By default, all of our links direct to mozilla.org and are redirected from there. This makes sure that if caminobrowser.org should ever go down, links can still be good.

Comment 7

[Chase Phillips]

(In reply to comment #6)
> We'd prefer to keep it on mozilla.org. By default, all of our links direct to
> mozilla.org and are redirected from there. This makes sure that if
> caminobrowser.org should ever go down, links can still be good.

If I understand bsmedberg's suggestion correctly, he was interested in the name being 'update.caminobrowser.org' to ensure that we can segregate that service from caminobrowser.org, or www.m.o, or update.m.o, ... if the traffic gets to be too high.  It's not that update.caminobrowser.org would need to not be aliased to one of those systems, it's just a safety net that would allow us (or caminobrowser.org) to treat traffic for that URL differently than general traffic to those systems.

Comment 8

[Håkan Waara]

So technically, I'd propose a .plist-file (as discussed on #camino, since we need to keep it simple and 10.2.8-compatible).  I propose the .plist include the following info:

* the latest version for a certain Mac OS X version
* a URL for that update

so basically 10.2.8 users might get one suggestion for update, and Tiger users another (maybe a later update, when we will have dropped 10.2.8-compatibility). Is that enough?

Comment 9

[Håkan Waara]

I'll have a go at this

Comment 10

[Simon Fraser [no longer active]]

(In reply to comment #8)
> So technically, I'd propose a .plist-file (as discussed on #camino, since we
> need to keep it simple and 10.2.8-compatible).  I propose the .plist include
> the following info:
> 
> * the latest version for a certain Mac OS X version
> * a URL for that update

* Date when that update was released

(so the UI can say "Camino 1.0 was released two days ago. do you want to download it?" or somesuch).

Comment 11

[Håkan Waara]

Attachment: Test.plist

So, this is a test.plist I generated with the property list editor.

The data will be structured like this:

<dict>
  <key>The required OS X version</key>
  <array>
    <string>URL for the webpage of the new Camino</string>
    <date>The date that version was released</date>
  <array>
</dict>

There's the latest Camino version info for every "minimum OS X version" key.
Comments?

Comment 12

[Håkan Waara]

I just noticed I forgot to add the Camino version element for each array.

So here's a revised structure:

<key>Mac OS X version</key>
<array>
  <real>Latest Camino version</real>
  <string>URL for that update</string>
  <date>Date of the release</date>
</array>

Comment 13

[Stuart Morgan]

>   <real>Latest Camino version</real>

Camino versions not infrequently contain letters (e.g., 1.0b2), so version is a string.

Comment 14

[Håkan Waara]

Should our auto-update feature suggest the user update even for betas?

If so, should we also include alphas? RCs? I'll need to take account of this into the comparision logic.

Comment 15

[Håkan Waara]

Stuart, see above question. :-)

Comment 16

[Håkan Waara]

On second thought -- I guess it's sufficient to check for equality. 

If the latest Camino for my system isn't the one I run right now, it's a newer one.

Comment 17

[Samuel Sidler (old account; do not CC)]

(In reply to comment #14)
> Should our auto-update feature suggest the user update even for betas?
> 
> If so, should we also include alphas? RCs? I'll need to take account of this
> into the comparision logic.
> 

Are update mechanism should suggest the latest stable version. In our current case, both 1.0b1 and 1.0b2 were stable releases so it would suggest them.

Comment 18

[Stuart Morgan]

> If the latest Camino for my system isn't the one I run right now, it's a newer
> one.

Unless someone is running a nightly.  1.0b2+ != 1.0b2, but 1.0b2 isn't newer.

Comment 19

[Håkan Waara]

Attachment: Patch for review

Here's a patch for review (lacking stuff like nib changes, etc so not for applying).

So basically it implements two prefs:
* The URL for the plist: app.update.url
* The URL to use if something fails: app.update.url.manual

So if we can't parse the plist, or there is some other error when checking for new updates, we simply redirect to the "manual" URL where the user can see if an update is available.

If an update is available on a OS version that is the same or less than the version you're running, a dialog pops up that says "An update is available since X days, would you like to download it?   [Cancel]  [[Update]]"

Otherwise it just reports that the current version is up to date.

Comment 20

[Håkan Waara]

BTW, please note the part "XXX: why doesn't this work?". 

I'd like advice on how to best type check there. That comment also has a typo in the closure "*/", but please ignore that since it's temporary.

Comment 21

[Stuart Morgan]

+      <array>
+        <string>Latest Camino version</string> (example: "1.0b2")
+        <string>URL for that release</string>
+        <date>Release date</date> (standard CFDate format, example: "2004-12-31T23:00:00Z")
+      </array>

This structure is semantically a dictionary; it really should be represented with one instead of using an array.

Comment 22

[Simon Fraser [no longer active]]

Comment on attachment 208240 [details] [diff] [review]
Patch for review

>Index: resources/application/all-camino.js
>===================================================================

>+pref("app.update.url", "http://www.konstochvanligasaker.se/test.plist");

Is this the final url, or just a test one?

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

>+//
>+// -checkForUpdates:
>+//
>+// Checks for updates by looking at the remote plist-file. For trunk builds, this
>+// checks that the trunk build isn't older than the latest stable release.
>+//
>+-(IBAction)checkForUpdates:(id)sender
>+{
>+  NSString *updateUrl = [[PreferenceManager sharedInstance] getStringPref:"app.update.url" withSuccess:NULL];

You should return if the pref is missing or empty.

>+  NSDictionary *rootDict = [NSDictionary dictionaryWithContentsOfURL:[NSURL URLWithString:updateUrl]];

This will do a synchronous fetch, which will hang the UI if the user is on a slow
or problematic network. This is not acceptable. You should use [NSURL loadResourceDataNotifyingClient:]
instead, or maybe even use necko.


>+  /* We parse a XML properties file which looks like this:
>+
>+    <dict>
>+      <key>Minimum OS requirement</key> (example: "10.2.8")
>+      <array>
>+        <string>Latest Camino version</string> (example: "1.0b2")
>+        <string>URL for that release</string>
>+        <date>Release date</date> (standard CFDate format, example: "2004-12-31T23:00:00Z")
>+      </array>
>+      ...
>+    </dict>
>+    
>+    There might be an arbitrary amount of keys in the root <dict>, for each time we bump
>+    the minimum requirements.
>+  */
>+  
>+  NSMutableArray *allKeys = [NSMutableArray arrayWithArray:[rootDict allKeys]];
>+  [allKeys sortUsingSelector:@selector(caseInsensitiveCompare:)];

Will this sort 10.0 before or after 9.9 (preparing for the future...)?

This use of system version as the key seems confusing and error prone.
Wouldn't it be better to have Camino call some CGI on a server somewhere, with
params for the user's OS version (and language), and have that script contain
the logic that sends back the url of the recommended download?

>+  
>+  // get the current system version and make it a string for comparison later
>+  NSString *myVersion = [NSString stringWithFormat:@"%x", [NSWorkspace systemVersion]];
>+
>+  // We iterate the keys from the end because we want the OS keys
>+  NSEnumerator *sortedOSKeys = [allKeys reverseObjectEnumerator];
>+  id curObj;
>+  
>+  while (curObj = [sortedOSKeys nextObject])
>+  {
>+    NSMutableString *reqVersion = [NSMutableString stringWithString:(NSString*)curObj];
>+    
>+    // Remove all dots from the OS version string, so "10.4.3" becomes "1043"
>+    [reqVersion replaceOccurrencesOfString:@"." withString:@"" 
>+                                   options:NSLiteralSearch 
>+                                     range:NSMakeRange(0, [reqVersion length])];
>+    
>+    if ([reqVersion isLessThanOrEqualTo:myVersion]) 

What is this -isLessThanOrEqualTo: method? 

>+    {
>+      NSArray *contents = [rootDict valueForKey:(NSString*)curObj];   
>+      
>+      if ([contents count] == 3) 
>+      {
>+        NSString *releaseVersion = [contents objectAtIndex:0];
>+        NSString *releaseURL = [contents objectAtIndex:1];
>+        NSDate *releaseDate = [contents objectAtIndex:2];

Why not use a dictionary?

>+        // XXX: why doesn't this work?

Because of class clusters. Even if you think something is an NSString, it might be
some other class. Rather than testing classes, you should ask whether the object
responds to the selectors that you want to send it.

Comment 23

[Samuel Sidler (old account; do not CC)]

(In reply to comment #22)
> (From update of attachment 208240 [details] [diff] [review] [edit])
> >Index: resources/application/all-camino.js
> >===================================================================
> 
> >+pref("app.update.url", "http://www.konstochvanligasaker.se/test.plist");
> 
> Is this the final url, or just a test one?
> 

No, you can use http://update.caminobrowser.org/test.plist for the time being.

Comment 24

[Håkan Waara]

Thanks for the review!

(In reply to comment #22)
> (From update of attachment 208240 [details] [diff] [review] [edit])
> >Index: resources/application/all-camino.js
> >===================================================================
> 
> >+pref("app.update.url", "http://www.konstochvanligasaker.se/test.plist");
> 
> Is this the final url, or just a test one?

Just my test URL. 

> 
> >Index: src/application/MainController.mm
> >===================================================================
> 
> >+//
> >+// -checkForUpdates:
> >+//
> >+// Checks for updates by looking at the remote plist-file. For trunk builds, this
> >+// checks that the trunk build isn't older than the latest stable release.
> >+//
> >+-(IBAction)checkForUpdates:(id)sender
> >+{
> >+  NSString *updateUrl = [[PreferenceManager sharedInstance] getStringPref:"app.update.url" withSuccess:NULL];
> 
> You should return if the pref is missing or empty.
> 
> >+  NSDictionary *rootDict = [NSDictionary dictionaryWithContentsOfURL:[NSURL URLWithString:updateUrl]];
> 
> This will do a synchronous fetch, which will hang the UI if the user is on a
> slow
> or problematic network. This is not acceptable. You should use [NSURL
> loadResourceDataNotifyingClient:]
> instead, or maybe even use necko.

I'll look into this.

> 
> 
> >+  /* We parse a XML properties file which looks like this:
> >+
> >+    <dict>
> >+      <key>Minimum OS requirement</key> (example: "10.2.8")
> >+      <array>
> >+        <string>Latest Camino version</string> (example: "1.0b2")
> >+        <string>URL for that release</string>
> >+        <date>Release date</date> (standard CFDate format, example: "2004-12-31T23:00:00Z")
> >+      </array>
> >+      ...
> >+    </dict>
> >+    
> >+    There might be an arbitrary amount of keys in the root <dict>, for each time we bump
> >+    the minimum requirements.
> >+  */
> >+  
> >+  NSMutableArray *allKeys = [NSMutableArray arrayWithArray:[rootDict allKeys]];
> >+  [allKeys sortUsingSelector:@selector(caseInsensitiveCompare:)];
> 
> Will this sort 10.0 before or after 9.9 (preparing for the future...)?
> 
> This use of system version as the key seems confusing and error prone.
> Wouldn't it be better to have Camino call some CGI on a server somewhere, with
> params for the user's OS version (and language), and have that script contain
> the logic that sends back the url of the recommended download?

I agree the current solution is kinda hackish and would also prefer some other way to do it.   

Any volunteers to write such a script? I've never done this, and also I'll need to investigate how to communicate with it from our code.

> 
> >+  
> >+  // get the current system version and make it a string for comparison later
> >+  NSString *myVersion = [NSString stringWithFormat:@"%x", [NSWorkspace systemVersion]];
> >+
> >+  // We iterate the keys from the end because we want the OS keys
> >+  NSEnumerator *sortedOSKeys = [allKeys reverseObjectEnumerator];
> >+  id curObj;
> >+  
> >+  while (curObj = [sortedOSKeys nextObject])
> >+  {
> >+    NSMutableString *reqVersion = [NSMutableString stringWithString:(NSString*)curObj];
> >+    
> >+    // Remove all dots from the OS version string, so "10.4.3" becomes "1043"
> >+    [reqVersion replaceOccurrencesOfString:@"." withString:@"" 
> >+                                   options:NSLiteralSearch 
> >+                                     range:NSMakeRange(0, [reqVersion length])];
> >+    
> >+    if ([reqVersion isLessThanOrEqualTo:myVersion]) 
> 
> What is this -isLessThanOrEqualTo: method? 

Yes, this definitely goes away if we use a script. 

> >+    {
> >+      NSArray *contents = [rootDict valueForKey:(NSString*)curObj];   
> >+      
> >+      if ([contents count] == 3) 
> >+      {
> >+        NSString *releaseVersion = [contents objectAtIndex:0];
> >+        NSString *releaseURL = [contents objectAtIndex:1];
> >+        NSDate *releaseDate = [contents objectAtIndex:2];
> 
> Why not use a dictionary?

Will do.

> 
> >+        // XXX: why doesn't this work?
> 
> Because of class clusters. Even if you think something is an NSString, it might
> be
> some other class. Rather than testing classes, you should ask whether the
> object
> responds to the selectors that you want to send it.
> 

Any general pointers on the CGI thing would be appreciated.

Comment 25

[Håkan Waara]

We were discussing this on IRC a few days ago, and there was some confusion about whether we should:

1. Display a dialog saying "There's a new update", etc - and the same for when there are no updates, or
2. Just redirect the user to a site when "Check for updates..." is chosen.

Personally I'd prefer #1 here, as many other mac apps do it. I need a decision here though before I can continue with the patch.

Comment 26

[Jasper]

I'd go for 1 aswell, but I have one question. You say there would be a "Cancel" and "Update" button, what would happen when you click on update? I'd say it would bring you to the website to allow you to download a new version.

I also wanted to know if there has been any talk yet on automatically looking for updates?

Comment 27

[Samuel Sidler (old account; do not CC)]

(In reply to comment #26)
> I'd go for 1 aswell, but I have one question. You say there would be a "Cancel"
> and "Update" button, what would happen when you click on update? I'd say it
> would bring you to the website to allow you to download a new version.

We have this all worked out via email. "Download" will take you to a /download/releases/1.0b2 page which downloads it (in like 3 seconds or whatever).

> I also wanted to know if there has been any talk yet on automatically looking
> for updates?
> 

Not for 1.0. That'll be done in the other bug.

Comment 28

[Håkan Waara]

Attachment: Example.plist

Here's a new example plist (which the script will return), based on all comments.

If there's no new version, an empty dictionary (no keys) will be returned.

Comment 29

[Wevah (Nate Weaver)]

Comment on attachment 208520 [details]
Example.plist

Changing charset.

Comment 30

[Wevah (Nate Weaver)]

> Changing charset.

Oops; content-type, too.

Comment 31

[Håkan Waara]

Attachment: Patch v2

Here's a revised patch, based on all feedback, which is much more elegant.

For testing I've used these two plists:
* http://www.konstochvanligasaker.se/example.plist
* http://www.konstochvanligasaker.se/empty-example.plist

also I've tried a number of cases including if it can't find the file etc. If there is some problem, we redirect to some standard URL defined in the prefs.

Comment 32

[Håkan Waara]

I filed bug 323473 for making the server side script to make this work.

If you know PHP/CGI scripting, you might be able to help out there.

Comment 33

[Håkan Waara]

Comment on attachment 208525 [details] [diff] [review]
Patch v2

Oops, wrong smorgan. Sorry. :)

Comment 34

[Simon Fraser [no longer active]]

Comment on attachment 208525 [details] [diff] [review]
Patch v2

Rather than adding complexity to MainController with the various url loading callbacks, I think we should make a small new class that handles this.

Comment 35

[Stuart Morgan]

Comment on attachment 208525 [details] [diff] [review]
Patch v2

Don't diff Localizable.strings or .nib files in patches.

+pref("app.update.url", "http://www.konstochvanligasaker.se/example.plist");
+pref("app.update.url.manual", "http://www.caminobrowser.org");

Why are these prefs? It adds complexity, and I'm not seeing the value for end-users to be able to this.

+// Checks for updates by looking at the remote plist-file. For trunk builds, this
+// checks that the trunk build isn't older than the latest stable release.

I think this should be more clear that logic is happening server-side to determine if there's a new build.

+  NSString *thisCaminoVersion = [NSString stringWithString:[[PreferenceManager sharedInstance] 
+              getStringPref:"general.useragent.vendorSub" withSuccess:&success]];

There's no need stringFromString the string; that just needlessly copies it.

+  NSString *thisSystemVersion = [NSString stringWithFormat:@"%x", [NSWorkspace systemVersion]];

Similarly, why convert to a string then put that in a URL instead of just putting the value directly into the URL?

+  NSURL *dataRequest = [NSURL URLWithString:[NSString stringWithFormat:@"%@?os_ver=%@?camino_ver=%@", rootURL, thisSystemVersion, thisCaminoVersion]];

|?| is the start of CGI params, not the separator. The second one needs to be |&|

+  NSLog ([dataRequest absoluteString]);

This looks like debugging, and shouldn't be in the patch

+    NSLog (releaseURL);
+    NSLog (releaseVersion);

Same here.

Comment 36

[Samuel Sidler (old account; do not CC)]

(In reply to comment #35)
> (From update of attachment 208525 [details] [diff] [review] [edit])
> +pref("app.update.url", "http://www.konstochvanligasaker.se/example.plist");
> +pref("app.update.url.manual", "http://www.caminobrowser.org");
> 
> Why are these prefs? It adds complexity, and I'm not seeing the value for
> end-users to be able to this.

We're using the prefs to allow the creators of optimized builds to change it, if they should want, and allow notifications to their builds.

In addition, this sets us up for the prefs that are used for the full-on update system Fx uses (and we're planning to use at some point). It's better to use those prefs now so we don't have to change things later.

Comment 37

[Steve Jacobs (irc: encro)]

It's probably worth pointing out the following update facility.

Sparkle:
http://www.andymatuschak.org/pages/sparkle

Open-Source, Free (MIT License)

From the website:
Sparkle is a module that developers can stick in their Cocoa applications (five-step install!) to get instant self-update functionality. By that, I mean that your app will be able to update itself, not just check for new versions: it値l read the update information from an appcast on your server, download, extract, install, restart, and even offer to show the users release notes before they decide if they want to update.

Comment 38

[Stuart Morgan]

That would be bug 185436, not this bug.

Comment 39

[Håkan Waara]

Attachment: Patch v3

Updated patch

Comment 40

[Nick Kreeger]

Comment on attachment 208791 [details] [diff] [review]
Patch v3

I don't think there is a need for two new files for this. If these files were for a new NIB Controller than I would agree that we need a new file. We could probably squeeze this small class into an existing file.

Comment 41

[Simon Fraser [no longer active]]

Comment on attachment 208791 [details] [diff] [review]
Patch v3

>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?
>+}

Comment 42

[Simon Fraser [no longer active]]

(In reply to comment #40)
> (From update of attachment 208791 [details] [diff] [review] [edit])
> I don't think there is a need for two new files for this. If these files were
> for a new NIB Controller than I would agree that we need a new file. We could
> probably squeeze this small class into an existing file.

I asked for the new files; I think it's worth it.

Of course, there are project changes that aren't shown in the patch.

Comment 43

[Håkan Waara]

(In reply to comment #41)
> >+- (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.

If I'm not mistaken, this should use the very buffer that has saved the data.
See http://developer.apple.com/documentation/Cocoa/Reference/Foundation/ObjC_classic/Classes/NSURL.html#//apple_ref/occ/instm/NSURL/resourceDataUsingCache:

Thanks for all comments (I learn a lot), new patch coming up soon.

Comment 44

[Håkan Waara]

Attachment: Patch v4

This patch addresses all your comments except:

* Still fetching the loaded data from the cache, as per the pointer.
* The dialog is modal, however I think that's pretty normal. Colloquy does it the same way. We show the dialog once we have all data loaded - so the user need only choose whether to update.

Also fixed a tiny bug I found in maincontroller where a variable was double-declared.

Comment 45

[Håkan Waara]

Oops, a line from my patch to bug 320660 slipped in. Please ignore. (That has r=stuart.morgan waiting on sr=, btw.)

Comment 46

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I'd recommend making this non-modal if at all possible.  It seems to me that this is an easy candidate for creating another category of "locked state" bug (bug 294129 tracks them all), for instance if a cookie sheet from a tab/windown comes up at the same time the data comes back from the server and throws the dialogue, or if a user invokes a menu or save dialogue when the data comes back, etc.

Comment 47

[Simon Fraser [no longer active]]

(In reply to comment #43)
> (In reply to comment #41)
> > >+- (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.
> 
> If I'm not mistaken, this should use the very buffer that has saved the data.

"If the receiver has already loaded its resource data through an invocation of loadResourceDataNotifyingClient:usingCache: and shouldUseCache is YES, returns the resource data. ... If the receiver has not already loaded its resource data, it will attempt to load it as a blocking operation"

Your original request is:
[dataRequest loadResourceDataNotifyingClient:self usingCache:NO];
so it never goes into the cache.

Comment 48

[Stuart Morgan]

A few other issues for the next round, since it already needs a re-spin:

+  NSString *rootURL = [[PreferenceManager sharedInstance] getStringPref:"app.update.url" withSuccess:NULL];

The fact that this is used whithout ever having been checked makes me nervous.  It should at least check that you get back a non-nil string.

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

This is changing an int to a string, not changing anything to an int.  I think the comment should just be removed, since |stringWithFormat:@"%x"| makes it very clear what is happening.

+  NSURL *dataRequest = [NSURL URLWithString:[NSString stringWithFormat:@"%@?os_ver=%@&camino_ver=%@", rootURL, thisSystemVersion, thisCaminoVersion]];
+  [dataRequest loadResourceDataNotifyingClient:self usingCache:NO];
+}

Part of this is user input, and a badly formed URL with give you back nil for |dataRequest|.  That needs checking too, to pass off to the manual backup (perhaps with a log statement?), or nothing will happen at all when the user tries to check for updates.

+  NSString *updatePage = [[PreferenceManager sharedInstance] getStringPref:"app.update.url.manual" withSuccess:NULL];

Again, what happens if this fails?

-- (NSWindow*)getFrontmostBrowserWindow;
+- (NSWindow* )getFrontmostBrowserWindow;

???

+  UpdateChecker *updater = [[[UpdateChecker alloc] init] autorelease];

It might be nice to make the standard convenience method in the class to do the alloc/init/autorelease part.


Lastly, a general question.  There's a pref for FF/TB called app.update.url.override.  a) Does anyone know why there needs to be another pref for this (is app.update.url not setable with user_pref?) and b) if it's part of the suite of core stuff being followed shouldn't it be respected here?

Comment 49

[Simon Fraser [no longer active]]

Comment on attachment 208909 [details] [diff] [review]
Patch v4

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

Test question: what keeps the UpdateChecker alive while it's waiting for the url load?

Comment 50

[Samuel Sidler (old account; do not CC)]

(In reply to comment #48)
> Lastly, a general question.  There's a pref for FF/TB called
> app.update.url.override.  a) Does anyone know why there needs to be another
> pref for this (is app.update.url not setable with user_pref?) and b) if it's
> part of the suite of core stuff being followed shouldn't it be respected here?

"app.update.url is always retrieved from the default pref branch, so it can't be modified by a user"

Comment 51

[Håkan Waara]

(In reply to comment #49)
> (From update of attachment 208909 [details] [diff] [review] [edit])
> >+-(IBAction)checkForUpdates:(id)sender
> >+{
> >+  UpdateChecker *updater = [[[UpdateChecker alloc] init] autorelease];
> >+  [updater checkForUpdate];
> >+}
> >+
> 
> Test question: what keeps the UpdateChecker alive while it's waiting for the
> url load?

NSURL, where we pass ourselves as delegate?

Comment 52

[Håkan Waara]

I've been looking at ADC for a good long while now, and also discussing in #camino. I need advice on the best way to do this dialog non-modal.

Seems like more or less the definition of a "dialog" is that it is in fact modal... so I should use NSWindow then I suppose? However that'd require arranging all the stuff manually (app icon, etc), no?

Comment 53

[Simon Fraser [no longer active]]

(In reply to comment #51)

> > Test question: what keeps the UpdateChecker alive while it's waiting for the
> > url load?
> 
> NSURL, where we pass ourselves as delegate?

Correct! But how many people reading the code would know that NSURL retains its client? Maybe UpdateChecker should retain itself while the url is running, just to be clearer.

Comment 54

[Stuart Morgan]

Comment on attachment 208909 [details] [diff] [review]
Patch v4

(r-'ing based on my comment 48 (including need for app.update.url.override support))

Comment 55

[Håkan Waara]

So consensus on #camino was that this should be an info panel similar to the bookmark info panel.

I only do this in my spare time so won't have the time to do it in hurry you might want (for 1.0) unless it's all I do during all of that time... so feel free to take it from my hands.

Comment 56

[Samuel Sidler (old account; do not CC)]

Too late for 1.0.

Comment 57

[Jasper]

We suck!

Comment 58

[Nick Kreeger]

Taking.

Comment 59

[Samuel Sidler (old account; do not CC)]

(In reply to comment #58)
> Taking.

Really? I was just going to WONTFIX this the other day but forgot. The "real" bug is bug 185436. This was just a temp bug for 1.0. A last minute effort. You should focus on bug 185436. (See comment 0)

Comment 60

[Samuel Sidler (old account; do not CC)]

Everyone say bye bye. The bug to follow is bug 185436.