Make update over cellular an option for user

Make update over cellular an option for user

- What is changed in update engine of Chrome OS?
A new state NEED_PERMISSION_TO_UPDATE will be broadcasted to Chrome UI
When update aborts due to cellular connection.
A new dbus interface SetUpdateOverCellularTarget() is added to set
update target (version and size) in preferences maintained by update
engine.

- What is the target in SetUpdateOverCellularTarget()?
When update engine broadcast NEED_PERMISSION_TO_UPDATE, it also
broadcasts the metadata of this update such as version and size. We call
this target. Once user grants the permission to proceed update over
cellular, we call SetUpdateOverCellularTarget() to send the target back
to Chrome OS.

- Why do we need to send the target back to update engine?
There's a corner: When NEED_PERMISSION_TO_UPDATE is broadcasted to
chrome UI, an update warning dialog will show the update size to user
and ask whether to proceed. If user just keep the dialog window long
enough until there's a new update available. Then clicking 'Continue'
will result in a much larger download as the update engine only download
the lastest update.

This rarely happens, but it's more user friendly to consider this case.

We send this target back to update engine to make it check with the
server. If the target matches the latest update on the server, then
update proceeds. Otherwise, update engine needs to broadcast
NEED_PERMISSION_TO_UPDATE again with the latest update metadata.

- Design doc: go/cros-cellular-updates

- Related chromium CL: https://codereview.chromium.org/2873193002/

- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/

BUG=chromium:691108
TEST=None

Review-Url: https://codereview.chromium.org/2881473002
Cr-Commit-Position: refs/heads/master@{#471609}
Committed: https://chromium.googlesource.com/chromium/src/+/eaad51c9f5331a5344804453307c3cf5fe1ef259

[weidongg, 2017-05-11 15:47:22]

weidongg@chromium.org changed reviewers:
+ alemate@chromium.org, derat@chromium.org

[weidongg, 2017-05-11 15:47:22]

alemate@chromium.org, derat@chromium.org: Please review the CL.

[weidongg, 2017-05-11 15:47:38]

The CQ bit was checked by weidongg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 15:48:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 16:03:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 16:03:38]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[weidongg, 2017-05-11 16:48:42]

The CQ bit was checked by weidongg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 16:49:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[weidongg, 2017-05-11 16:55:10]

The CQ bit was unchecked by weidongg@chromium.org

[weidongg, 2017-05-11 18:32:05]

The CQ bit was checked by weidongg@chromium.org to run a CQ dry run

[weidongg, 2017-05-11 18:33:01]

The CQ bit was unchecked by weidongg@chromium.org

[commit-bot: I haz the power, 2017-05-11 18:33:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[weidongg, 2017-05-11 19:03:16]

The CQ bit was checked by weidongg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 19:05:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 21:30:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-11 21:30:01]

Dry run: This issue passed the CQ dry run.

[Alexander Alekseev, 2017-05-12 03:47:22]

lgtm

[Daniel Erat, 2017-05-12 13:37:04]

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/fake_upda...
File chromeos/dbus/fake_update_engine_client.cc (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/fake_upda...
chromeos/dbus/fake_update_engine_client.cc:100: // Do nothing.
nit: delete this comment. "{}" makes it clear you're doing nothing. :-P

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:486: "Failed to set UpdateEngine to allow
updates over cellular to a given "
nit: s/UpdateEngine/update_engine/

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:489: if (response) {
you don't need this lengthy error-handling code. see the implementation in
dbus/object_proxy.cc. i think the only case where you need to log an error
yourself is if response is null, e.g.

  if (!response) {
    LOG(ERROR) << update_engine::kSetUpdateOverCellularTarget
               << " call failed";
  }

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:614: }
put blank lines between method definitions

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:618: // Do nothing.
delete comment

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:44: UPDATE_STATUS_NEED_PERMISSION_TO_UPDATE
nit: put trailing comma after last item in lists

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:155: typedef base::Callback<void(bool)>
SetTargetCallback;
i think that 'using' is preferred now.

this should probably also be SetUpdateOverCellularTargetCallback.

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:158: // performs update to this given
target after |RequestUpdateCheck| is invoked
i think we usually just use || around variable names. functions are usually
written as RequestUpdateCheck() (including in the comment you added above).

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:162: int64_t target_size,
please document what target_version and target_size contain. is target_version a
chrome os version number? mind giving an example?

[weidongg, 2017-05-12 16:33:17]

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/fake_upda...
File chromeos/dbus/fake_update_engine_client.cc (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/fake_upda...
chromeos/dbus/fake_update_engine_client.cc:100: // Do nothing.
On 2017/05/12 13:37:03, Daniel Erat wrote:
> nit: delete this comment. "{}" makes it clear you're doing nothing. :-P

Done.

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:486: "Failed to set UpdateEngine to allow
updates over cellular to a given "
On 2017/05/12 13:37:03, Daniel Erat wrote:
> nit: s/UpdateEngine/update_engine/

Done.

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:489: if (response) {
On 2017/05/12 13:37:03, Daniel Erat wrote:
> you don't need this lengthy error-handling code. see the implementation in
> dbus/object_proxy.cc. i think the only case where you need to log an error
> yourself is if response is null, e.g.
> 
>   if (!response) {
>     LOG(ERROR) << update_engine::kSetUpdateOverCellularTarget
>                << " call failed";
>   }

Done, should I modify the OnSetUpdateOverCellularPermission() using the same
strategy?

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:614: }
On 2017/05/12 13:37:03, Daniel Erat wrote:
> put blank lines between method definitions

Done.

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:618: // Do nothing.
On 2017/05/12 13:37:03, Daniel Erat wrote:
> delete comment

Done.

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:44: UPDATE_STATUS_NEED_PERMISSION_TO_UPDATE
On 2017/05/12 13:37:03, Daniel Erat wrote:
> nit: put trailing comma after last item in lists

Done.

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:155: typedef base::Callback<void(bool)>
SetTargetCallback;
On 2017/05/12 13:37:03, Daniel Erat wrote:
> i think that 'using' is preferred now.
> 
> this should probably also be SetUpdateOverCellularTargetCallback.

Do you mean I should 'using base::Callback;' at the beginning of the file?

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:158: // performs update to this given
target after |RequestUpdateCheck| is invoked
On 2017/05/12 13:37:03, Daniel Erat wrote:
> i think we usually just use || around variable names. functions are usually
> written as RequestUpdateCheck() (including in the comment you added above).

Done.

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:162: int64_t target_size,
On 2017/05/12 13:37:03, Daniel Erat wrote:
> please document what target_version and target_size contain. is target_version
a
> chrome os version number? mind giving an example?

Ok, I add explanation.

[Daniel Erat, 2017-05-12 16:44:48]

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:489: if (response) {
On 2017/05/12 16:33:16, weidongg wrote:
> On 2017/05/12 13:37:03, Daniel Erat wrote:
> > you don't need this lengthy error-handling code. see the implementation in
> > dbus/object_proxy.cc. i think the only case where you need to log an error
> > yourself is if response is null, e.g.
> > 
> >   if (!response) {
> >     LOG(ERROR) << update_engine::kSetUpdateOverCellularTarget
> >                << " call failed";
> >   }
> 
> Done, should I modify the OnSetUpdateOverCellularPermission() using the same
> strategy?

yes, please do. unnecessary error handling code like this just increases the
binary size. thanks!

please avoid copying-and-pasting code in general, too. if you actually needed
this, the right approach would be to refactor the common code into a new helper
function in the anonymous namespace.

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:155: typedef base::Callback<void(bool)>
SetTargetCallback;
On 2017/05/12 16:33:17, weidongg wrote:
> On 2017/05/12 13:37:03, Daniel Erat wrote:
> > i think that 'using' is preferred now.
> > 
> > this should probably also be SetUpdateOverCellularTargetCallback.
> 
> Do you mean I should 'using base::Callback;' at the beginning of the file?

no, this:

  using SetUpdateOverCellularTargetCallback =
      base::Callback<void(bool success)>;

(i think you can include parameter names in callbacks)

[weidongg, 2017-05-12 17:00:15]

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.cc (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.cc:489: if (response) {
On 2017/05/12 16:44:48, Daniel Erat wrote:
> On 2017/05/12 16:33:16, weidongg wrote:
> > On 2017/05/12 13:37:03, Daniel Erat wrote:
> > > you don't need this lengthy error-handling code. see the implementation in
> > > dbus/object_proxy.cc. i think the only case where you need to log an error
> > > yourself is if response is null, e.g.
> > > 
> > >   if (!response) {
> > >     LOG(ERROR) << update_engine::kSetUpdateOverCellularTarget
> > >                << " call failed";
> > >   }
> > 
> > Done, should I modify the OnSetUpdateOverCellularPermission() using the same
> > strategy?
> 
> yes, please do. unnecessary error handling code like this just increases the
> binary size. thanks!
> 
> please avoid copying-and-pasting code in general, too. if you actually needed
> this, the right approach would be to refactor the common code into a new
helper
> function in the anonymous namespace.

Ok, I see. Good to know.

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/2881473002/diff/60001/chromeos/dbus/update_en...
chromeos/dbus/update_engine_client.h:155: typedef base::Callback<void(bool)>
SetTargetCallback;
On 2017/05/12 16:44:48, Daniel Erat wrote:
> On 2017/05/12 16:33:17, weidongg wrote:
> > On 2017/05/12 13:37:03, Daniel Erat wrote:
> > > i think that 'using' is preferred now.
> > > 
> > > this should probably also be SetUpdateOverCellularTargetCallback.
> > 
> > Do you mean I should 'using base::Callback;' at the beginning of the file?
> 
> no, this:
> 
>   using SetUpdateOverCellularTargetCallback =
>       base::Callback<void(bool success)>;
> 
> (i think you can include parameter names in callbacks)

Done.

[weidongg, 2017-05-12 17:58:50]

weidongg@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[weidongg, 2017-05-12 17:58:50]

dmazzoni@chromium.org: Please review changes in 

chrome/browser/extensions/api/system_private/system_private_api.cc

[weidongg, 2017-05-12 17:59:27]

The CQ bit was checked by weidongg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 18:00:55]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[weidongg, 2017-05-12 18:12:18]

Description was changed from

==========
Make update over cellular an option for user

- What is changed in update engine of Chrome OS?
A new state NEED_PERMISSION_TO_UPDATE will be broadcasted to Chrome UI
When update aborts due to cellular connection.
A new dbus interface |SetUpdateOverCellularTarget| is added to set
update target (version and size) in preferences maintained by update
engine.

- What is the target in |SetUpdateOverCellularTarget|?
When update engine broadcast NEED_PERMISSION_TO_UPDATE, it also
broadcasts the metadata of this update such as version and size. We call
this target. Once user grants the permission to proceed update over
cellular, we call |SetUpdateOverCellularTarget| to send the target back
to Chrome OS.

- Why do we need to send the target back to update engine?
There's a corner: When NEED_PERMISSION_TO_UPDATE is broadcasted to
chrome UI, an update warning dialog will show the update size to user
and ask whether to proceed. If user just keep the dialog window long
enough until there's a new update available. Then clicking 'Continue'
will result in a much larger download as the update engine only download
the lastest update.

This rarely happens, but it's more user friendly to consider this case.

We send this target back to update engine to make it check with the
server. If the target matches the latest update on the server, then
update proceeds. Otherwise, update engine needs to broadcast
NEED_PERMISSION_TO_UPDATE again with the latest update metadata.

- Design doc: go/cros-cellular-updates

- Related chromium CL: https://codereview.chromium.org/2873193002/

- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/

BUG=chromium:691108
TEST=None
==========

to

==========
Make update over cellular an option for user

- What is changed in update engine of Chrome OS?
A new state NEED_PERMISSION_TO_UPDATE will be broadcasted to Chrome UI
When update aborts due to cellular connection.
A new dbus interface SetUpdateOverCellularTarget() is added to set
update target (version and size) in preferences maintained by update
engine.

- What is the target in SetUpdateOverCellularTarget()?
When update engine broadcast NEED_PERMISSION_TO_UPDATE, it also
broadcasts the metadata of this update such as version and size. We call
this target. Once user grants the permission to proceed update over
cellular, we call SetUpdateOverCellularTarget() to send the target back
to Chrome OS.

- Why do we need to send the target back to update engine?
There's a corner: When NEED_PERMISSION_TO_UPDATE is broadcasted to
chrome UI, an update warning dialog will show the update size to user
and ask whether to proceed. If user just keep the dialog window long
enough until there's a new update available. Then clicking 'Continue'
will result in a much larger download as the update engine only download
the lastest update.

This rarely happens, but it's more user friendly to consider this case.

We send this target back to update engine to make it check with the
server. If the target matches the latest update on the server, then
update proceeds. Otherwise, update engine needs to broadcast
NEED_PERMISSION_TO_UPDATE again with the latest update metadata.

- Design doc: go/cros-cellular-updates

- Related chromium CL: https://codereview.chromium.org/2873193002/

- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/

BUG=chromium:691108
TEST=None
==========

[Daniel Erat, 2017-05-12 18:13:30]

thanks, lgtm w/nits

https://codereview.chromium.org/2881473002/diff/100001/chromeos/dbus/update_e...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/2881473002/diff/100001/chromeos/dbus/update_e...
chromeos/dbus/update_engine_client.h:158: // Set the target in the preferences
maintained by update engine which then
nit: s/Set/Sets/

https://codereview.chromium.org/2881473002/diff/100001/chromeos/dbus/update_e...
chromeos/dbus/update_engine_client.h:161: // - target_version: the chrome os
version we want to update to.
nit: s/chrome os/Chrome OS/

https://codereview.chromium.org/2881473002/diff/100001/chromeos/dbus/update_e...
chromeos/dbus/update_engine_client.h:162: // - target_size: the size of that
chrome os version in bytes.
same here

[weidongg, 2017-05-12 18:23:19]

Thanks for the code review.

https://codereview.chromium.org/2881473002/diff/100001/chromeos/dbus/update_e...
File chromeos/dbus/update_engine_client.h (right):

https://codereview.chromium.org/2881473002/diff/100001/chromeos/dbus/update_e...
chromeos/dbus/update_engine_client.h:158: // Set the target in the preferences
maintained by update engine which then
On 2017/05/12 18:13:30, Daniel Erat wrote:
> nit: s/Set/Sets/

Done.

https://codereview.chromium.org/2881473002/diff/100001/chromeos/dbus/update_e...
chromeos/dbus/update_engine_client.h:161: // - target_version: the chrome os
version we want to update to.
On 2017/05/12 18:13:30, Daniel Erat wrote:
> nit: s/chrome os/Chrome OS/

Done.

https://codereview.chromium.org/2881473002/diff/100001/chromeos/dbus/update_e...
chromeos/dbus/update_engine_client.h:162: // - target_size: the size of that
chrome os version in bytes.
On 2017/05/12 18:13:30, Daniel Erat wrote:
> same here

Done.

[dmazzoni, 2017-05-12 18:54:56]

On 2017/05/12 17:58:50, weidongg wrote:
> mailto:dmazzoni@chromium.org: Please review changes in 
> 
> chrome/browser/extensions/api/system_private/system_private_api.cc

I'm not a good owner for this change, please send to someone in
extensions/OWNERS

[weidongg, 2017-05-12 19:43:19]

weidongg@chromium.org changed reviewers:
+ reillyg@chromium.org
- dmazzoni@chromium.org

[weidongg, 2017-05-12 19:45:33]

reillyg@chromium.org: Please review the changes in:

chrome/browser/extensions/api/system_private/system_private_api.cc

[Reilly Grant (use Gerrit), 2017-05-12 20:01:53]

lgtm

[weidongg, 2017-05-12 20:32:09]

The CQ bit was checked by weidongg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 20:33:10]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[weidongg, 2017-05-12 20:33:16]

The CQ bit was checked by weidongg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 20:33:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 21:36:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 21:36:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[weidongg, 2017-05-12 22:04:44]

The CQ bit was checked by weidongg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 22:05:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 00:16:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 00:16:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[weidongg, 2017-05-13 00:21:14]

The CQ bit was checked by weidongg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-13 00:22:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 00:31:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 00:31:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[weidongg, 2017-05-13 00:48:44]

The CQ bit was checked by weidongg@chromium.org

[weidongg, 2017-05-13 00:48:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
alemate@chromium.org, derat@chromium.org
Link to the patchset: https://codereview.chromium.org/2881473002/#ps120001
(title: "Apply fix to patch set 6")

[commit-bot: I haz the power, 2017-05-13 00:49:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 00:58:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 00:58:37]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[weidongg, 2017-05-13 01:11:36]

The CQ bit was checked by weidongg@chromium.org

[commit-bot: I haz the power, 2017-05-13 01:12:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 01:19:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 01:19:35]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[weidongg, 2017-05-13 01:32:28]

The CQ bit was checked by weidongg@chromium.org

[commit-bot: I haz the power, 2017-05-13 01:32:48]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 01:40:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 01:40:53]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Daniel Erat, 2017-05-13 02:20:23]

it looks like you have a merge conflict in your DEPS change. please merge and
check that you're using whichever revision of cros_system_api is the most recent
between the one that you're requesting and the one that's now checked in.

[weidongg, 2017-05-13 02:26:30]

The CQ bit was checked by weidongg@chromium.org

[weidongg, 2017-05-13 02:26:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
alemate@chromium.org, derat@chromium.org, reillyg@chromium.org
Link to the patchset: https://codereview.chromium.org/2881473002/#ps140001
(title: "remove DEPS as it is already update to date")

[commit-bot: I haz the power, 2017-05-13 02:26:50]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[weidongg, 2017-05-13 02:27:45]

On 2017/05/13 02:20:23, Daniel Erat wrote:
> it looks like you have a merge conflict in your DEPS change. please merge and
> check that you're using whichever revision of cros_system_api is the most
recent
> between the one that you're requesting and the one that's now checked in.

Thanks for reminding, I removed the DEPS from CL as it is already one revision
ahead of mine.

[commit-bot: I haz the power, 2017-05-13 02:50:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-13 02:50:40]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[weidongg, 2017-05-13 22:27:11]

The CQ bit was checked by weidongg@chromium.org

[commit-bot: I haz the power, 2017-05-13 22:27:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-13 22:49:38]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1494714431700280,
"parent_rev": "a672d19ad76be908d9339fe093f88fd1e66b99f8", "commit_rev":
"eaad51c9f5331a5344804453307c3cf5fe1ef259"}

[commit-bot: I haz the power, 2017-05-13 22:49:56]

Description was changed from

==========
Make update over cellular an option for user

- What is changed in update engine of Chrome OS?
A new state NEED_PERMISSION_TO_UPDATE will be broadcasted to Chrome UI
When update aborts due to cellular connection.
A new dbus interface SetUpdateOverCellularTarget() is added to set
update target (version and size) in preferences maintained by update
engine.

- What is the target in SetUpdateOverCellularTarget()?
When update engine broadcast NEED_PERMISSION_TO_UPDATE, it also
broadcasts the metadata of this update such as version and size. We call
this target. Once user grants the permission to proceed update over
cellular, we call SetUpdateOverCellularTarget() to send the target back
to Chrome OS.

- Why do we need to send the target back to update engine?
There's a corner: When NEED_PERMISSION_TO_UPDATE is broadcasted to
chrome UI, an update warning dialog will show the update size to user
and ask whether to proceed. If user just keep the dialog window long
enough until there's a new update available. Then clicking 'Continue'
will result in a much larger download as the update engine only download
the lastest update.

This rarely happens, but it's more user friendly to consider this case.

We send this target back to update engine to make it check with the
server. If the target matches the latest update on the server, then
update proceeds. Otherwise, update engine needs to broadcast
NEED_PERMISSION_TO_UPDATE again with the latest update metadata.

- Design doc: go/cros-cellular-updates

- Related chromium CL: https://codereview.chromium.org/2873193002/

- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/

BUG=chromium:691108
TEST=None
==========

to

==========
Make update over cellular an option for user

- What is changed in update engine of Chrome OS?
A new state NEED_PERMISSION_TO_UPDATE will be broadcasted to Chrome UI
When update aborts due to cellular connection.
A new dbus interface SetUpdateOverCellularTarget() is added to set
update target (version and size) in preferences maintained by update
engine.

- What is the target in SetUpdateOverCellularTarget()?
When update engine broadcast NEED_PERMISSION_TO_UPDATE, it also
broadcasts the metadata of this update such as version and size. We call
this target. Once user grants the permission to proceed update over
cellular, we call SetUpdateOverCellularTarget() to send the target back
to Chrome OS.

- Why do we need to send the target back to update engine?
There's a corner: When NEED_PERMISSION_TO_UPDATE is broadcasted to
chrome UI, an update warning dialog will show the update size to user
and ask whether to proceed. If user just keep the dialog window long
enough until there's a new update available. Then clicking 'Continue'
will result in a much larger download as the update engine only download
the lastest update.

This rarely happens, but it's more user friendly to consider this case.

We send this target back to update engine to make it check with the
server. If the target matches the latest update on the server, then
update proceeds. Otherwise, update engine needs to broadcast
NEED_PERMISSION_TO_UPDATE again with the latest update metadata.

- Design doc: go/cros-cellular-updates

- Related chromium CL: https://codereview.chromium.org/2873193002/

- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/

BUG=chromium:691108
TEST=None

Review-Url: https://codereview.chromium.org/2881473002
Cr-Commit-Position: refs/heads/master@{#471609}
Committed:
https://chromium.googlesource.com/chromium/src/+/eaad51c9f5331a5344804453307c...
==========

[commit-bot: I haz the power, 2017-05-13 22:49:58]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/eaad51c9f5331a5344804453307c...