Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2896533002:
cros: Simple password view for lock. Adds test support. (Closed)
Descriptioncros: Simple password view for lock. Adds test support.
This CL primarily adds test support code; the password view is still
WIP.
BUG=719015
Review-Url: https://codereview.chromium.org/2896533002
Cr-Commit-Position: refs/heads/master@{#478758}
Committed: https://chromium.googlesource.com/chromium/src/+/d979ce246d8afa995c88357c2a5288702e6d032f
Patch Set 1 : Initial upload #
Total comments: 24
Patch Set 2 : Rebase #Patch Set 3 : Address comments #Patch Set 4 : Address comment #
Total comments: 14
Patch Set 5 : Address comments #
Depends on Patchset: Messages
Total messages: 57 (47 generated)
The CQ bit was checked by jdufault@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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_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 jdufault@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 jdufault@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_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 jdufault@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.
Description was changed from ========== cros: Simple password view for lock. Adds test support. This CL primarily adds test support code; the password view is still WIP. BUG= ========== to ========== cros: Simple password view for lock. Adds test support. This CL primarily adds test support code; the password view is still WIP. BUG=719015 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
jdufault@chromium.org changed reviewers: + jamescook@chromium.org, xiyuan@chromium.org
jamescook@, xiyuan@, PTAL. The whole point of this CL is to establish a test fixture (LoginTestBase) that all/most of the login views tests will derive from.
https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... File ash/login/ui/login_password_view.cc (right): https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:24: const int kSubmitButtonWidthDp = 20; nit: const -> constexpr ? https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:76: textfield_->SetLayoutManager(new views::FillLayout()); Should this be set to |textfield_sizer | instead? https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:127: SubmitPassword(); nit: DCHCKE_EQ(sender, submit_button_) https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:134: textfield_->SchedulePaint(); SchedulePaint() is probably not needed since SetText should trigger a paint. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... File ash/login/ui/login_password_view_test.cc (right): https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:11: #include "ash/test/ash_test_base.h" nit: unused? https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:13: #include "ui/aura/window.h" nit: unused? https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:15: #include "ui/views/view.h" nit: unused? https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:16: #include "ui/views/widget/widget.h" nit: unused? https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:41: LoginPasswordView* view = nullptr; Put member vars in protected. And view -> view_, password -> password_. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_test... File ash/login/ui/login_test_base.cc (right): https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_test... ash/login/ui/login_test_base.cc:30: views::View* initially_focused_; nit: IMHO, it is easier to read if this is called |content_|. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_test... ash/login/ui/login_test_base.cc:57: fake_user_manager_ = base::MakeUnique<user_manager::FakeUserManager>(); I don't think we will have a UserManager running inside ash. Everything ash knows about user/session should go through SessionController.
The CQ bit was checked by jdufault@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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by jdufault@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...
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Patchset #3 (id:130001) has been deleted
The CQ bit was checked by jdufault@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/2896533002/diff/60001/ash/login/ui/login_pass... File ash/login/ui/login_password_view.cc (right): https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:24: const int kSubmitButtonWidthDp = 20; On 2017/05/24 20:11:02, xiyuan wrote: > nit: const -> constexpr ? Done. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:76: textfield_->SetLayoutManager(new views::FillLayout()); On 2017/05/24 20:11:02, xiyuan wrote: > Should this be set to |textfield_sizer | instead? textfield_sizer has a SizeRangeLayout set on it already. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:127: SubmitPassword(); On 2017/05/24 20:11:02, xiyuan wrote: > nit: DCHCKE_EQ(sender, submit_button_) Done. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:134: textfield_->SchedulePaint(); On 2017/05/24 20:11:02, xiyuan wrote: > SchedulePaint() is probably not needed since SetText should trigger a paint. Done. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... File ash/login/ui/login_password_view_test.cc (right): https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:11: #include "ash/test/ash_test_base.h" On 2017/05/24 20:11:02, xiyuan wrote: > nit: unused? Done. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:13: #include "ui/aura/window.h" On 2017/05/24 20:11:02, xiyuan wrote: > nit: unused? Done. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:15: #include "ui/views/view.h" On 2017/05/24 20:11:02, xiyuan wrote: > nit: unused? Done. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:16: #include "ui/views/widget/widget.h" On 2017/05/24 20:11:02, xiyuan wrote: > nit: unused? Done. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view_test.cc:41: LoginPasswordView* view = nullptr; On 2017/05/24 20:11:02, xiyuan wrote: > Put member vars in protected. And view -> view_, password -> password_. Done. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_test... File ash/login/ui/login_test_base.cc (right): https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_test... ash/login/ui/login_test_base.cc:30: views::View* initially_focused_; On 2017/05/24 20:11:02, xiyuan wrote: > nit: IMHO, it is easier to read if this is called |content_|. Done. https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_test... ash/login/ui/login_test_base.cc:57: fake_user_manager_ = base::MakeUnique<user_manager::FakeUserManager>(); On 2017/05/24 20:11:02, xiyuan wrote: > I don't think we will have a UserManager running inside ash. Everything ash > knows about user/session should go through SessionController. Done.
https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... File ash/login/ui/login_password_view.cc (right): https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:76: textfield_->SetLayoutManager(new views::FillLayout()); On 2017/06/07 18:09:13, jdufault wrote: > On 2017/05/24 20:11:02, xiyuan wrote: > > Should this be set to |textfield_sizer | instead? > > textfield_sizer has a SizeRangeLayout set on it already. Can you elaboarate why |textfield_| needs a layout manager? It seems to me that we should not mess how |textfield_| layouts its children (if any).
The CQ bit was checked by jdufault@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/2896533002/diff/60001/ash/login/ui/login_pass... File ash/login/ui/login_password_view.cc (right): https://codereview.chromium.org/2896533002/diff/60001/ash/login/ui/login_pass... ash/login/ui/login_password_view.cc:76: textfield_->SetLayoutManager(new views::FillLayout()); On 2017/06/07 18:19:59, xiyuan wrote: > On 2017/06/07 18:09:13, jdufault wrote: > > On 2017/05/24 20:11:02, xiyuan wrote: > > > Should this be set to |textfield_sizer | instead? > > > > textfield_sizer has a SizeRangeLayout set on it already. > > Can you elaboarate why |textfield_| needs a layout manager? It seems to me that > we should not mess how |textfield_| layouts its children (if any). Looks like the |textfield_| layout manager is not needed so I've removed it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
LGTM with nits https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... File ash/login/ui/login_password_view.cc (right): https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.cc:23: constexpr int kSubmitButtonWidthDp = 20; optional: Pixel constants are generally assumed to be Dp everywhere, so you could remove the Dp suffix. Up to you. https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.cc:59: { Do you need a new scope here? https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... File ash/login/ui/login_password_view.h (right): https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.h:20: class ASH_EXPORT LoginPasswordView : public views::View, class documentation please. optional: Little ascii-art picture of what this looks like? https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.h:36: using OnPasswordSubmit = base::Callback<void(const base::string16& password)>; Can this be OnceCallback? https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.h:38: explicit LoginPasswordView(const OnPasswordSubmit& on_submit); Document if on_submit is allowed to be null. If not, DCHECK(on_submit_) in constructor. https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... File ash/login/ui/login_password_view_test.cc (right): https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view_test.cc:15: namespace { Hooray for everything in anonymous namespace! https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view_test.cc:108: } // namespace ash nit: blank line above Also, nice tests!
https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... File ash/login/ui/login_password_view.cc (right): https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.cc:23: constexpr int kSubmitButtonWidthDp = 20; On 2017/06/08 17:26:31, James Cook wrote: > optional: Pixel constants are generally assumed to be Dp everywhere, so you > could remove the Dp suffix. Up to you. I've been confused in the past by the unit type so I strongly prefer having a Dp suffix. https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.cc:59: { On 2017/06/08 17:26:31, James Cook wrote: > Do you need a new scope here? Done. https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... File ash/login/ui/login_password_view.h (right): https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.h:20: class ASH_EXPORT LoginPasswordView : public views::View, On 2017/06/08 17:26:31, James Cook wrote: > class documentation please. > > optional: Little ascii-art picture of what this looks like? Done. https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.h:36: using OnPasswordSubmit = base::Callback<void(const base::string16& password)>; On 2017/06/08 17:26:31, James Cook wrote: > Can this be OnceCallback? Changed to RepeatingCallback. It is expected this will be called many times (ie, every time the user hits enter). https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view.h:38: explicit LoginPasswordView(const OnPasswordSubmit& on_submit); On 2017/06/08 17:26:31, James Cook wrote: > Document if on_submit is allowed to be null. If not, DCHECK(on_submit_) in > constructor. Done. https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... File ash/login/ui/login_password_view_test.cc (right): https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view_test.cc:15: namespace { On 2017/06/08 17:26:31, James Cook wrote: > Hooray for everything in anonymous namespace! Acknowledged. https://codereview.chromium.org/2896533002/diff/170001/ash/login/ui/login_pas... ash/login/ui/login_password_view_test.cc:108: } // namespace ash On 2017/06/08 17:26:31, James Cook wrote: > nit: blank line above > > Also, nice tests! Done.
The CQ bit was checked by jdufault@chromium.org to run a CQ dry run
The CQ bit was checked by jdufault@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, jamescook@chromium.org Link to the patchset: https://codereview.chromium.org/2896533002/#ps190001 (title: "Address comments")
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.
The CQ bit was checked by jdufault@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": 190001, "attempt_start_ts": 1497294907702040,
"parent_rev": "bb9c0e1b5b591255889c90a97788c1bf78993453", "commit_rev":
"d979ce246d8afa995c88357c2a5288702e6d032f"}
Message was sent while issue was closed.
Description was changed from ========== cros: Simple password view for lock. Adds test support. This CL primarily adds test support code; the password view is still WIP. BUG=719015 ========== to ========== cros: Simple password view for lock. Adds test support. This CL primarily adds test support code; the password view is still WIP. BUG=719015 Review-Url: https://codereview.chromium.org/2896533002 Cr-Commit-Position: refs/heads/master@{#478758} Committed: https://chromium.googlesource.com/chromium/src/+/d979ce246d8afa995c88357c2a52... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:190001) as https://chromium.googlesource.com/chromium/src/+/d979ce246d8afa995c88357c2a52... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
