About page should not automatically trigger OS updates

About page should not automatically trigger OS updates

Currently, if the user opens the Help/About page on Chrome OS it will
automatically check for an update and begin downloading/installing it.
Users should be able to decide when their update is applied, e.g., in
case they are connected to a tethered or bandwidth-limited network.

This disables the auto-update check on Chrome OS, replacing it with a
button the user can press to check for updates. (This has no effect on
the scheduled auto-updating service.)

Screenshots: http://imgur.com/a/k2s6y#0

BUG=394226
R=estade@chromium.org, deymo@chromium.org

Committed: https://crrev.com/5832e1e7030ffe2d41ee563be334eee58d8eb046
Cr-Commit-Position: refs/heads/master@{#291580}

[michaelpg, 2014-08-14 22:52:57]

https://codereview.chromium.org/476983002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right):

https://codereview.chromium.org/476983002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:137: if
(!update_engine_client->HasObserver(this))
Note that this is an existing "bug" (a NOT_REACHED should be thrown if you
changed the channel and then this function is triggered, since the observer
would be added twice). It isn't really detectable though because it would occur
on release builds.

But now that this function can be called multiple times, it's definitely
necessary.

[Evan Stade, 2014-08-18 18:43:04]

lgtm

https://codereview.chromium.org/476983002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/help/version_updater_chromeos.h (right):

https://codereview.chromium.org/476983002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:27: protected:
one space

[michaelpg, 2014-08-18 21:53:21]

deymo, could you take a look? Thanks!

https://codereview.chromium.org/476983002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/help/version_updater_chromeos.h (right):

https://codereview.chromium.org/476983002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:27: protected:
On 2014/08/18 18:43:04, Evan Stade wrote:
> one space

Done.

[deymo, 2014-08-19 05:09:12]

lgtm in general. I left one comment. Thanks for implementing this.

Did you test that the OOBE workflow still works?

https://codereview.chromium.org/476983002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/476983002/diff/40001/chrome/browser/resources...
chrome/browser/resources/help/help_page.js:303: // Hide the status container if
we haven't checked for updates yet.
This comment isn't quite accurate. We want to show the status of the current
update even if we didn't initiate it. Example:

Check for update, update is in progress (i.e. 20% done), refresh page --> update
should show progress.

Also, if you open this page to see if there's an update in progress you should
see that. It looks like the code is right but the comment is not.

[michaelpg, 2014-08-19 05:20:02]

How does this affect OOBE? I know OOBE checks for updates but I don't see that
it uses any of these files.

https://codereview.chromium.org/476983002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/help/help_page.js (right):

https://codereview.chromium.org/476983002/diff/40001/chrome/browser/resources...
chrome/browser/resources/help/help_page.js:303: // Hide the status container if
we haven't checked for updates yet.
On 2014/08/19 05:09:12, deymo wrote:
> This comment isn't quite accurate. We want to show the status of the current
> update even if we didn't initiate it. Example:
> 
> Check for update, update is in progress (i.e. 20% done), refresh page -->
update
> should show progress.
> 
> Also, if you open this page to see if there's an update in progress you should
> see that. It looks like the code is right but the comment is not.

I agree -- this is the desired behavior and what the code does. I'll update the
comment.

[deymo, 2014-08-19 05:30:23]

I'm not sure, but isn't the OOBE also calling the same CheckForUpdate() method
that you refactored in two? or that is somewhere else? (I'm not familiar with
that code :)

[michaelpg, 2014-08-19 06:42:45]

No, it looks like OOBE just uses the update engine dbus client (like
VersionUpdater does):
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

If we grep for "version_updater.h" we see that it's only being used in the
Help WebUI. So there shouldn't be any effect on OOBE from this CL.

Thanks for bringing it up, though. I never see that update screen since I'm
always using my own builds so I didn't even think about it.


On Mon, Aug 18, 2014 at 10:30 PM, <deymo@chromium.org> wrote:

> I'm not sure, but isn't the OOBE also calling the same CheckForUpdate()
> method
> that you refactored in two? or that is somewhere else? (I'm not familiar
> with
> that code :)
>
> https://codereview.chromium.org/476983002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[michaelpg, 2014-08-22 22:52:58]

Thanks.

[michaelpg, 2014-08-22 22:53:03]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2014-08-22 22:54:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaelpg@chromium.org/476983002/80001

[commit-bot: I haz the power, 2014-08-23 21:19:35]

Committed patchset #3 (80001) as 248e7742df60713716e64d70d1065f971c750a45

[commit-bot: I haz the power, 2014-09-10 02:30:54]

Patchset 3 (id:??) landed as
https://crrev.com/5832e1e7030ffe2d41ee563be334eee58d8eb046
Cr-Commit-Position: refs/heads/master@{#291580}