|
|
Created:
3 years, 7 months ago by weidongg Modified:
3 years, 7 months ago CC:
chromium-reviews, alemate+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, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow proper message in about Chrome OS page
- Background:
Updates over cellular is blocked by Chrome UI by default, but we now
allow so if user is actively checking for updates (e.g clicking 'check
for updates' button). But auto-updates over cellular is still not
supported.
- Changes in this CL:
Add a |interactive| parameter to IsUpdateOverCellularAllowed(), which
indicates whether user is actively checking for updates.
Show proper warning message when user is over cellular connection.
- Design doc: go/cros-cellular-updates
BUG=chromium:691108
TEST=go/about_chrome_os_err_msg for screenshots of different messages
shown in different situations.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2897773002
Cr-Commit-Position: refs/heads/master@{#473953}
Committed: https://chromium.googlesource.com/chromium/src/+/a02f7daa0abeca53945fe248847d955cae1a064a
Patch Set 1 #
Total comments: 8
Patch Set 2 : Apply fix to patch set 1 #Patch Set 3 : Prevent updated status when dialog is shown #
Total comments: 12
Patch Set 4 : Apply fix to patch set 3 #
Messages
Total messages: 27 (13 generated)
Description was changed from ========== Show proper message in about Chrome OS page - Background: Updates over cellular is blocked by Chrome UI by default, but we now allow so if user is actively checking for updates (e.g clicking 'check for updates' button). But auto-updates over cellular is still not supported. - Changes in this CL: Add a |interactive| parameter to IsUpdateOverCellularAllowed(), which indicates whether user is actively checking for updates. Show proper warning message when user is over cellular connection. - Design doc: go/cros-cellular-updates BUG=chromium:691108 TEST=go/about_chrome_os_err_msg for screenshots of different messages shown in different situations. ========== to ========== Show proper message in about Chrome OS page - Background: Updates over cellular is blocked by Chrome UI by default, but we now allow so if user is actively checking for updates (e.g clicking 'check for updates' button). But auto-updates over cellular is still not supported. - Changes in this CL: Add a |interactive| parameter to IsUpdateOverCellularAllowed(), which indicates whether user is actively checking for updates. Show proper warning message when user is over cellular connection. - Design doc: go/cros-cellular-updates BUG=chromium:691108 TEST=go/about_chrome_os_err_msg for screenshots of different messages shown in different situations. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
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...
weidongg@chromium.org changed reviewers: + stevenjb@chromium.org, xiyuan@chromium.org
xiyuan@chromium.org, stevenjb@chromium.org: Please review changes.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:181: this.hasCheckedForUpdates_ = false; Should this be moved to onUpdateWarningDialogClose_ ? https://codereview.chromium.org/2897773002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2897773002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/about_handler.cc:106: bool cellular = network && network->IsConnectedState() && nit: const bool
https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:181: this.hasCheckedForUpdates_ = false; On 2017/05/22 18:14:08, xiyuan wrote: > Should this be moved to onUpdateWarningDialogClose_ ? There will be a tiny issue if I put it there: although the update warning dialog is shown above the page, the page shows 'your chromebook is up to date' in the background (See https://screenshot.googleplex.com/Ts7vdSHHKYO). Then page turns that message off the dialog closes. The reason is that both currentUpdateStatusEvent_ and hasCheckedForUpdates_ trigger the message toggling, currentUpdateStatusEvent_ turns the message on here after receiving UPDATED and hasCheckedForUpdates_ turns the message off when dialog closes. https://codereview.chromium.org/2897773002/diff/1/chrome/browser/ui/webui/set... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2897773002/diff/1/chrome/browser/ui/webui/set... chrome/browser/ui/webui/settings/about_handler.cc:106: bool cellular = network && network->IsConnectedState() && On 2017/05/22 18:14:08, xiyuan wrote: > nit: const bool Done.
https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:181: this.hasCheckedForUpdates_ = false; On 2017/05/22 19:02:54, weidongg wrote: > On 2017/05/22 18:14:08, xiyuan wrote: > > Should this be moved to onUpdateWarningDialogClose_ ? > > There will be a tiny issue if I put it there: although the update > warning dialog is shown above the page, the page shows 'your chromebook is up to > date' in the background (See https://screenshot.googleplex.com/Ts7vdSHHKYO). > Then page turns that message off the dialog closes. > The reason is that both currentUpdateStatusEvent_ and hasCheckedForUpdates_ > trigger the message toggling, currentUpdateStatusEvent_ turns the message on > here after receiving UPDATED and hasCheckedForUpdates_ turns the message off > when dialog closes. Why would the page gets an UPDATED status while the dialog is open?
https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:181: this.hasCheckedForUpdates_ = false; On 2017/05/22 19:28:03, xiyuan wrote: > On 2017/05/22 19:02:54, weidongg wrote: > > On 2017/05/22 18:14:08, xiyuan wrote: > > > Should this be moved to onUpdateWarningDialogClose_ ? > > > > There will be a tiny issue if I put it there: although the update > > warning dialog is shown above the page, the page shows 'your chromebook is up > to > > date' in the background (See https://screenshot.googleplex.com/Ts7vdSHHKYO). > > Then page turns that message off the dialog closes. > > The reason is that both currentUpdateStatusEvent_ and hasCheckedForUpdates_ > > trigger the message toggling, currentUpdateStatusEvent_ turns the message on > > here after receiving UPDATED and hasCheckedForUpdates_ turns the message off > > when dialog closes. > > Why would the page gets an UPDATED status while the dialog is open? The state transition for update engine is IDLE->CHECKING_FOR_UPDATE->NEED_PERMISSION_TO_UPDATE->REPORTING_ERROR_EVENT->IDLE, but JS maps IDLE to UPDATED state and version updater[1] blocks the error from showing to user by map error state to UPDATED. So for JS the state transition will be UPDATED->CHECKING->NEED_PERMISSION_TO_UPDATE->UPDATED->UPDATED. [1]: https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/help/version_upd...
https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:181: this.hasCheckedForUpdates_ = false; On 2017/05/22 19:38:03, weidongg wrote: > On 2017/05/22 19:28:03, xiyuan wrote: > > On 2017/05/22 19:02:54, weidongg wrote: > > > On 2017/05/22 18:14:08, xiyuan wrote: > > > > Should this be moved to onUpdateWarningDialogClose_ ? > > > > > > There will be a tiny issue if I put it there: although the update > > > warning dialog is shown above the page, the page shows 'your chromebook is > up > > to > > > date' in the background (See https://screenshot.googleplex.com/Ts7vdSHHKYO). > > > Then page turns that message off the dialog closes. > > > The reason is that both currentUpdateStatusEvent_ and hasCheckedForUpdates_ > > > trigger the message toggling, currentUpdateStatusEvent_ turns the message on > > > here after receiving UPDATED and hasCheckedForUpdates_ turns the message off > > > when dialog closes. > > > > Why would the page gets an UPDATED status while the dialog is open? > > The state transition for update engine is > IDLE->CHECKING_FOR_UPDATE->NEED_PERMISSION_TO_UPDATE->REPORTING_ERROR_EVENT->IDLE, > but JS maps IDLE to UPDATED state and version updater[1] blocks the error from > showing to user by map error state to UPDATED. So for JS the state transition > will be UPDATED->CHECKING->NEED_PERMISSION_TO_UPDATE->UPDATED->UPDATED. > > [1]: > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/help/version_upd... Can we use fix this? Either not sending UPDATED on C++ side (at lest for need-permission-to-update case). Or don't show the UPDATED message when update warning dialog is shown. Setting |this.hasCheckedForUpdates_| feels like a work around and easy to be broken later.
https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2897773002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/about_page/about_page.js:181: this.hasCheckedForUpdates_ = false; On 2017/05/22 20:10:28, xiyuan wrote: > On 2017/05/22 19:38:03, weidongg wrote: > > On 2017/05/22 19:28:03, xiyuan wrote: > > > On 2017/05/22 19:02:54, weidongg wrote: > > > > On 2017/05/22 18:14:08, xiyuan wrote: > > > > > Should this be moved to onUpdateWarningDialogClose_ ? > > > > > > > > There will be a tiny issue if I put it there: although the update > > > > warning dialog is shown above the page, the page shows 'your chromebook is > > up > > > to > > > > date' in the background (See > https://screenshot.googleplex.com/Ts7vdSHHKYO). > > > > Then page turns that message off the dialog closes. > > > > The reason is that both currentUpdateStatusEvent_ and > hasCheckedForUpdates_ > > > > trigger the message toggling, currentUpdateStatusEvent_ turns the message > on > > > > here after receiving UPDATED and hasCheckedForUpdates_ turns the message > off > > > > when dialog closes. > > > > > > Why would the page gets an UPDATED status while the dialog is open? > > > > The state transition for update engine is > > > IDLE->CHECKING_FOR_UPDATE->NEED_PERMISSION_TO_UPDATE->REPORTING_ERROR_EVENT->IDLE, > > but JS maps IDLE to UPDATED state and version updater[1] blocks the error from > > showing to user by map error state to UPDATED. So for JS the state transition > > will be UPDATED->CHECKING->NEED_PERMISSION_TO_UPDATE->UPDATED->UPDATED. > > > > [1]: > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/help/version_upd... > > Can we use fix this? Either not sending UPDATED on C++ side (at lest for > need-permission-to-update case). Or don't show the UPDATED message when update > warning dialog is shown. > > Setting |this.hasCheckedForUpdates_| feels like a work around and easy to be > broken later. Sure, I uploaded a new patch set to prevent updated status being shown to user when the dialog is shown.
https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:232: // dialog is shown. nit: can you combine the new comment with old one? e.g. // Do not show the "updated" status if we haven't checked yet // or the update warning dialog is shown to user. https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:436: this.hasCheckedForUpdates_ = false; This runs when user clicks "Continue" on the dialog as well, right? Do we need to worry that user clicks "Check for Update" before our SetUpdateOverCellularTarget finishes in "Continue" case?
https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_utils_chromeos.h (right): https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_utils_chromeos.h:18: // up to the update engine. This comment confused me until I examined the code. How about something like: Returns true if updates over cellular networks are allowed. If |interactive| is true (e.g. the user clicked on a 'check for updates' button), updates over cellular are allowed unless prohibited by policy. If |interactive| is false, updates over cellular may be allowed by default for the device type, or by policy. https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:55: // one used in update engine. The comment doesn't align very well with the logic here. Maybe just: // Update Engine prohibits updates over Bluetooth (That said, we don't actually support Bluetooth network connections and probably never will, so we could just remove this line.) https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:60: // policy is not set. I think this comment just confuses things; we should rely on the comment for IsUpdateOverCellularAllowed() instead. https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:112: IDS_UPGRADE_NETWORK_LIST_CELLULAR_NOT_RECOMMENDED) The name of this string is confusing. Maybe IDS_UPGRADE_NETWORK_LIST_CELLULAR_ALLOWED_NOT_AUTOMATIC ?
https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/about_page/about_page.js (right): https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:232: // dialog is shown. On 2017/05/22 22:36:34, xiyuan wrote: > nit: can you combine the new comment with old one? > > e.g. > > // Do not show the "updated" status if we haven't checked yet > // or the update warning dialog is shown to user. Done. https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/about_page/about_page.js:436: this.hasCheckedForUpdates_ = false; On 2017/05/22 22:36:34, xiyuan wrote: > This runs when user clicks "Continue" on the dialog as well, right? Do we need > to worry that user clicks "Check for Update" before our > SetUpdateOverCellularTarget finishes in "Continue" case? Yes, you are right. This runs whenever the dialog closes. The button disappear when transitioning to CHECKING state (I tested in samus, I didn't notice it appears in the first place.). If user manages to click the button before transitioning to CHECKING state, I think there will be two AttempUpdate dbus calls and one of them will get ignored in Update engine. https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/help_utils_chromeos.h (right): https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/help_utils_chromeos.h:18: // up to the update engine. On 2017/05/22 22:57:32, stevenjb wrote: > This comment confused me until I examined the code. How about something like: > > Returns true if updates over cellular networks are allowed. If |interactive| is > true (e.g. the user clicked on a 'check for updates' button), updates over > cellular are allowed unless prohibited by policy. If |interactive| is false, > updates over cellular may be allowed by default for the device type, or by > policy. > > Sorry for the confusing comment. Thanks, this is much better. https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/help/version_updater_chromeos.cc (right): https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:55: // one used in update engine. On 2017/05/22 22:57:32, stevenjb wrote: > The comment doesn't align very well with the logic here. Maybe just: > // Update Engine prohibits updates over Bluetooth > > (That said, we don't actually support Bluetooth network connections and probably > never will, so we could just remove this line.) Ok, removed. https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/help/version_updater_chromeos.cc:60: // policy is not set. On 2017/05/22 22:57:32, stevenjb wrote: > I think this comment just confuses things; we should rely on the comment for > IsUpdateOverCellularAllowed() instead. Ok, removed https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/settings/about_handler.cc (right): https://codereview.chromium.org/2897773002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/settings/about_handler.cc:112: IDS_UPGRADE_NETWORK_LIST_CELLULAR_NOT_RECOMMENDED) On 2017/05/22 22:57:32, stevenjb wrote: > The name of this string is confusing. Maybe > IDS_UPGRADE_NETWORK_LIST_CELLULAR_ALLOWED_NOT_AUTOMATIC ? Aha, it's better. let's use this.
lgtm
lgtm
The CQ bit was checked by weidongg@chromium.org
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": 60001, "attempt_start_ts": 1495554831588740, "parent_rev": "8cd44d248f4d7439aeb914c5e7ca4b74df94563f", "commit_rev": "a02f7daa0abeca53945fe248847d955cae1a064a"}
Message was sent while issue was closed.
Description was changed from ========== Show proper message in about Chrome OS page - Background: Updates over cellular is blocked by Chrome UI by default, but we now allow so if user is actively checking for updates (e.g clicking 'check for updates' button). But auto-updates over cellular is still not supported. - Changes in this CL: Add a |interactive| parameter to IsUpdateOverCellularAllowed(), which indicates whether user is actively checking for updates. Show proper warning message when user is over cellular connection. - Design doc: go/cros-cellular-updates BUG=chromium:691108 TEST=go/about_chrome_os_err_msg for screenshots of different messages shown in different situations. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Show proper message in about Chrome OS page - Background: Updates over cellular is blocked by Chrome UI by default, but we now allow so if user is actively checking for updates (e.g clicking 'check for updates' button). But auto-updates over cellular is still not supported. - Changes in this CL: Add a |interactive| parameter to IsUpdateOverCellularAllowed(), which indicates whether user is actively checking for updates. Show proper warning message when user is over cellular connection. - Design doc: go/cros-cellular-updates BUG=chromium:691108 TEST=go/about_chrome_os_err_msg for screenshots of different messages shown in different situations. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2897773002 Cr-Commit-Position: refs/heads/master@{#473953} Committed: https://chromium.googlesource.com/chromium/src/+/a02f7daa0abeca53945fe248847d... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/a02f7daa0abeca53945fe248847d... |