Help: Implement version updating on Win.

Help: Implement version updating on Win.

BUG=115123
TEST=none
R=csilv

[csilv, 2012-03-15 05:23:42]

lgtm

[James Hawkins, 2012-03-15 16:31:36]

+finnur,gpu

[Finnur, 2012-03-16 12:03:22]

Please consider fixing http://crbug.com/2462 while you are at it (see
GoogleUpdate::CheckForUpdate). I've been postponing it until a major upgrade of
the About box update mechanism (because then we'd be guaranteed to do a full
scale QA test on it end-to-end), but that never happened because we've not had a
reason (until now) to change the update mechanism as it has been working fine
for us so far.

http://codereview.chromium.org/9700049/diff/1/chrome/browser/ui/webui/help/ve...
File chrome/browser/ui/webui/help/version_updater_win.h (right):

http://codereview.chromium.org/9700049/diff/1/chrome/browser/ui/webui/help/ve...
chrome/browser/ui/webui/help/version_updater_win.h:42:
scoped_refptr<GoogleUpdate> google_updater_;
I forget... if the name of the file includes _win.cc/h does that not compile on
Aura? I ask because AboutChromeView #defined this only if !USE_AURA.

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:55: NULL);  // No window
widget.
How does setting a NULL window widget work when update needs to show the UAC
prompts with Vista/Windows?

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:73: const string16&
error_message) {
Where's the check to make sure we don't try to upgrade chromium builds?

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:96: NULL);  // No window
widget.
Is the page that shows this a singleton or can the user get frustrated with how
long it takes to upgrade and open a new page and start the upgrade process
again?

(Also: window widget == NULL)

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:104: // staged then
UPGRADE_SUCESSFUL will be returned.
nit: Separate these two clauses with linebreak and move lines 100-104 below 107.

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:133:
content::RecordAction(UserMetricsAction("UpgradeCheck_Upgraded"));
You need braces (multiline).

Alternatively (since this is so much simplified), you can just do:

content::RecordAction(UserMetricsAction("UpgradeCheck_AlreadyUpgraded"));
status = NEARLY_UPDATED;
break;

... on line 126 and leave just the else clause plus what is below this line.

(Unless you think the UPGRADE_SUCCESSFUL logic will grow in the future?)

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.h (right):

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.h:42:
scoped_refptr<GoogleUpdate> google_updater_;
I forget... if the name of the file includes _win.cc/h does that
not compile on Aura? I ask because AboutChromeView #defined this
only if !USE_AURA.

[Finnur, 2012-03-16 12:05:11]

And when I refer to bug http://crbug.com/2462 I'm only talking about the Updater
(ignore the parts about unspecified 'other things' that need to move off the
FILE thread). :)

[cpu_(ooo_6.6-7.5), 2012-03-19 00:25:06]

did you mean gpu or cpu?  :)

I am not familiar with this code. Don't even know what is the feature we are
trying to implement.

[James Hawkins, 2012-03-26 21:55:40]

-cpu

gpu not here anymore?

[James Hawkins, 2012-03-26 23:18:14]

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:55: NULL);  // No window
widget.
On 2012/03/16 12:03:22, Finnur wrote:
> How does setting a NULL window widget work when update needs to show the UAC
> prompts with Vista/Windows?

I didn't quite understand the requirement of the widget.  If this object is
eventually the only code that calls the updater, it seems like we can remove
this parameter from the API and have CheckForUpdate() handle the code for
acquiring the top-level browser widget.

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:73: const string16&
error_message) {
On 2012/03/16 12:03:22, Finnur wrote:
> Where's the check to make sure we don't try to upgrade chromium builds?

Done.

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:96: NULL);  // No window
widget.
On 2012/03/16 12:03:22, Finnur wrote:
> Is the page that shows this a singleton or can the user get frustrated with
how
> long it takes to upgrade and open a new page and start the upgrade process
> again?
> 
> (Also: window widget == NULL)

chrome:chrome (and thus the help page) is a singleton.

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:104: // staged then
UPGRADE_SUCESSFUL will be returned.
On 2012/03/16 12:03:22, Finnur wrote:
> nit: Separate these two clauses with linebreak and move lines 100-104 below
107.

Done.

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:133:
content::RecordAction(UserMetricsAction("UpgradeCheck_Upgraded"));
On 2012/03/16 12:03:22, Finnur wrote:
> You need braces (multiline).
> 
> Alternatively (since this is so much simplified), you can just do:
> 
> content::RecordAction(UserMetricsAction("UpgradeCheck_AlreadyUpgraded"));
> status = NEARLY_UPDATED;
> break;
> 
> ... on line 126 and leave just the else clause plus what is below this line.
> 
> (Unless you think the UPGRADE_SUCCESSFUL logic will grow in the future?)

Done.

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.h (right):

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.h:42:
scoped_refptr<GoogleUpdate> google_updater_;
On 2012/03/16 12:03:22, Finnur wrote:
> I forget... if the name of the file includes _win.cc/h does that
> not compile on Aura? I ask because AboutChromeView #defined this
> only if !USE_AURA.

Correct, this is windows only (since it ends in _win).

[Finnur, 2012-03-27 10:44:25]

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:55: NULL);  // No window
widget.
InitiateGoogleUpdateCheck needs the widget for elevation. It seems simpler to me
to just call GetWidget() here (like the old code does), as opposed to having
CheckForUpdate figure out which widget to use. #shrug#

http://codereview.chromium.org/9700049/diff/10001/chrome/browser/ui/webui/hel...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

http://codereview.chromium.org/9700049/diff/10001/chrome/browser/ui/webui/hel...
chrome/browser/ui/webui/help/version_updater_win.cc:79: #endif
This comment is (and was) misleading. We show an error for Chromium builds, last
time I checked. It is CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY. Is that still the
case with the new code?

http://codereview.chromium.org/9700049/diff/10001/chrome/browser/ui/webui/hel...
chrome/browser/ui/webui/help/version_updater_win.cc:136: status =
NEARLY_UPDATED;
Umm... aren't you missing a break here now to avoid getting the UMA signal
"UpgradeCheck_Upgraded"?

[James Hawkins, 2012-03-28 20:52:32]

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:55: NULL);  // No window
widget.
On 2012/03/27 10:44:25, Finnur wrote:
> InitiateGoogleUpdateCheck needs the widget for elevation. It seems simpler to
me
> to just call GetWidget() here (like the old code does), as opposed to having
> CheckForUpdate figure out which widget to use. #shrug#

We don't have a GetWidget() for WebUI.  So you're saying we definitely need to
have access to the widget of the browser window containing the WebUI page?

http://codereview.chromium.org/9700049/diff/10001/chrome/browser/ui/webui/hel...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

http://codereview.chromium.org/9700049/diff/10001/chrome/browser/ui/webui/hel...
chrome/browser/ui/webui/help/version_updater_win.cc:79: #endif
On 2012/03/27 10:44:25, Finnur wrote:
> This comment is (and was) misleading. We show an error for Chromium builds,
last
> time I checked. It is CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY. Is that still
the
> case with the new code?

That's not currently the case, though we can make that the case.

http://codereview.chromium.org/9700049/diff/10001/chrome/browser/ui/webui/hel...
chrome/browser/ui/webui/help/version_updater_win.cc:136: status =
NEARLY_UPDATED;
On 2012/03/27 10:44:25, Finnur wrote:
> Umm... aren't you missing a break here now to avoid getting the UMA signal
> "UpgradeCheck_Upgraded"?

Done.

[Finnur, 2012-03-28 22:09:12]

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:55: NULL);  // No window
widget.
Oh, I see. In that case it doesn't matter, I guess, at what point we obtain the
widget -- whether we do it here or inside CheckForUpdate, as you suggested
(which makes sense now).

I'm not sure it matters which Chrome window we obtain to get the widget as long
as we get a valid HWND from it, but Ananta would know (I think he added this
code).

I do believe, though, that elevation on Vista won't work without *a* widget.

On 2012/03/28 20:52:32, James Hawkins wrote:
> On 2012/03/27 10:44:25, Finnur wrote:
> > InitiateGoogleUpdateCheck needs the widget for elevation. It seems simpler
to
> me
> > to just call GetWidget() here (like the old code does), as opposed to having
> > CheckForUpdate figure out which widget to use. #shrug#
> 
> We don't have a GetWidget() for WebUI.  So you're saying we definitely need to
> have access to the widget of the browser window containing the WebUI page?

http://codereview.chromium.org/9700049/diff/10001/chrome/browser/ui/webui/hel...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

http://codereview.chromium.org/9700049/diff/10001/chrome/browser/ui/webui/hel...
chrome/browser/ui/webui/help/version_updater_win.cc:79: #endif
It is good to have an error to look up, as opposed to silently failing. I
believe we may even have help center articles on the subject...

On 2012/03/28 20:52:32, James Hawkins wrote:
> On 2012/03/27 10:44:25, Finnur wrote:
> > This comment is (and was) misleading. We show an error for Chromium builds,
> last
> > time I checked. It is CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY. Is that still
> the
> > case with the new code?
> 
> That's not currently the case, though we can make that the case.

[James Hawkins, 2012-03-30 20:34:53]

+ananta

[ananta, 2012-03-30 20:52:53]

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
File chrome/browser/ui/webui/help/version_updater_win.cc (right):

http://codereview.chromium.org/9700049/diff/7001/chrome/browser/ui/webui/help...
chrome/browser/ui/webui/help/version_updater_win.cc:55: NULL);  // No window
widget.
On 2012/03/28 22:09:12, Finnur wrote:
> Oh, I see. In that case it doesn't matter, I guess, at what point we obtain
the
> widget -- whether we do it here or inside CheckForUpdate, as you suggested
> (which makes sense now).
> 
> I'm not sure it matters which Chrome window we obtain to get the widget as
long
> as we get a valid HWND from it, but Ananta would know (I think he added this
> code).
> 
> I do believe, though, that elevation on Vista won't work without *a* widget.
> 
> On 2012/03/28 20:52:32, James Hawkins wrote:
> > On 2012/03/27 10:44:25, Finnur wrote:
> > > InitiateGoogleUpdateCheck needs the widget for elevation. It seems simpler
> to
> > me
> > > to just call GetWidget() here (like the old code does), as opposed to
having
> > > CheckForUpdate figure out which widget to use. #shrug#
> > 
> > We don't have a GetWidget() for WebUI.  So you're saying we definitely need
to
> > have access to the widget of the browser window containing the WebUI page?
> 

I don't remember adding this code :(. However Finnur's comment about using any
available HWND does make sense. It would be great if we could test this to be
sure.

[finnur1, 2012-05-03 10:18:08]

James, did you consider my request to look into http://crbug.com/2462 (ignoring
the 'possibly other stuff' section).

I think this is the perfect opportunity since we are going to be doing a great
deal of testing of auto-update...

[James Hawkins, 2012-05-03 16:44:22]

On 2012/05/03 10:18:08, finnur1 wrote:
> James, did you consider my request to look into http://crbug.com/2462
(ignoring
> the 'possibly other stuff' section).
> 
> I think this is the perfect opportunity since we are going to be doing a great
> deal of testing of auto-update...

I would but I barely have time to finish this CL :-/  I'll consider it.

[Finnur, 2012-05-03 17:44:42]

So, end-to-end testing of auto-update has not occurred yet, correct?
If so, I might take a stab at it. I suspect it isn't too much work...

[James Hawkins, 2012-05-03 17:45:38]

On 2012/05/03 17:44:42, Finnur wrote:
> So, end-to-end testing of auto-update has not occurred yet, correct?
> If so, I might take a stab at it. I suspect it isn't too much work...

Correct.  I don't currently have access to a windows machine for testing :-/

[James Hawkins, 2012-07-18 15:50:49]

On 2012/05/03 17:45:38, James Hawkins wrote:
> On 2012/05/03 17:44:42, Finnur wrote:
> > So, end-to-end testing of auto-update has not occurred yet, correct?
> > If so, I might take a stab at it. I suspect it isn't too much work...
> 
> Correct.  I don't currently have access to a windows machine for testing :-/

Deleting this CL since MAD has landed this functionality.