About box auto-update improvements

About box auto-update improvements.

The About box now knows how to check to see if updates have been installed
in the background without anyone having to click the Update button in the box.

The About box no longer gets stuck in the "installed" state.  Even if an
update has been installed, the About box will still check for new updates when
reopened.

BUG=13165, 20488
TEST=Play with the about box and auto-update a whole lot

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=30078

[TVL, 2009-10-26 03:35:39]

i realize you aren't looking for review yet, but one thin that caught my eye.

Also, do you want to grab http://crbug.com/25582 while you are working on the
about box?

http://codereview.chromium.org/338012/diff/6001/7003
File base/worker_pool_mac.mm (right):

http://codereview.chromium.org/338012/diff/6001/7003#newcode87
Line 87: NSOperationQueue* operation_queue = [WorkerPoolObjC
sharedOperationQueue];
didn't realize we were using this, do we care about the bugs before 10.5.7?  -- 

  http://www.mikeash.com/?page=pyblog/dont-use-nsoperationqueue.html
  http://www.mikeash.com/?page=pyblog/use-nsoperationqueue.html

[Mark Mentovai, 2009-10-26 05:01:29]

This is ready for review now.  I'll probably go through and revise or add some
comments tomorrow, but I think I'm done playing with the code now.

NSOperationQueue:

I don't know if we care, most of the 10.5 world is on 10.5.8 now, and this was
fixed in 10.5.7...

We've been using NSOperationQueue for over a year already and nobody's tried to
pin anything on it.  I doubt that adding one more not-hit-often case would
really make things any worse.  The links you gave make it sound like it's only a
problem when you have more than one NSOperationQueue in a process.  Certainly
it's not impossible, but as you've seen, we take care to use only a single one.

[TVL, 2009-10-26 14:23:52]

http://codereview.chromium.org/338012/diff/7010/6007
File chrome/app/keystone_glue.mm (right):

http://codereview.chromium.org/338012/diff/7010/6007#newcode281
Line 281: NSNotificationCenter* center = [NSNotificationCenter defaultCenter];
why unregister for this?

http://codereview.chromium.org/338012/diff/7010/6007#newcode373
Line 373: count:1];
autorelease or dictionaryWithObject:forKey:?

http://codereview.chromium.org/338012/diff/7010/6011
File chrome/app/keystone_glue_unittest.mm (right):

http://codereview.chromium.org/338012/diff/7010/6011#newcode25
Line 25: count:1];
dictionaryWithObject:forKey:?

http://codereview.chromium.org/338012/diff/7010/6009
File chrome/browser/cocoa/about_window_controller.mm (right):

http://codereview.chromium.org/338012/diff/7010/6009#newcode102
Line 102: [bundle objectForInfoDictionaryKey:@"CFBundleShortVersionString"];
skip this and just kChromeVersion since you added it?

http://codereview.chromium.org/338012/diff/7010/6009#newcode148
Line 148: if (recentStatus == kAutoupdateInstallFailed) {
do you need to be able to detect if an existing update check or install is
happening and jump directly to that UI state?

http://codereview.chromium.org/338012/diff/7010/6009#newcode276
Line 276: // instead of springing this sheet or dialog.
fyi - windows uses the dialog, so we already have a string for that context.  it
also might be usable for a notification when the about window has been closed
while the update is installing.

[Mark Mentovai, 2009-10-26 17:02:55]

New version available.

http://codereview.chromium.org/338012/diff/7010/6009
File chrome/browser/cocoa/about_window_controller.mm (right):

http://codereview.chromium.org/338012/diff/7010/6009#newcode102
Line 102: [bundle objectForInfoDictionaryKey:@"CFBundleShortVersionString"];
TVL wrote:
> skip this and just kChromeVersion since you added it?

I had that initially but I put it back this way because I've got to get the
SVNRevision too (for non-Google Chrome builds).  This dictionary should be
cached so it shouldn't be a big deal, and it's the framework's dictionary so
it's guaranteed to match what's running.

I figure that in a weird world where we want to localize version numbers (maybe
they want to use those weird dots in Asian languages?) this will do the right
thing.

http://codereview.chromium.org/338012/diff/7010/6009#newcode148
Line 148: if (recentStatus == kAutoupdateInstallFailed) {
TVL wrote:
> do you need to be able to detect if an existing update check or install is
> happening and jump directly to that UI state?

I thought about this on the way into work today.  I'll do a follow-up this
afternoon to add states for "checking" and "installing" to KeystoneGlue.  Some
of the logic around a couple of these operations might change and I want to keep
it isolated, so I'll do it as a follow-up.  The changes here are no worse than
what we had before.  In fact, they're better in most respects because we're not
restricting ourselves in how we handle the callbacks.  The notification scheme
is so much better than "call back whoever called into you last."

http://codereview.chromium.org/338012/diff/7010/6009#newcode276
Line 276: // instead of springing this sheet or dialog.
TVL wrote:
> fyi - windows uses the dialog, so we already have a string for that context.

Yeah, I know.  I'm thinking of this as a post-beta task.  Dialogs really suck.

> it
> also might be usable for a notification when the about window has been closed
> while the update is installing.

Right.  Well, again, dialogs suck, but I agree that it would be nice to have a
way to do something UI-side when an update has been installed in the background.
 This is what I had in mind with .keystone_install signaling running processes. 
It might be nice if we could do an info bar instead of a dialog if there's a
window open, but I'm not sure if we're supposed to use info bars for things that
aren't related to the tab/page being displayed.

I'm just thinking that I absolutely hate when I'm using a program and then, out
of the blue, without any action from me, a dialog pops up and says "wut u
wearin?"  It usually happens while I'm typing.  That's the worst time to engage
in pillow talk with a dialog box.

[TVL, 2009-10-26 17:08:00]

On 2009/10/26 17:02:55, Mark Mentovai wrote:
> New version available.
> 
> http://codereview.chromium.org/338012/diff/7010/6009
> File chrome/browser/cocoa/about_window_controller.mm (right):
> 
> http://codereview.chromium.org/338012/diff/7010/6009#newcode102
> Line 102: [bundle objectForInfoDictionaryKey:@"CFBundleShortVersionString"];
> TVL wrote:
> > skip this and just kChromeVersion since you added it?
> 
> I had that initially but I put it back this way because I've got to get the
> SVNRevision too (for non-Google Chrome builds).  This dictionary should be
> cached so it shouldn't be a big deal, and it's the framework's dictionary so
> it's guaranteed to match what's running.
> 
> I figure that in a weird world where we want to localize version numbers
(maybe
> they want to use those weird dots in Asian languages?) this will do the right
> thing.
> 
> http://codereview.chromium.org/338012/diff/7010/6009#newcode148
> Line 148: if (recentStatus == kAutoupdateInstallFailed) {
> TVL wrote:
> > do you need to be able to detect if an existing update check or install is
> > happening and jump directly to that UI state?
> 
> I thought about this on the way into work today.  I'll do a follow-up this
> afternoon to add states for "checking" and "installing" to KeystoneGlue.  Some
> of the logic around a couple of these operations might change and I want to
keep
> it isolated, so I'll do it as a follow-up.  The changes here are no worse than
> what we had before.  In fact, they're better in most respects because we're
not
> restricting ourselves in how we handle the callbacks.  The notification scheme
> is so much better than "call back whoever called into you last."


yup

> 
> http://codereview.chromium.org/338012/diff/7010/6009#newcode276
> Line 276: // instead of springing this sheet or dialog.
> TVL wrote:
> > fyi - windows uses the dialog, so we already have a string for that context.
> 
> Yeah, I know.  I'm thinking of this as a post-beta task.  Dialogs really suck.
> 
> > it
> > also might be usable for a notification when the about window has been
closed
> > while the update is installing.
> 
> Right.  Well, again, dialogs suck, but I agree that it would be nice to have a
> way to do something UI-side when an update has been installed in the
background.
>  This is what I had in mind with .keystone_install signaling running
processes. 
> It might be nice if we could do an info bar instead of a dialog if there's a
> window open, but I'm not sure if we're supposed to use info bars for things
that
> aren't related to the tab/page being displayed.
> 
> I'm just thinking that I absolutely hate when I'm using a program and then,
out
> of the blue, without any action from me, a dialog pops up and says "wut u
> wearin?"  It usually happens while I'm typing.  That's the worst time to
engage
> in pillow talk with a dialog box.

We use infobars for default browser, so that's not related to content.  The down
side would be if no windows are opened or >1, what do we do?  anyway, that's
follow up.  was just providing background since i was the one that switched away
from a new button in there.

[TVL, 2009-10-26 17:09:59]

lgtm

[Mark Mentovai, 2009-10-26 17:58:34]

+andybons

[Andrew Bonventre, 2009-10-26 19:12:18]

lgtm (just along for the ride to get familiar with other portions of the code)

[Mark Mentovai, 2009-10-26 19:18:44]

Final version up.  I just changed the keystone_glue* files to keep the KSG unit
test from screwing up the state for the about window controller unit test.

[TVL, 2009-10-26 19:23:42]

slgtm