Implement upgrade notifications....

Implement upgrade notifications.

When we detect that the installed version is newer than the version you are
running we show a little throbbing orange dot over the wrench menu.

If you open the wrench menu and close it again, the throbbing will stop.

However, if you look at the contents of the wrench menu you'll notice that
the About box menu item has been removed and in its place is a menu item
"Update Chrome Now" with a bright orange icon to draw your attention to it.

Clicking on the icon shows a dialog box asking whether you want to restart
Chrome. If you do, the browser restarts with your session restored 
(even if you have Session Restore turned off).

Known issues:

- Currently this is Windows only. We'll have to port this to Linux and do
something differnet for Mac (which doesn't have the wrench menu).

- Showing an icon in front of Update Chrome causes the checkbox for the
bookmark bar menu to go away. Given that we will soon redesign the menus I'm
not going to spend much time trying to fix it.

BUG=27941
TEST=Wait for Chrome to be upgraded in the background, an orange dot should
appear over the wrench menu and if you select Update Chrome your session should
be retained.


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

[Finnur, 2010-05-25 23:32:04]

Looks a bit scarier than it is, since there is a bit of plumbing involved...

[Ben Goodger (Google), 2010-05-26 00:06:33]

http://codereview.chromium.org/2225003/diff/1/4
File chrome/browser/app_menu_model.cc (right):

http://codereview.chromium.org/2225003/diff/1/4#newcode191
chrome/browser/app_menu_model.cc:191: } else {
nit: no else after return

http://codereview.chromium.org/2225003/diff/1/15
File chrome/browser/upgrade_detector.cc (right):

http://codereview.chromium.org/2225003/diff/1/15#newcode30
chrome/browser/upgrade_detector.cc:30: static int kNotifyUserAfterMs = 0;
What's your reason for having this delay? Is there a reason you don't just
notify immediately?

http://codereview.chromium.org/2225003/diff/1/19
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/2225003/diff/1/19#newcode817
chrome/browser/views/toolbar_view.cc:817:
SkBitmapOperations::CreateBlendedBitmap(
How expensive is this operation? How many frames are there?

[Finnur, 2010-05-26 00:27:52]

Updated.

http://codereview.chromium.org/2225003/diff/1/15
File chrome/browser/upgrade_detector.cc (right):

http://codereview.chromium.org/2225003/diff/1/15#newcode30
chrome/browser/upgrade_detector.cc:30: static int kNotifyUserAfterMs = 0;
See comment above the statics (this particular value is meant to be set to 2
weeks for the stable channel).

http://codereview.chromium.org/2225003/diff/1/19
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/2225003/diff/1/19#newcode817
chrome/browser/views/toolbar_view.cc:817:
SkBitmapOperations::CreateBlendedBitmap(
I see about a 100 calls to the GetAppIcon function during one animation
(pulsating/fadein). There is some fixed cost up-front (not sure how much) for
calling the function and then it depends on the dimensions of the images (which
are pretty small: 7x7 pixels). I haven't measured the time it takes each time to
call it. Want me to?

[John Grabowski, 2010-05-26 01:15:11]

>
>
>
> - Currently this is Windows only. We'll have to port this to Linux and do
> something differnet for Mac (which doesn't have the wrench menu).
>

Mac has Wrench menu if you check Chrome...Preferences...Basics..."Show Page
and Tools menus".  But, in general, people don't enable them since all their
features are covered in the main menubar.

Strongly recommend you discuss Mac notification issues with Cole.

jrg

[Ben Goodger (Google), 2010-05-26 01:27:25]

We're planning to turn on a unified menu on all platforms (see Cole
for mocks) for M6 so I think this still works.

-Ben

On Tue, May 25, 2010 at 6:15 PM, John Grabowski <jrg@chromium.org> wrote:
>>
>>
>> - Currently this is Windows only. We'll have to port this to Linux and do
>> something differnet for Mac (which doesn't have the wrench menu).
>
> Mac has Wrench menu if you check Chrome...Preferences...Basics..."Show Page
> and Tools menus".  But, in general, people don't enable them since all their
> features are covered in the main menubar.
> Strongly recommend you discuss Mac notification issues with Cole.
> jrg
>
>

[Finnur, 2010-05-26 04:06:42]

> We're planning to turn on a unified menu on all platforms (see Cole
> for mocks) for M6 so I think this still works.

Plus, I asked Brian earlier and we've already decided to do this for Windows,
even if it means finding a different way of doing this for the Mac.

[John Grabowski, 2010-05-26 04:13:13]

Not suggesting you change paths.
Was trying to help direct your 'We'll have to ... do something different for
Mac' comment.

jrg

On Tue, May 25, 2010 at 9:06 PM, <finnur@chromium.org> wrote:

> We're planning to turn on a unified menu on all platforms (see Cole
>> for mocks) for M6 so I think this still works.
>>
>
> Plus, I asked Brian earlier and we've already decided to do this for
> Windows,
> even if it means finding a different way of doing this for the Mac.
>
>
> http://codereview.chromium.org/2225003/show
>

[Finnur, 2010-05-26 16:29:46]

> How expensive is this operation?

I used the high-res timer of our built-in base::Time class to time this in my
debug build. I see that it takes on average 23 microseconds to execute
CreateBlendedBitmap once with my two 7x7 images.

Does this sound (and the changelist as a whole) look good?

[Ben Goodger (Google), 2010-05-26 18:17:31]

I see. The reason I asked about the cost of the image operations was that in
theme installation (admittedly a much more complex task) we do many image
operations and the effect is slow.

There are ways to improve the performance of this if it ever causes a negative
effect, but let's worry about that if one becomes apparent.

LGTM.

[Nico, 2010-08-27 05:14:09]

http://codereview.chromium.org/2225003/diff/41001/42008
File chrome/browser/browser_shutdown.cc (right):

http://codereview.chromium.org/2225003/diff/41001/42008#newcode172
chrome/browser/browser_shutdown.cc:172:
Upgrade::RelaunchChromeBrowser(command_line);
Isn't this very racy? RelaunchChromeBrowser() mostly just calls
base::LaunchApp() directly, which means we're basically hope that the current
process finishes fast enough before the new process starts up.

[Nico, 2010-08-27 05:17:15]

http://codereview.chromium.org/2225003/diff/41001/42008
File chrome/browser/browser_shutdown.cc (right):

http://codereview.chromium.org/2225003/diff/41001/42008#newcode172
chrome/browser/browser_shutdown.cc:172:
Upgrade::RelaunchChromeBrowser(command_line);
On 2010/08/27 05:14:09, Nico wrote:
> Isn't this very racy? RelaunchChromeBrowser() mostly just calls
> base::LaunchApp() directly, which means we're basically hope that the current
> process finishes fast enough before the new process starts up.

This might be the cause for http://crbug.com/50803 and possibly
http://crbug.com/49631