Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873193002:
Make update over cellular an option for user (Closed)
|
Created:
3 years, 7 months ago by weidongg Modified:
3 years, 7 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, alemate+watch_chromium.org, asvitkine+watch_chromium.org, hashimoto+watch_chromium.org, michaelpg+watch-md-settings_chromium.org, achuith+watch_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, davemoore+watch_chromium.org, afakhry Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake 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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2873193002
Cr-Commit-Position: refs/heads/master@{#471639}
Committed: https://chromium.googlesource.com/chromium/src/+/f472642fd6c20b63b0f1a2f62564f2aad56f4be3
Patch Set 1 #
Total comments: 16
Patch Set 2 : Move update engine related code to another CL #
Total comments: 16
Patch Set 3 : Make update over cellular an option for user #
Total comments: 28
Patch Set 4 : Bad patch set, please ignore #Patch Set 5 : Apply fix to patch set 3 #
Total comments: 2
Patch Set 6 : Apply fix to patch set 5 #
Total comments: 3
Patch Set 7 : Run callback when SetUpdateOverCellularTarget fails #Patch Set 8 : Apply fix to trybot errors #Patch Set 9 : Put code in CHROME_OS wrapper to fix trybot error #Messages
Total messages: 47 (24 generated)
Description was changed from ========== Make update over cellular an option for user - Allow updates over cellular by default in chrome UI, delay the decision making as to whether to perform updates until after update engine detects that an update is available. - Handle NEED_PERMISSION_TO_UPDATE state broadcasted by update engine. Show up an update warning dialog (showing update size )and let user to decide whether proceed to download the update over cellular. - Make dbus calls to update engine to request update over cellular if the user chooses to proceed. - Corner case: The update warning dialog shows up about update version 1.0, then version 1.1 is pushed to the server. The user selects to proceed updates, which results in a much larger download as the update engine always get the latest version from the server. Solution is to add |SetUpdateOverCellularTarget| dbus call to set update size and version in user preferences. Then update engine checks these target with the server to see if there's a new update available. If it does, simply broadcast NEED_PERMISSION_TO_UPDATE again with new update size and version. - Design doc: go/cros-cellular-updates BUG=chromium:691108 CQ-DEPEND=CL:479467 TEST=None ========== to ========== Make update over cellular an option for user - Allow updates over cellular by default in chrome UI, delay the decision making as to whether to perform updates until after update engine detects that an update is available. - Handle NEED_PERMISSION_TO_UPDATE state broadcasted by update engine. Show up an update warning dialog (showing update size )and let user to decide whether proceed to download the update over cellular. - Make dbus calls to update engine to request update over cellular if the user chooses to proceed. - Corner case: The update warning dialog shows up about update version 1.0, then version 1.1 is pushed to the server. The user selects to proceed updates, which results in a much larger download as the update engine always get the latest version from the server. Solution is to add |SetUpdateOverCellularTarget| dbus call to set update size and version in user preferences. Then update engine checks these target with the server to see if there's a new update available. If it does, simply broadcast NEED_PERMISSION_TO_UPDATE again with new update size and version. - Design doc: go/cros-cellular-updates BUG=chromium:691108 CQ-DEPEND=CL:479467 TEST=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
weidongg@chromium.org changed reviewers: + stevenjb@chromium.org
Please review the CL, the os-side CL is https://chromium-review.googlesource.com/#/c/479467/
weidongg@chromium.org changed reviewers: + pkasting@chromium.org
pkasting@chromium.org: Please review the CL. It is dependent on os-side CL: https://chromium-review.googlesource.com/#/c/479467/
It looks like this should have been sent to a different reviewer. The only files in here I believe I own are in c/b/ui/, and all of those are under the webui/ directory, which has more specific OWNERS. Folks in https://cs.chromium.org/chromium/src/ui/webui/PLATFORM_OWNERS?type=cs (which that file refers to) would probably be better?
weidongg@chromium.org changed reviewers: - pkasting@chromium.org
weidongg@chromium.org changed reviewers: + xiyuan@chromium.org
Could you separate the WebUI changes from the update_engine_client.cc related changes into a separate CL? Also please add derat@ and alemate@ to the update_engine_client.cc CL. Thanks! https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:177: dialog.setUpdateWarningMessage(event.version, event.size); This can be done a bit more cleanly by using data binding to set something like this.updateInfo_ = { version: event.version, size: event.size } then using data binding to bind updateInfo_ to the dialog, e.g. update-info="[[updateInfo_]] as an attribute of the dialog. https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:183: }.bind(this)); This can be done by adding on-close="onUpdateWarningDialogClose_" to the HTML and moving this to the named function. This is generally preferred to injecting the event in JS like this. https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:219: }, This should also be defined (and documented) in the prototype and marked /** @override */ here. I'm confused why we would need to pass |target_size| here. https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/update_warning_dialog.js (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/update_warning_dialog.js:50: Math.floor(Number(size) / (1024 * 1024))); This can be accomplished with an updateInfo_ observer if we switch to using data binding to pass version and size. https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/help_handler.cc (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/help_handler.cc:609: const std::string& version, Document that this is unused with: const std::string& /* version */, https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/help_handler.cc:610: const int64_t size, ditto https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_chromeos.cc:192: LOG(ERROR) << "Error setting update over cellular target."; {} https://codereview.chromium.org/2873193002/diff/1/chromeos/dbus/update_engine... File chromeos/dbus/update_engine_client.h (right): https://codereview.chromium.org/2873193002/diff/1/chromeos/dbus/update_engine... chromeos/dbus/update_engine_client.h:159: // in the |callback|. Document |target_version| and |target_size|. In particular I am unclear what |target_size| is used for.
Description was changed from
==========
Make update over cellular an option for user
- Allow updates over cellular by default in chrome UI, delay the
decision making as to whether to perform updates until after update
engine detects that an update is available.
- Handle NEED_PERMISSION_TO_UPDATE state broadcasted by update engine.
Show up an update warning dialog (showing update size )and let user to
decide whether proceed to download the update over cellular.
- Make dbus calls to update engine to request update over cellular if
the user chooses to proceed.
- Corner case:
The update warning dialog shows up about update version 1.0, then
version 1.1 is pushed to the server. The user selects to proceed
updates, which results in a much larger download as the update engine
always get the latest version from the server.
Solution is to add |SetUpdateOverCellularTarget| dbus call to set update
size and version in user preferences. Then update engine checks these
target with the server to see if there's a new update available. If it
does, simply broadcast NEED_PERMISSION_TO_UPDATE again with new update
size and version.
- Design doc: go/cros-cellular-updates
BUG=chromium:691108
CQ-DEPEND=CL:479467
TEST=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
==========
https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:177: dialog.setUpdateWarningMessage(event.version, event.size); On 2017/05/10 23:29:05, stevenjb wrote: > This can be done a bit more cleanly by using data binding to set something like > this.updateInfo_ = { version: event.version, size: event.size } then using data > binding to bind updateInfo_ to the dialog, e.g. update-info="[[updateInfo_]] as > an attribute of the dialog. Done. https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:183: }.bind(this)); On 2017/05/10 23:29:05, stevenjb wrote: > This can be done by adding on-close="onUpdateWarningDialogClose_" to the HTML > and moving this to the named function. This is generally preferred to injecting > the event in JS like this. Done. https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:219: }, On 2017/05/10 23:29:05, stevenjb wrote: > This should also be defined (and documented) in the prototype and marked /** > @override */ here. I'm confused why we would need to pass |target_size| here. > I discussed with @adlr, he says "Technically version should be enough, but there are a few scenarios where it could try to get a delta (small) update and then fail a few times and then go for a full update (big), which would use more data than the user agreed to. Also, it's unlikely, but a server change could remove the delta update and leave the full one which would be the same issue." https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/update_warning_dialog.js (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/update_warning_dialog.js:50: Math.floor(Number(size) / (1024 * 1024))); On 2017/05/10 23:29:05, stevenjb wrote: > This can be accomplished with an updateInfo_ observer if we switch to using data > binding to pass version and size. Done. https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/help_handler.cc (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/help_handler.cc:609: const std::string& version, On 2017/05/10 23:29:05, stevenjb wrote: > Document that this is unused with: > const std::string& /* version */, Done. https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/help_handler.cc:610: const int64_t size, On 2017/05/10 23:29:05, stevenjb wrote: > ditto Done. https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/ui/webui/hel... chrome/browser/ui/webui/help/version_updater_chromeos.cc:192: LOG(ERROR) << "Error setting update over cellular target."; On 2017/05/10 23:29:05, stevenjb wrote: > {} Done.
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/2881473002/
- 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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
==========
https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js (right): https://codereview.chromium.org/2873193002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:219: }, On 2017/05/11 16:34:05, weidongg wrote: > On 2017/05/10 23:29:05, stevenjb wrote: > > This should also be defined (and documented) in the prototype and marked /** > > @override */ here. I'm confused why we would need to pass |target_size| here. > > > I discussed with @adlr, he says > "Technically version should be enough, but there are a few scenarios where it > could try to get a delta (small) update and then fail a few times and then go > for a full update (big), which would use more data than the user agreed to. > Also, it's unlikely, but a server change could remove the delta update and leave > the full one which would be the same issue." I'm still confused. It looks like we are just passing the 'size' parameter from the 'updateStatusChaged' event back to requestUdpateOverCellular? Do we anticipate the UI ever changing either the size or version information? It seems like this doesn't actually need any parameters and the model (the C++ code) should just retrieve those values? https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:98: }, I think this can just be undefined initially; we will never show the dialog without setting this. Also, this needs a type which should be defined in the browser proxy: /** * @typedef {{ * version: string, * size: string * }} */ var AboutPageUpdateInfo; Then here: /** @private {!AboutPageUpdateInfo|undefined} */ updateInfo_: Object, https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:180: this.async(function() { This no longer needs to be async since we do not need to call this.$$() to get the dialog. https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js (right): https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:81: * connectionTypes: (string|undefined), It looks like this now has 'version' and 'size' properties? https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:152: * cellular. Maybe provide a brief explanation of why/how size is used and where it comes from, since it is not obvious. https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/update_warning_dialog.js (right): https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/update_warning_dialog.js:16: /** @public */ We don't use @public since that is the default. We do need a type though: /** @type {!AboutPageUpdateInfo|undefined} */ https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/update_warning_dialog.js:19: value: {version: '', size: ''}, Shouldn't need to provide a default value here since it will always be set by the page. https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/update_warning_dialog.js:20: observer: '_updateInfoChanged', Polymer uses _foo, but in the WebUI code we use foo_. https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/about_page/update_warning_dialog.js:53: _updateInfoChanged: function() { updateInfoChanged_:
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/2881473002/
- 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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
Yes we don't change the size or version in the Chrome UI, we simply send it back
to update engine. This is helpful to prevent a corner case:
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 to make Chrome UI show up update warning dialog
again.
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page.js (right):
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:98: },
On 2017/05/11 17:20:15, stevenjb wrote:
> I think this can just be undefined initially; we will never show the dialog
> without setting this.
>
> Also, this needs a type which should be defined in the browser proxy:
> /**
> * @typedef {{
> * version: string,
> * size: string
> * }}
> */
> var AboutPageUpdateInfo;
>
> Then here:
>
> /** @private {!AboutPageUpdateInfo|undefined} */
> updateInfo_: Object,
Done.
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page.js:180:
this.async(function() {
On 2017/05/11 17:20:15, stevenjb wrote:
> This no longer needs to be async since we do not need to call this.$$() to get
> the dialog.
Done.
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js
(right):
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:81: *
connectionTypes: (string|undefined),
On 2017/05/11 17:20:15, stevenjb wrote:
> It looks like this now has 'version' and 'size' properties?
Done.
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:152: *
cellular.
On 2017/05/11 17:20:15, stevenjb wrote:
> Maybe provide a brief explanation of why/how size is used and where it comes
> from, since it is not obvious.
Done.
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/about_page/update_warning_dialog.js
(right):
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:16: /**
@public */
On 2017/05/11 17:20:16, stevenjb wrote:
> We don't use @public since that is the default. We do need a type though:
>
> /** @type {!AboutPageUpdateInfo|undefined} */
Done.
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:19: value:
{version: '', size: ''},
On 2017/05/11 17:20:16, stevenjb wrote:
> Shouldn't need to provide a default value here since it will always be set by
> the page.
Done.
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:20:
observer: '_updateInfoChanged',
On 2017/05/11 17:20:16, stevenjb wrote:
> Polymer uses _foo, but in the WebUI code we use foo_.
Done.
https://codereview.chromium.org/2873193002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/about_page/update_warning_dialog.js:53:
_updateInfoChanged: function() {
On 2017/05/11 17:20:16, stevenjb wrote:
> updateInfoChanged_:
Done.
https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:226: <template is="dom-if" if="[[showUpdateWarningDialog_]]" restamp> Should this block under <if expr="chromeos"> ? https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (left): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:4: nit: restore this empty line and insert one after the @fileoverview comment. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:274: return ""; If this is intended, please document why. Otherwise create a proper message or at least put a TODO. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater.h (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater.h:66: const base::string16&)> You need to update version_updater_win.cc and version_updater_mac.mm where StatusCallback is invoked, similar to what you did in version_updater_basic.cc. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater.h:88: // engine. The preferences are latter used by update engine to match the given nit: preferences -> |target_version| and |target_size| arguments latter -> later "preferences" usually refers to a Pref in PrefService in Chrome so avoid using that for something else. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (left): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:305: nit: please restore https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:583: base::Bind(&AboutHandler::SetUpdateStatus, base::Unretained(this)), Use weak_factory_.GetWeakPtr() instead of base::Unretained(this) https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.h (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.h:111: // Checks for and applies update over cellular connection, triggered by JS. nit: insert an empty line before
https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:226: <template is="dom-if" if="[[showUpdateWarningDialog_]]" restamp> On 2017/05/11 20:37:25, xiyuan wrote: > Should this block under <if expr="chromeos"> ? Good catch. Yes it should. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:274: return ""; On 2017/05/11 20:37:25, xiyuan wrote: > If this is intended, please document why. Otherwise create a proper message or > at least put a TODO. Also use '' not "" (but it seems like we should have a proper message here?) https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:162: * cellular. The target version and size are the same as we received from nit: s/we/were/ https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:166: * didn't agree. s/agree/agree to/ https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:58: // need to check device policy and user preferences during checking for update s/need/needs/ s/during/when/ s/update/updates/ https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:192: LOG(ERROR) << "Error setting update over cellular target."; Can't we just invoke callback_.Run(FAILED, ...), at least for now? https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:583: base::Bind(&AboutHandler::SetUpdateStatus, base::Unretained(this)), On 2017/05/11 20:37:25, xiyuan wrote: > Use weak_factory_.GetWeakPtr() instead of base::Unretained(this) Technically we don't need a weak_ptr here (and don't use one above in RequestUpdate() since version_updater_ is an owned memeber and will be destroyed with this class.
https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.html (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.html:226: <template is="dom-if" if="[[showUpdateWarningDialog_]]" restamp> On 2017/05/11 20:37:25, xiyuan wrote: > Should this block under <if expr="chromeos"> ? Yes, you are right. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:274: return ""; On 2017/05/11 21:03:45, stevenjb wrote: > On 2017/05/11 20:37:25, xiyuan wrote: > > If this is intended, please document why. Otherwise create a proper message or > > at least put a TODO. > > Also use '' not "" (but it seems like we should have a proper message here?) > This transition is IDLE->CHECKING_FOR_UPDATE->NEED_PERMISSION_TO_UPDATE->REPORTING_ERROR_EVENT ->IDLE. Right now the UI shows your device is update to date upon error event. This behavior should be changed in the future. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page_browser_proxy.js (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:162: * cellular. The target version and size are the same as we received from On 2017/05/11 21:03:45, stevenjb wrote: > nit: s/we/were/ Done. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page_browser_proxy.js:166: * didn't agree. On 2017/05/11 21:03:45, stevenjb wrote: > s/agree/agree to/ Done. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater.h (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater.h:66: const base::string16&)> On 2017/05/11 20:37:25, xiyuan wrote: > You need to update version_updater_win.cc and version_updater_mac.mm where > StatusCallback is invoked, similar to what you did in version_updater_basic.cc. Done. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater.h:88: // engine. The preferences are latter used by update engine to match the given On 2017/05/11 20:37:25, xiyuan wrote: > nit: preferences -> |target_version| and |target_size| arguments > latter -> later > > "preferences" usually refers to a Pref in PrefService in Chrome so avoid using > that for something else. Ok, I see. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:58: // need to check device policy and user preferences during checking for update On 2017/05/11 21:03:45, stevenjb wrote: > s/need/needs/ > s/during/when/ > s/update/updates/ Done. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:192: LOG(ERROR) << "Error setting update over cellular target."; On 2017/05/11 21:03:45, stevenjb wrote: > Can't we just invoke callback_.Run(FAILED, ...), at least for now? > I think we could do that in the future. Running this callback will trigger js to show errors we haven't defined. Right now, all the error messages are blocked by VersionUpdaterCros::UpdateStatusChanged. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (left): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:305: On 2017/05/11 20:37:25, xiyuan wrote: > nit: please restore Done. https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:583: base::Bind(&AboutHandler::SetUpdateStatus, base::Unretained(this)), On 2017/05/11 21:03:45, stevenjb wrote: > On 2017/05/11 20:37:25, xiyuan wrote: > > Use weak_factory_.GetWeakPtr() instead of base::Unretained(this) > > Technically we don't need a weak_ptr here (and don't use one above in > RequestUpdate() since version_updater_ is an owned memeber and will be destroyed > with this class. Should I leave it unchanged? https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.h (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.h:111: // Checks for and applies update over cellular connection, triggered by JS. On 2017/05/11 20:37:26, xiyuan wrote: > nit: insert an empty line before Done.
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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
lgtm I update CL description to insert an empty time between subject and body, and inserted an enter to wrapped a line. The codereview description will be used as commit message. The usual format is like: [Subject, ideally less than 72 chars] <empty_line> [body, ideally less than 80 chars] https://codereview.chromium.org/2873193002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2873193002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:8: */ nit: insert an empty line after the @fileoverview comment block
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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
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/2881473002/
- Dependent on chromeos CL:
https://chromium-review.googlesource.com/#/c/479467/
https://chromium-review.googlesource.com/#/c/481102/
BUG=chromium:691108
TEST=None
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========
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/2881473002/ - Dependent on chromeos CL: https://chromium-review.googlesource.com/#/c/479467/ https://chromium-review.googlesource.com/#/c/481102/ BUG=chromium:691108 TEST=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== 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/2881473002/ - Dependent on chromeos CL: https://chromium-review.googlesource.com/#/c/479467/ https://chromium-review.googlesource.com/#/c/481102/ BUG=chromium:691108 TEST=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Thanks for the review. I update the commit message from |SetUpdateOverCellularTarget| to SetUpdateOverCellularTarget(). https://codereview.chromium.org/2873193002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2873193002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:8: */ On 2017/05/12 17:59:28, xiyuan wrote: > nit: insert an empty line after the @fileoverview comment block Done.
https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:192: LOG(ERROR) << "Error setting update over cellular target."; On 2017/05/11 23:50:44, weidongg wrote: > On 2017/05/11 21:03:45, stevenjb wrote: > > Can't we just invoke callback_.Run(FAILED, ...), at least for now? > > > I think we could do that in the future. Running this callback will trigger js to > show errors we haven't defined. Right now, all the error messages are blocked by > VersionUpdaterCros::UpdateStatusChanged. Hmm. I'm concerned about having a path where |callback_| is never run. Can we just pass an empty error message, or add a temporary one? https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/about_handler.cc:585: target_version, target_size); If AboutHandler::SetUpdateStatus never runs then update-status-changed is never fired, which I guess is OK, but we should make a note of that in the browser proxy JS.
https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): https://codereview.chromium.org/2873193002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:192: LOG(ERROR) << "Error setting update over cellular target."; On 2017/05/12 18:28:54, stevenjb wrote: > On 2017/05/11 23:50:44, weidongg wrote: > > On 2017/05/11 21:03:45, stevenjb wrote: > > > Can't we just invoke callback_.Run(FAILED, ...), at least for now? > > > > > I think we could do that in the future. Running this callback will trigger js > to > > show errors we haven't defined. Right now, all the error messages are blocked > by > > VersionUpdaterCros::UpdateStatusChanged. > > Hmm. I'm concerned about having a path where |callback_| is never run. Can we > just pass an empty error message, or add a temporary one? Yes, we could pass an empty error message, In that case, About Chrome OS page shows an exclamation mark with no message on the top line. screenshot: https://screenshot.googleplex.com/T6YcfWFd2Vc https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/about_handler.cc:585: target_version, target_size); On 2017/05/12 18:28:54, stevenjb wrote: > If AboutHandler::SetUpdateStatus never runs then update-status-changed is never > fired, which I guess is OK, but we should make a note of that in the browser > proxy JS. We could run the callback if SetUpdateOverCellularTarget() fails.
lgtm as-is or with more explicit handling of the new error case in SetUpdateStatus(). https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/settings/about_handler.cc:585: target_version, target_size); On 2017/05/12 20:31:57, weidongg wrote: > On 2017/05/12 18:28:54, stevenjb wrote: > > If AboutHandler::SetUpdateStatus never runs then update-status-changed is > never > > fired, which I guess is OK, but we should make a note of that in the browser > > proxy JS. > > We could run the callback if SetUpdateOverCellularTarget() fails. That would be fine also, although maybe a little bit redundant, I think it's easier just to handle the result in SetUpdateStatus() (which could easily be modified to handle an error status with an empty message appropriately).
On 2017/05/12 20:50:16, stevenjb wrote: > lgtm as-is or with more explicit handling of the new error case in > SetUpdateStatus(). > > https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/settings/about_handler.cc (right): > > https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... > chrome/browser/ui/webui/settings/about_handler.cc:585: target_version, > target_size); > On 2017/05/12 20:31:57, weidongg wrote: > > On 2017/05/12 18:28:54, stevenjb wrote: > > > If AboutHandler::SetUpdateStatus never runs then update-status-changed is > > never > > > fired, which I guess is OK, but we should make a note of that in the browser > > > proxy JS. > > > > We could run the callback if SetUpdateOverCellularTarget() fails. > > That would be fine also, although maybe a little bit redundant, I think it's > easier just to handle the result in SetUpdateStatus() (which could easily be > modified to handle an error status with an empty message appropriately). SetUpdateStatus() basically send the |message| set in the callback parameter to js to show on the about page. Do you mean I could set the message to empty string here?
On 2017/05/12 21:17:30, weidongg wrote: > On 2017/05/12 20:50:16, stevenjb wrote: > > lgtm as-is or with more explicit handling of the new error case in > > SetUpdateStatus(). > > > > > https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... > > File chrome/browser/ui/webui/settings/about_handler.cc (right): > > > > > https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... > > chrome/browser/ui/webui/settings/about_handler.cc:585: target_version, > > target_size); > > On 2017/05/12 20:31:57, weidongg wrote: > > > On 2017/05/12 18:28:54, stevenjb wrote: > > > > If AboutHandler::SetUpdateStatus never runs then update-status-changed is > > > never > > > > fired, which I guess is OK, but we should make a note of that in the > browser > > > > proxy JS. > > > > > > We could run the callback if SetUpdateOverCellularTarget() fails. > > > > That would be fine also, although maybe a little bit redundant, I think it's > > easier just to handle the result in SetUpdateStatus() (which could easily be > > modified to handle an error status with an empty message appropriately). > SetUpdateStatus() basically send the |message| set in the callback parameter to > js to show on the about page. Do you mean I could set the message to empty > string here? I'm suggesting that you could send a specific status + error message in the callback and then explicitly either send an event to the JS that it knows how to handle or not send an event at all with a comment describing why. I just think it is better to do that here than to make the decision not to invoke the callback in the lower level code.
On 2017/05/12 21:33:26, stevenjb wrote: > On 2017/05/12 21:17:30, weidongg wrote: > > On 2017/05/12 20:50:16, stevenjb wrote: > > > lgtm as-is or with more explicit handling of the new error case in > > > SetUpdateStatus(). > > > > > > > > > https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... > > > File chrome/browser/ui/webui/settings/about_handler.cc (right): > > > > > > > > > https://codereview.chromium.org/2873193002/diff/100001/chrome/browser/ui/webu... > > > chrome/browser/ui/webui/settings/about_handler.cc:585: target_version, > > > target_size); > > > On 2017/05/12 20:31:57, weidongg wrote: > > > > On 2017/05/12 18:28:54, stevenjb wrote: > > > > > If AboutHandler::SetUpdateStatus never runs then update-status-changed > is > > > > never > > > > > fired, which I guess is OK, but we should make a note of that in the > > browser > > > > > proxy JS. > > > > > > > > We could run the callback if SetUpdateOverCellularTarget() fails. > > > > > > That would be fine also, although maybe a little bit redundant, I think it's > > > easier just to handle the result in SetUpdateStatus() (which could easily be > > > modified to handle an error status with an empty message appropriately). > > SetUpdateStatus() basically send the |message| set in the callback parameter > to > > js to show on the about page. Do you mean I could set the message to empty > > string here? > > I'm suggesting that you could send a specific status + error message in the > callback and then explicitly either send an event to the JS that it knows how to > handle or not send an event at all with a comment describing why. I just think > it is better to do that here than to make the decision not to invoke the > callback in the lower level code. Yes, I agree. Right now I am sending empty message to SetUpdateStatus() with state FAILED, perhaps I need a new state like FAILED_SETTING_UPDATE_OVER_CELLULAR_TARGET to separately handle the case.
still lgtm
The CQ bit was checked by weidongg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by weidongg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by weidongg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2873193002/#ps160001 (title: "Put code in CHROME_OS wrapper to fix trybot error")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494792497885940,
"parent_rev": "95f66c414c5815481766f77c915356c93636dc39", "commit_rev":
"f472642fd6c20b63b0f1a2f62564f2aad56f4be3"}
Message was sent while issue was closed.
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/2881473002/ - Dependent on chromeos CL: https://chromium-review.googlesource.com/#/c/479467/ https://chromium-review.googlesource.com/#/c/481102/ BUG=chromium:691108 TEST=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== 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/2881473002/ - Dependent on chromeos CL: https://chromium-review.googlesource.com/#/c/479467/ https://chromium-review.googlesource.com/#/c/481102/ BUG=chromium:691108 TEST=None CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2873193002 Cr-Commit-Position: refs/heads/master@{#471639} Committed: https://chromium.googlesource.com/chromium/src/+/f472642fd6c20b63b0f1a2f62564... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/f472642fd6c20b63b0f1a2f62564... |
