Supporting updates over mobile connections |
||||||
Issue descriptionThis is a follow up bug to crbug.com/691108 . There are few remaining issues: 1. 'update_over_cellular enable' (An old method to turn on updates over cellular connections, some users still use it.) in crosh directly sets a preference kPrefsUpdateOverCellularPermission in Chrome OS, resulting in async behavior in UI. IMO, we should make this preference default to true since we should enable updates over mobile connections by default. And SetUpdateOverCellularPermission() dbus interface could be called to set the preference to false to disable the updates. 2. Fix to 691108 supports both tethered and cellular connections, but the code and documents are still using keyword 'cellular'. We should refactor the code and documents to using 'mobile' instead of 'cellular' for accuracy. 3. Fix to 691108 adds a state NEED_PERMISSION_TO_UPDATE to state machine of update engine. This makes the state machine more complicated which is not desirable. A better way is to add an error code to already existing state REPORTING_ERROR_EVENT. This requires the update engine also broadcast its error code to UI. 4. Fix to 691108 use a version number and update size (Corner case that a full update immediately comes after a delta update) to uniquely identify an update, but probably a better way is to use hash code of the update metadata. The design doc: go/cros-mobile-updates One pager: go/cros-cellular-updates
,
Jul 6 2017
,
Jul 6 2017
,
Jul 6 2017
,
Jul 7 2017
1. I think kPrefsUpdateOverCellularPermission is fine as it is right now. ==true means "never ask me for updating over cellular"; ==false means "ask me whenever you would update but you didn't because of cellular". This doesn't need to create async behavior in the UI. Think that enterprises may want to force this value to true to ensure its fleet of chromebooks is up to date. 2. "cellular" data means either a modem directly connected to your chromebook or a hotstop from a phone. I think that changing this to "mobile" is a cosmetic change an bit of waste of time. "cellular" is the name shill gives to cellular connections. 3. You propose to add the error code to REPORTING_ERROR_EVENT. That creates the same two states but with another name: (REPORTING_ERROR_EVENT, error_code_that_needs_permission) and (REPORTING_ERROR_EVENT, other_error_code). That's still two states but masked as if it is only one. Adding the error code to the signal is a change in the DBus api which is super annoying to do (need to keep in sync chrome and UE). Chrome is uprev by the builders, UE is uprev by the CQ. Fun times. I wouldn't bother with this change. 4. (version_number, update_size) as an identifier already covers the corner case of delta update followed by full update (full update size >> delta update size; and if that's not the case, hey, what's the problem then?).
,
Jul 7 2017
1. That makes sense, we should keep it as it is. kPrefsUpdateOverCellularPermission is set by UI in OOBE phase before checking for updates if IsCellularFirst switch is on. In About ChromeOS page the message shown is based on checking this switch. But 'update_over_cellular enable' directly set the preference to true in UpdateEngine. So we have an async between this preference and switch state. 2. I thought cellular does not include tethered networks. We could leave it as it is. 3. Adding a state NEED_PERMISSION_TO_UPDATE before REPORTING_ERROR_EVENT makes it hard for UI to handle. Since we already handled it in UI, it not a bug just a little improvement. Considering the time and effort, we could move this to low priority. 4. We could use one parameter instead of two. Since it has already fixed the corner case, we could leave it as it is.
,
Aug 22 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by weidongg@chromium.org
, Jul 6 2017