Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2755173002:
[CrOS Tether] Add a connection dialog to md-settings. (Closed)
|
Created:
3 years, 9 months ago by Kyle Horimoto Modified:
3 years, 9 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-closure_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, vitalyp+closure_chromium.org, dbeam+watch-settings_chromium.org, srahim+watch_chromium.org, stevenjb+watch-md-settings_chromium.org, jlklein+watch-closure_chromium.org, Ryan Hansberry Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[CrOS Tether] Add a connection dialog to md-settings.
The dialog is currently unused, and it will be hooked up to the rest of the page in a later CL.
BUG=672263
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2755173002
Cr-Commit-Position: refs/heads/master@{#458139}
Committed: https://chromium.googlesource.com/chromium/src/+/86c3ba045bd9c7c3dfa92e010b91e2daff8e8ca6
Patch Set 1 #Patch Set 2 : Add comments. #
Total comments: 10
Patch Set 3 : stevenjb@ comment. #
Total comments: 2
Patch Set 4 : stevenjb@ comment. #Patch Set 5 : Fix closure compiler error. #
Total comments: 6
Patch Set 6 : dbeam@ comments. #
Total comments: 8
Patch Set 7 : dbeam@ comments. #
Total comments: 2
Patch Set 8 : dbeam@ comments. #
Messages
Total messages: 34 (15 generated)
Description was changed from ========== [CrOS Tether] Add a connection dialog to md-settings. The dialog is currently unused, and it will be hooked up to the rest of the page in a later CL. BUG=672263 ========== to ========== [CrOS Tether] Add a connection dialog to md-settings. The dialog is currently unused, and it will be hooked up to the rest of the page in a later CL. BUG=672263 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
khorimoto@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.js (right): https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.js:20: CrScrollableBehavior, You shouldn't need this behavior?
https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.js (right): https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.js:20: CrScrollableBehavior, On 2017/03/17 20:43:54, stevenjb wrote: > You shouldn't need this behavior? Done.
lgtm https://codereview.chromium.org/2755173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.js (right): https://codereview.chromium.org/2755173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.js:19: I18nBehavior, Also, minor nit: if you remove the , here clang will put this one one line.
https://codereview.chromium.org/2755173002/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.js (right): https://codereview.chromium.org/2755173002/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.js:19: I18nBehavior, On 2017/03/17 20:50:44, stevenjb wrote: > Also, minor nit: if you remove the , here clang will put this one one line. Done.
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/2755173002/#ps60001 (title: "stevenjb@ comment.")
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/2755173002/#ps80001 (title: "Fix closure compiler error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.html (right): https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:22: --iron-icon-fill-color: var(--google-blue-500); indent https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:59: tetherData_.tetherNostDeviceName)]] indent https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:67: <template is="dom-if" if="[[tetherData_.isTetherHostCurrentlyOnWifi"> you didn't close this ]] https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.js (right): https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.js:41: this.getDialog_().showModal(); note: showModal throws if the dialog is already open
The CQ bit was unchecked by dbeam@chromium.org
https://codereview.chromium.org/2755173002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.html (right): https://codereview.chromium.org/2755173002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:14: @apply(--layout-vertical); why are you using this rather than just raw CSS? https://codereview.chromium.org/2755173002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:26: @apply(--layout-vertical); why not combine with .body? https://codereview.chromium.org/2755173002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:68: <li>$i18n{tetherConnectionDescriptionWiFi}</li> nit: for simple stuff, just bind to hidden="[[tetherData_.isTetherHostCurrentlyOnWifi]]" instead of dom-if
https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.html (right): https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:22: --iron-icon-fill-color: var(--google-blue-500); On 2017/03/17 21:50:04, Dan Beam wrote: > indent Done. https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:59: tetherData_.tetherNostDeviceName)]] On 2017/03/17 21:50:05, Dan Beam wrote: > indent Done. https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:67: <template is="dom-if" if="[[tetherData_.isTetherHostCurrentlyOnWifi"> On 2017/03/17 21:50:04, Dan Beam wrote: > you didn't close this ]] Done. https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.js (right): https://codereview.chromium.org/2755173002/diff/20001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.js:41: this.getDialog_().showModal(); On 2017/03/17 21:50:05, Dan Beam wrote: > note: showModal throws if the dialog is already open Done. https://codereview.chromium.org/2755173002/diff/80001/chrome/browser/resource... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.html (right): https://codereview.chromium.org/2755173002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:14: @apply(--layout-vertical); On 2017/03/17 21:51:39, Dan Beam wrote: > why are you using this rather than just raw CSS? Done. https://codereview.chromium.org/2755173002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:26: @apply(--layout-vertical); On 2017/03/17 21:51:39, Dan Beam wrote: > why not combine with .body? Done. https://codereview.chromium.org/2755173002/diff/80001/chrome/browser/resource... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:68: <li>$i18n{tetherConnectionDescriptionWiFi}</li> On 2017/03/17 21:51:38, Dan Beam wrote: > nit: for simple stuff, just bind to > hidden="[[tetherData_.isTetherHostCurrentlyOnWifi]]" instead of dom-if Done.
https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.html (right): https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:2: <link rel="import" href="chrome://resources/html/cr.html"> not needed https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:4: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html"> still needed? https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icons/device-icons.html"> don't use iron-icons from polymer, use an icons.html in either settings or ui/webui https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:22: flex-direction: row; this is the default, you might not need it
https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.html (right): https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:2: <link rel="import" href="chrome://resources/html/cr.html"> On 2017/03/17 22:21:30, Dan Beam wrote: > not needed Done. https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:4: <link rel="import" href="chrome://resources/polymer/v1_0/iron-flex-layout/iron-flex-layout.html"> On 2017/03/17 22:21:30, Dan Beam wrote: > still needed? Done. https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:6: <link rel="import" href="chrome://resources/polymer/v1_0/iron-icons/device-icons.html"> On 2017/03/17 22:21:30, Dan Beam wrote: > don't use iron-icons from polymer, use an icons.html in either settings or > ui/webui Done. https://codereview.chromium.org/2755173002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:22: flex-direction: row; On 2017/03/17 22:21:30, Dan Beam wrote: > this is the default, you might not need it Done.
lgtm https://codereview.chromium.org/2755173002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.html (right): https://codereview.chromium.org/2755173002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:66: </template> </template> -> </li>
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, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2755173002/#ps140001 (title: "dbeam@ comments.")
https://codereview.chromium.org/2755173002/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/internet_page/tether_connection_dialog.html (right): https://codereview.chromium.org/2755173002/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/internet_page/tether_connection_dialog.html:66: </template> On 2017/03/17 22:55:16, Dan Beam wrote: > </template> -> </li> Done.
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490029306677330,
"parent_rev": "9e0fd9a87d69362103af512b8e7b8946604abe71", "commit_rev":
"86c3ba045bd9c7c3dfa92e010b91e2daff8e8ca6"}
Message was sent while issue was closed.
Description was changed from ========== [CrOS Tether] Add a connection dialog to md-settings. The dialog is currently unused, and it will be hooked up to the rest of the page in a later CL. BUG=672263 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CrOS Tether] Add a connection dialog to md-settings. The dialog is currently unused, and it will be hooked up to the rest of the page in a later CL. BUG=672263 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2755173002 Cr-Commit-Position: refs/heads/master@{#458139} Committed: https://chromium.googlesource.com/chromium/src/+/86c3ba045bd9c7c3dfa92e010b91... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/86c3ba045bd9c7c3dfa92e010b91... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
