Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2949363002:
[CrOS Tether] Network detail settings page: Do not show irrelevant fields for Tether networks. (Closed)
|
Created:
3 years, 6 months ago by Kyle Horimoto Modified:
3 years, 6 months ago Reviewers:
stevenjb CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org, arv+watch_chromium.org, Jeremy Klein, Ryan Hansberry, lesliewatkins, jmann, James Hawkins Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[CrOS Tether] Network detail settings page: Do not show irrelevant fields for Tether networks.
Previously, the Advanced, Networks, and Proxy fields were shown, which are not related to Tether networks.
BUG=672263, 730246
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2949363002
Cr-Commit-Position: refs/heads/master@{#481782}
Committed: https://chromium.googlesource.com/chromium/src/+/5930ec69a322a83322e9a6976a5c9fc791429c4a
Patch Set 1 #
Total comments: 8
Patch Set 2 : stevenjb@ comments. #Patch Set 3 : Fix Closure error. #Messages
Total messages: 16 (8 generated)
Description was changed from ========== [CrOS Tether] Network detail settings page: Do not show irrelevant fields for Tether networks. Previously, the Advanced, Networks, and Proxy fields were shown, which are not related to Tether networks. BUG=672263,730246 ========== to ========== [CrOS Tether] Network detail settings page: Do not show irrelevant fields for Tether networks. Previously, the Advanced, Networks, and Proxy fields were shown, which are not related to Tether networks. BUG=672263,730246 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
khorimoto@chromium.org changed reviewers: + stevenjb@chromium.org
I'm pretty sure we don't want to hide Proxy for Tether networks, plus see other comments. https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/internet_page/internet_detail_page.js (right): https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.js:889: /** @type {!Array<string>} */ var fields = []; +if (type != CrOnc.Type.TETHER) https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.js:932: return false; Modify getAdvancedFields_ instead (see above) https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.js:968: networkProperties.Type == CrOnc.Type.TETHER) { This seems incorrect to me. Connected Tether networks still have IP config and nameservers, especially since we are hiding the underlying wifi network; we should extract that information from the wifi network instead. https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.js:973: return this.isRememberedOrConnected_(networkProperties); This logic may need to change though, we probably don't want to support configuring the IP address of an unconnected Tether network. This could be: if (type == tether) return this.isConnectedState_()
https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/internet_page/internet_detail_page.js (right): https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.js:889: /** @type {!Array<string>} */ var fields = []; On 2017/06/22 22:29:19, stevenjb wrote: > +if (type != CrOnc.Type.TETHER) Done. https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.js:932: return false; On 2017/06/22 22:29:19, stevenjb wrote: > Modify getAdvancedFields_ instead (see above) That's not good enough. When this network is connected, isRememberedOrConnected_() still returns true and the section is displayed. https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.js:968: networkProperties.Type == CrOnc.Type.TETHER) { On 2017/06/22 22:29:19, stevenjb wrote: > This seems incorrect to me. Connected Tether networks still have IP config and > nameservers, especially since we are hiding the underlying wifi network; we > should extract that information from the wifi network instead. As discussed offline, UX says we should not display this for now. Added a comment for clarity. Maybe we can come back to this in v2 and display the Wi-Fi network properties instead. https://codereview.chromium.org/2949363002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/internet_page/internet_detail_page.js:973: return this.isRememberedOrConnected_(networkProperties); On 2017/06/22 22:29:19, stevenjb wrote: > This logic may need to change though, we probably don't want to support > configuring the IP address of an unconnected Tether network. This could be: > if (type == tether) > return this.isConnectedState_() Acknowledged.
lgtm
The CQ bit was checked by khorimoto@chromium.org
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
Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
The CQ bit was checked by khorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2949363002/#ps40001 (title: "Fix Closure 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": 40001, "attempt_start_ts": 1498178693731040,
"parent_rev": "5cbbc0943d80dbd7ba1e89694f921d61ff12e3ab", "commit_rev":
"5930ec69a322a83322e9a6976a5c9fc791429c4a"}
Message was sent while issue was closed.
Description was changed from ========== [CrOS Tether] Network detail settings page: Do not show irrelevant fields for Tether networks. Previously, the Advanced, Networks, and Proxy fields were shown, which are not related to Tether networks. BUG=672263,730246 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CrOS Tether] Network detail settings page: Do not show irrelevant fields for Tether networks. Previously, the Advanced, Networks, and Proxy fields were shown, which are not related to Tether networks. BUG=672263,730246 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2949363002 Cr-Commit-Position: refs/heads/master@{#481782} Committed: https://chromium.googlesource.com/chromium/src/+/5930ec69a322a83322e9a6976a5c... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5930ec69a322a83322e9a6976a5c... |
