Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1226093005:
Give ProfileChooserView signin button focus. (Closed)
DescriptionGive ProfileChooserView signin button focus.
When the profile chooser bubble first opens, the signin button should
have focus.
BUG=449088
Committed: https://crrev.com/11a1f5c08e8171ada6420bead547745d44fb8a82
Cr-Commit-Position: refs/heads/master@{#338705}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Override WidgetDelegate::GetInitiallyFocusedView(). #
Total comments: 10
Patch Set 3 : Address comments. #
Depends on Patchset: Messages
Total messages: 22 (7 generated)
wjmaclean@chromium.org changed reviewers: + thakis@chromium.org
Small CL, please take a look?
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226093005/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thakis@chromium.org changed reviewers: + msw@chromium.org
rerouting to a more nested owner
https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:516: profile_bubble_->signin_current_profile_link_->RequestFocus(); Make ProfileChooserView override WidgetDelegate's GetInitiallyFocusedView()? https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:191: // crbug.com/449088 nit: I don't think this comment is helpful, ditto to crbug.com/502370
I addressed your comments as best I could ... ptal? https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:516: profile_bubble_->signin_current_profile_link_->RequestFocus(); On 2015/07/13 20:06:22, msw wrote: > Make ProfileChooserView override WidgetDelegate's GetInitiallyFocusedView()? Done. https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:191: // crbug.com/449088 On 2015/07/13 20:06:22, msw wrote: > nit: I don't think this comment is helpful, ditto to crbug.com/502370 While I address the other comment, can you elaborate on what you would *like* to see? Or do you just want the comment deleted. Since the test is basically one line long, I had though it was somewhat self-documenting, but I can provide further details.
https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1226093005/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:191: // crbug.com/449088 On 2015/07/13 21:11:40, wjmaclean wrote: > On 2015/07/13 20:06:22, msw wrote: > > nit: I don't think this comment is helpful, ditto to crbug.com/502370 > > While I address the other comment, can you elaborate on what you would *like* to > see? Or do you just want the comment deleted. Since the test is basically one > line long, I had though it was somewhat self-documenting, but I can provide > further details. The test name and code are sufficiently straightforward and self-explanatory of the desired behavior; there isn't a need to point to a bug or even comment here. Generally, comments should describe non-obvious behavior, and crbug links are good to reference works in progress or complex/nuanced decisions in bug reports. These tests are simple and the bugs are fixed, just remove these. https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:511: profile_bubble_->initially_focused_view_ = Why is this controlled here? Let ProfileChooserView::GetInitiallyFocusedView return |signin_current_profile_link_| directly and eliminate |initially_focused_view_|. (ProfileChooserView knows the |view_mode| and is a more appropriate user of its |signin_current_profile_link_| member) https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:512: (view_mode == profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER) I don't think there's a need for this conditional behavior; if |ProfileChooserView::signin_current_profile_link_| is null, then this is effectively the same thing, and I think that the behavior may work even if it isn't null and the view isn't part of the current hierarchy. https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:515: profile_bubble_->GetWidget()->set_focus_on_creation(true); Why is this needed? The button should be focused on Show() below without this. https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.h (right): https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.h:100: views::View* GetInitiallyFocusedView() override; nit: merge this with the BubbleDelegateView overrides (or feel free to split those by their constituent subclasses) https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:172: DCHECK(ProfileChooserView::profile_bubble_); nit: this isn't needed.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Ptal. https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:511: profile_bubble_->initially_focused_view_ = On 2015/07/13 21:37:30, msw wrote: > Why is this controlled here? Let ProfileChooserView::GetInitiallyFocusedView > return |signin_current_profile_link_| directly and eliminate > |initially_focused_view_|. (ProfileChooserView knows the |view_mode| and is a > more appropriate user of its |signin_current_profile_link_| member) Done. https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:512: (view_mode == profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER) On 2015/07/13 21:37:30, msw wrote: > I don't think there's a need for this conditional behavior; if > |ProfileChooserView::signin_current_profile_link_| is null, then this is > effectively the same thing, and I think that the behavior may work even if it > isn't null and the view isn't part of the current hierarchy. Done. https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:515: profile_bubble_->GetWidget()->set_focus_on_creation(true); On 2015/07/13 21:37:30, msw wrote: > Why is this needed? The button should be focused on Show() below without this. Done. https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.h (right): https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.h:100: views::View* GetInitiallyFocusedView() override; On 2015/07/13 21:37:30, msw wrote: > nit: merge this with the BubbleDelegateView overrides (or feel free to split > those by their constituent subclasses) Done. https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc (right): https://codereview.chromium.org/1226093005/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view_browsertest.cc:172: DCHECK(ProfileChooserView::profile_bubble_); On 2015/07/13 21:37:30, msw wrote: > nit: this isn't needed. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226093005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks
On 2015/07/14 16:57:58, msw wrote: > lgtm, thanks Np, thanks for the feedback - it really improved the CL!
The CQ bit was checked by wjmaclean@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1226093005/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/11a1f5c08e8171ada6420bead547745d44fb8a82 Cr-Commit-Position: refs/heads/master@{#338705} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
