Fixed channel switch when user decides to switch to another channel in

Fixed channel switch when user decides to switch to another channel in
he middle of current channel transition.

For example:
1. User is currently on the beta channel and decides to switch to the dev channel.
2. In the middle of the switch the user decides to switch to the stable channel
   instead of currently downloading dev.

But update engine moves to UPDATE_STATUS_REPORTING_ERROR_EVENT because
target channel isn't equal to downloading channel, so request to start
downloading of stable channel fails. Almost immediately update engine moves
to UPDATE_STATUS_IDLE, but user needs to reload chrome://help page to initiate
update check. Current changelist handles the issue and implements automatic
update checks when update engine is idle.

BUG=311162
TEST=unit_tests:VersionUpdaterCrosTest.*

Committed: https://crrev.com/84a24d2929796a717aef73ec7d85a87e09cd251d
Cr-Commit-Position: refs/heads/master@{#296693}

[ygorshenin1, 2014-09-18 14:09:18]

ygorshenin@chromium.org changed reviewers:
+ jhawkins@chromium.org, satorux@chromium.org

[ygorshenin1, 2014-09-18 14:09:18]

+ jhawkins@ for chrome/*
+ satorux@ for chromeos/*

[James Hawkins, 2014-09-18 15:48:22]

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

https://codereview.chromium.org/578293002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:61: bool
can_update_for_testing_;
Why is this necessary?

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

https://codereview.chromium.org/578293002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos_unittest.cc:74:
ASSERT_EQ(0, fake_update_engine_client_->request_update_check_call_count());
It doesn't look like any of these tests need to be ASSERT_ (as opposed to
EXPECT_).

[ygorshenin1, 2014-09-18 15:59:07]

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

https://codereview.chromium.org/578293002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:61: bool
can_update_for_testing_;
To simplify testing and omit unnecessary checks in unit tests. For instance,
before call to UpdateEngineClient::RequestUpdateCheck() VersionUpdaterCros
checks that the device is registered and we're currently not on the OOBE screen,
etc. Of course, instead of boolean flag we can check that command line has
kTestType switch. What do you think?

On 2014/09/18 15:48:22, James Hawkins wrote:
> Why is this necessary?

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

https://codereview.chromium.org/578293002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos_unittest.cc:74:
ASSERT_EQ(0, fake_update_engine_client_->request_update_check_call_count());
On 2014/09/18 15:48:22, James Hawkins wrote:
> It doesn't look like any of these tests need to be ASSERT_ (as opposed to
> EXPECT_).

Done.

[ygorshenin1, 2014-09-18 15:59:15]

PTAL

[James Hawkins, 2014-09-18 16:34:03]

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

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:221: bool
VersionUpdaterCros::EnsureCanUpdate(const StatusCallback& callback) {
What change required this being on the VersionUpdaterCros object?

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

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:48: bool
EnsureCanUpdate(const StatusCallback& callback);
I'm not sure this API is really where we should go with what we need.  Is there
not a successful StatusCallback available?  The name of the method is not
intuitive either, as it doesn't ensure anything.

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:56: // True if an update
check was scheduled when update engine'll move to an idle
This comment is difficult to understand; there appears to be an issue with the
tenses; questionable part highlighted:

"True if an update check was scheduled *when update engine will move* to an idle
state."

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/help/version_updater_chromeos_unittest.cc (right):

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos_unittest.cc:64:
TEST_F(VersionUpdaterCrosTest, SetChannelDuringChannelSwitch) {
SetChannelDuringChannelSwitch is not particularly informative.  Can you either
make the test name more descriptive with respect to the behavior being tested or
add a test comment describing the same?

[ygorshenin1, 2014-09-18 19:46:47]

PTAL

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

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:221: bool
VersionUpdaterCros::EnsureCanUpdate(const StatusCallback& callback) {
On 2014/09/18 16:34:03, James Hawkins wrote:
> What change required this being on the VersionUpdaterCros object?

Check of can_update_for_testing_ field.

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

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:48: bool
EnsureCanUpdate(const StatusCallback& callback);
Agree with you that the name of the method is not intuitive, but I can't invent
a better name. Could you please propose how to name the method?

On 2014/09/18 16:34:03, James Hawkins wrote:
> I'm not sure this API is really where we should go with what we need.  Is
there
> not a successful StatusCallback available?  The name of the method is not
> intuitive either, as it doesn't ensure anything.

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:56: // True if an update
check was scheduled when update engine'll move to an idle
On 2014/09/18 16:34:03, James Hawkins wrote:
> This comment is difficult to understand; there appears to be an issue with the
> tenses; questionable part highlighted:
> 
> "True if an update check was scheduled *when update engine will move* to an
idle
> state."

Done.

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/help/version_updater_chromeos_unittest.cc (right):

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos_unittest.cc:64:
TEST_F(VersionUpdaterCrosTest, SetChannelDuringChannelSwitch) {
On 2014/09/18 16:34:03, James Hawkins wrote:
> SetChannelDuringChannelSwitch is not particularly informative.  Can you either
> make the test name more descriptive with respect to the behavior being tested
or
> add a test comment describing the same?

Done.

[satorux1, 2014-09-22 07:02:11]

chromeos/dbus lgtm with a comment:

https://codereview.chromium.org/578293002/diff/60001/chromeos/dbus/fake_updat...
File chromeos/dbus/fake_update_engine_client.h (right):

https://codereview.chromium.org/578293002/diff/60001/chromeos/dbus/fake_updat...
chromeos/dbus/fake_update_engine_client.h:68: int
request_update_check_call_count() const {
Please add a comment.

[ygorshenin1, 2014-09-22 09:23:26]

James, PTAL.

https://codereview.chromium.org/578293002/diff/60001/chromeos/dbus/fake_updat...
File chromeos/dbus/fake_update_engine_client.h (right):

https://codereview.chromium.org/578293002/diff/60001/chromeos/dbus/fake_updat...
chromeos/dbus/fake_update_engine_client.h:68: int
request_update_check_call_count() const {
On 2014/09/22 07:02:11, satorux1 wrote:
> Please add a comment.

Done.

[James Hawkins, 2014-09-22 16:48:31]

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

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:48: bool
EnsureCanUpdate(const StatusCallback& callback);
On 2014/09/18 19:46:47, ygorshenin1 wrote:
> Agree with you that the name of the method is not intuitive, but I can't
invent
> a better name. Could you please propose how to name the method?
> 
> On 2014/09/18 16:34:03, James Hawkins wrote:
> > I'm not sure this API is really where we should go with what we need.  Is
> there
> > not a successful StatusCallback available?  The name of the method is not
> > intuitive either, as it doesn't ensure anything.
> 

GetUpdateStatus and don't return a bool?  I don't know.  The fact that this is
not easy to name is a sign that we haven't reached an intuitive design for these
requirements.

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

https://codereview.chromium.org/578293002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:56: // True if an update
check should be scheduled when the update engine will
Optional nit: This is easier to grok for me if you use the phrasing, "when the
update engine is idle."

https://codereview.chromium.org/578293002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:61: bool
can_update_for_testing_;
I took a high-level look at what you're trying to accomplish, and I think,
barring other design changes, the most appropriate thing for you to do is to use
object mocking to mock out the CanUpdate method to return true under test.

A better design would be to create an interface with a method on it that
performs this check.  Then you'd implement the real implementation in the
version_updater_chromeos.cc file and have a test implementation in the
_unittest.cc file.

[ygorshenin1, 2014-09-22 18:58:53]

Patchset #6 (id:100001) has been deleted

[ygorshenin1, 2014-09-22 19:01:54]

PTAL

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

https://codereview.chromium.org/578293002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:56: // True if an update
check should be scheduled when the update engine will
On 2014/09/22 16:48:31, James Hawkins wrote:
> Optional nit: This is easier to grok for me if you use the phrasing, "when the
> update engine is idle."

Done.

https://codereview.chromium.org/578293002/diff/80001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:61: bool
can_update_for_testing_;
I've made a virtual EnsureCanUpdate() method and overridden it in the test code.

On 2014/09/22 16:48:31, James Hawkins wrote:
> I took a high-level look at what you're trying to accomplish, and I think,
> barring other design changes, the most appropriate thing for you to do is to
use
> object mocking to mock out the CanUpdate method to return true under test.
> 
> A better design would be to create an interface with a method on it that
> performs this check.  Then you'd implement the real implementation in the
> version_updater_chromeos.cc file and have a test implementation in the
> _unittest.cc file.

[James Hawkins, 2014-09-22 20:23:08]

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

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:48: bool
EnsureCanUpdate(const StatusCallback& callback);
On 2014/09/22 16:48:31, James Hawkins wrote:
> On 2014/09/18 19:46:47, ygorshenin1 wrote:
> > Agree with you that the name of the method is not intuitive, but I can't
> invent
> > a better name. Could you please propose how to name the method?
> > 
> > On 2014/09/18 16:34:03, James Hawkins wrote:
> > > I'm not sure this API is really where we should go with what we need.  Is
> > there
> > > not a successful StatusCallback available?  The name of the method is not
> > > intuitive either, as it doesn't ensure anything.
> > 
> 
> GetUpdateStatus and don't return a bool?  I don't know.  The fact that this is
> not easy to name is a sign that we haven't reached an intuitive design for
these
> requirements.

This comment is unresolved.

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

https://codereview.chromium.org/578293002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:83: if
(!EnsureCanUpdate(callback))
this->EnsureCanUpdate, right?

[ygorshenin1, 2014-09-23 10:23:16]

I've decided to move EnsureCanUpdate() function back to an anonymous namespace,
where it was before my change.

PTAL

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

https://codereview.chromium.org/578293002/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/help/version_updater_chromeos.h:48: bool
EnsureCanUpdate(const StatusCallback& callback);
I've moved back EnsureCanUpdate() to the anonymous namespace.

On 2014/09/22 20:23:08, James Hawkins wrote:
> On 2014/09/22 16:48:31, James Hawkins wrote:
> > On 2014/09/18 19:46:47, ygorshenin1 wrote:
> > > Agree with you that the name of the method is not intuitive, but I can't
> > invent
> > > a better name. Could you please propose how to name the method?
> > > 
> > > On 2014/09/18 16:34:03, James Hawkins wrote:
> > > > I'm not sure this API is really where we should go with what we need. 
Is
> > > there
> > > > not a successful StatusCallback available?  The name of the method is
not
> > > > intuitive either, as it doesn't ensure anything.
> > > 
> > 
> > GetUpdateStatus and don't return a bool?  I don't know.  The fact that this
is
> > not easy to name is a sign that we haven't reached an intuitive design for
> these
> > requirements.
> 
> This comment is unresolved.

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

https://codereview.chromium.org/578293002/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/help/version_updater_chromeos.cc:83: if
(!EnsureCanUpdate(callback))
Could you please explain a difference between a direct call and a call via
'this' pointer in this case?

On 2014/09/22 20:23:08, James Hawkins wrote:
> this->EnsureCanUpdate, right?

[James Hawkins, 2014-09-24 22:05:46]

lgtm

[ygorshenin1, 2014-09-25 09:03:27]

The CQ bit was checked by ygorshenin@chromium.org

[ygorshenin1, 2014-09-25 09:03:42]

Many thanks!

[commit-bot: I haz the power, 2014-09-25 09:04:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578293002/160001

[commit-bot: I haz the power, 2014-09-25 11:40:56]

Committed patchset #8 (id:160001) as 3a7c9fc642696135e3937663d51debc98cb7e9b5

[commit-bot: I haz the power, 2014-09-25 11:41:26]

Patchset 8 (id:??) landed as
https://crrev.com/84a24d2929796a717aef73ec7d85a87e09cd251d
Cr-Commit-Position: refs/heads/master@{#296693}