Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2659973003:
[Harmony] Harmony Chrome OS Wifi dialog. (Closed)
Description[Harmony] Harmony Chrome OS Wifi dialog.
BUG=680381
Review-Url: https://codereview.chromium.org/2659973003
Cr-Commit-Position: refs/heads/master@{#447278}
Committed: https://chromium.googlesource.com/chromium/src/+/f121dba52f1410c0303990f943459dc5bf0011c2
Patch Set 1 #
Total comments: 5
Patch Set 2 : Address oshima@'s comments. #
Total comments: 6
Patch Set 3 : Address ellyjones@'s comments. #
Messages
Total messages: 27 (11 generated)
xdai@chromium.org changed reviewers: + ellyjones@chromium.org, xiyuan@chromium.org
xiyuan@, ellyjones@, could you help review this CL please? Thanks!
oshima@chromium.org changed reviewers: + oshima@chromium.org
drive by https://codereview.chromium.org/2659973003/diff/1/chrome/browser/chromeos/opt... File chrome/browser/chromeos/options/network_config_view.cc (right): https://codereview.chromium.org/2659973003/diff/1/chrome/browser/chromeos/opt... chrome/browser/chromeos/options/network_config_view.cc:245: size.set_height(std::max(size.height(), predefined_size.height())); size.SetToMax https://codereview.chromium.org/2659973003/diff/1/chrome/browser/chromeos/opt... chrome/browser/chromeos/options/network_config_view.cc:252: horiz_padding / 2, vert_padding / 2); ClampToCenteredSize
Patchset #2 (id:20001) has been deleted
oshima@, I've addressed your comments. Please take another look, thanks for your review! https://codereview.chromium.org/2659973003/diff/1/chrome/browser/chromeos/opt... File chrome/browser/chromeos/options/network_config_view.cc (right): https://codereview.chromium.org/2659973003/diff/1/chrome/browser/chromeos/opt... chrome/browser/chromeos/options/network_config_view.cc:245: size.set_height(std::max(size.height(), predefined_size.height())); On 2017/01/27 22:09:18, oshima wrote: > size.SetToMax Done. https://codereview.chromium.org/2659973003/diff/1/chrome/browser/chromeos/opt... chrome/browser/chromeos/options/network_config_view.cc:252: horiz_padding / 2, vert_padding / 2); On 2017/01/27 22:09:18, oshima wrote: > ClampToCenteredSize This function only shrinks to the smaller size, thus can't be used here.
https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/options/DEPS (right): https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/options/DEPS:4: "!chrome/browser/ui/views/harmony/layout_delegate.h", Would "gn check" be happy about this?
The CQ bit was checked by xdai@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...
https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/options/wifi_config_view.cc (right): https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/options/wifi_config_view.cc:952: const int kLableMinWidth = 150; Lable -> Label https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/options/wifi_config_view.cc:970: column_set->AddPaddingColumn( this column wasn't here before - is it meant to be added?
my bits lgtm, but please wait for xiyuan@'s and ellyjones@'s review. https://codereview.chromium.org/2659973003/diff/1/chrome/browser/chromeos/opt... File chrome/browser/chromeos/options/network_config_view.cc (right): https://codereview.chromium.org/2659973003/diff/1/chrome/browser/chromeos/opt... chrome/browser/chromeos/options/network_config_view.cc:252: horiz_padding / 2, vert_padding / 2); On 2017/01/27 22:25:17, xdai1 wrote: > On 2017/01/27 22:09:18, oshima wrote: > > ClampToCenteredSize > > This function only shrinks to the smaller size, thus can't be used here. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xiyuan@, ellyjones@, please take another look at this CL, thanks! https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/options/DEPS (right): https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/options/DEPS:4: "!chrome/browser/ui/views/harmony/layout_delegate.h", On 2017/01/30 16:59:10, xiyuan wrote: > Would "gn check" be happy about this? The dry run passed. So I guess it's happy? ellyjones@, do you know why these files harmony/layout_delgate.h/cc were put in chrome/browser/ui/views directory？ https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/options/wifi_config_view.cc (right): https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/options/wifi_config_view.cc:952: const int kLableMinWidth = 150; On 2017/01/30 18:06:40, Elly Fong-Jones wrote: > Lable -> Label Done. https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/options/wifi_config_view.cc:970: column_set->AddPaddingColumn( On 2017/01/30 18:06:41, Elly Fong-Jones wrote: > this column wasn't here before - is it meant to be added? Yes. See this mock https://improv.googleplex.com/projects/ahtzfmdvb2dsZS5jb206aW1wcm92LXNlcnZpY2... (Note: this isn't the final mock since we had some modifications discussed in the offline email, like the vertical spacing between rows was adjusted to 8dp following the default Harmony spec). Since it doesn't specify exactly how much space it should occupy behind the password visible button, I just put the default horizontal spacing here.
lgtm then since gn does not complain about deps
On 2017/01/30 18:46:37, xdai1 wrote: > xiyuan@, ellyjones@, please take another look at this CL, thanks! > > https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/options/DEPS (right): > > https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/options/DEPS:4: > "!chrome/browser/ui/views/harmony/layout_delegate.h", > On 2017/01/30 16:59:10, xiyuan wrote: > > Would "gn check" be happy about this? > > The dry run passed. So I guess it's happy? > ellyjones@, do you know why these files harmony/layout_delgate.h/cc were put in > chrome/browser/ui/views directory？ Yes - because LayoutDelegate was only needed in c/b/ui/views. If it's needed somewhere else, we could move it, but you could also just include it out of c/b/ui/views. I think that's logically the right place for it. > https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/options/wifi_config_view.cc (right): > > https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/options/wifi_config_view.cc:952: const int > kLableMinWidth = 150; > On 2017/01/30 18:06:40, Elly Fong-Jones wrote: > > Lable -> Label > > Done. > > https://codereview.chromium.org/2659973003/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/options/wifi_config_view.cc:970: > column_set->AddPaddingColumn( > On 2017/01/30 18:06:41, Elly Fong-Jones wrote: > > this column wasn't here before - is it meant to be added? > > Yes. See this mock > https://improv.googleplex.com/projects/ahtzfmdvb2dsZS5jb206aW1wcm92LXNlcnZpY2... > > (Note: this isn't the final mock since we had some modifications discussed in > the offline email, like the vertical spacing between rows was adjusted to 8dp > following the default Harmony spec). Since it doesn't specify exactly how much > space it should occupy behind the password visible button, I just put the > default horizontal spacing here.
On 2017/01/30 18:54:54, Elly Fong-Jones wrote: > Yes - because LayoutDelegate was only needed in c/b/ui/views. If it's needed > somewhere else, we could move it, but you could also just include it out of > c/b/ui/views. I think that's logically the right place for it. > If it's in c/b/ui/views, the files in c/b/ can't include these layout_delegate.h/cc files since it's excluded in the DEPS: https://cs.chromium.org/chromium/src/chrome/browser/DEPS?rcl=0&l=71. Since many files in c/b/chromeos/ might need these layout_delegate.h/cc for Harmony project, any possibility that these files can be moved to another place?
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org Link to the patchset: https://codereview.chromium.org/2659973003/#ps60001 (title: "Address ellyjones@'s comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/30 22:44:06, xdai1 wrote: > On 2017/01/30 18:54:54, Elly Fong-Jones wrote: > > Yes - because LayoutDelegate was only needed in c/b/ui/views. If it's needed > > somewhere else, we could move it, but you could also just include it out of > > c/b/ui/views. I think that's logically the right place for it. > > > > If it's in c/b/ui/views, the files in c/b/ can't include these > layout_delegate.h/cc files since it's excluded in the DEPS: > https://cs.chromium.org/chromium/src/chrome/browser/DEPS?rcl=0&l=71. Since many > files in c/b/chromeos/ might need these layout_delegate.h/cc for Harmony > project, any possibility that these files can be moved to another place? ellyjones@, let's talk offline to see if we can move these files to another place.
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485885612503540,
"parent_rev": "345632b5cbddaa65124eb98bce6fa933fa211c35", "commit_rev":
"f121dba52f1410c0303990f943459dc5bf0011c2"}
Message was sent while issue was closed.
Description was changed from ========== [Harmony] Harmony Chrome OS Wifi dialog. BUG=680381 ========== to ========== [Harmony] Harmony Chrome OS Wifi dialog. BUG=680381 Review-Url: https://codereview.chromium.org/2659973003 Cr-Commit-Position: refs/heads/master@{#447278} Committed: https://chromium.googlesource.com/chromium/src/+/f121dba52f1410c0303990f94345... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f121dba52f1410c0303990f94345...
Message was sent while issue was closed.
Drive-by. When landing changes to Harmonize a dialog, consider posting screenshots on the associated bug. This can make it easier to tell whether things are correct. There's a lot of nitpicky issues that can be hard to spot. In the case of this CL, the code as checked-in has problems with the order of params in the code added in WifiConfigView::Init(). I'll send a patch separately to address where we can ensure it does the right thing.
Message was sent while issue was closed.
On 2017/01/31 22:20:27, Peter Kasting wrote: > Drive-by. When landing changes to Harmonize a dialog, consider posting > screenshots on the associated bug. This can make it easier to tell whether > things are correct. There's a lot of nitpicky issues that can be hard to spot. > > In the case of this CL, the code as checked-in has problems with the order of > params in the code added in WifiConfigView::Init(). I'll send a patch > separately to address where we can ensure it does the right thing. Thanks! Will do it for the future CLs.
Message was sent while issue was closed.
On 2017/01/31 18:01:43, xdai1 wrote: > On 2017/01/30 22:44:06, xdai1 wrote: > > On 2017/01/30 18:54:54, Elly Fong-Jones wrote: > > > Yes - because LayoutDelegate was only needed in c/b/ui/views. If it's needed > > > somewhere else, we could move it, but you could also just include it out of > > > c/b/ui/views. I think that's logically the right place for it. > > > > > > > If it's in c/b/ui/views, the files in c/b/ can't include these > > layout_delegate.h/cc files since it's excluded in the DEPS: > > https://cs.chromium.org/chromium/src/chrome/browser/DEPS?rcl=0&l=71. Since > many > > files in c/b/chromeos/ might need these layout_delegate.h/cc for Harmony > > project, any possibility that these files can be moved to another place? > > ellyjones@, let's talk offline to see if we can move these files to another > place. I think https://bugs.chromium.org/p/chromium/issues/detail?id=687340 and https://bugs.chromium.org/p/chromium/issues/detail?id=687349 together should address this. If you need more discussions here, please keep me in the loop. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
