Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2705553002:
Cleanups for DialogClientView, DialogClientViewTest. (Closed)
DescriptionCleanups for DialogClientView, DialogClientViewTest.
This is a precursor for http://crrev.com/2706423002 with some orthogonal
stuff to make review there easier.
We want to use a LayoutManager for DialogClientView. This requires DCV
to know about, and manage, all of its direct child views. Start by
consolidating "CreateExtraView()" and other view creation into a
"SetupViews()" method, and add a DCHECK to ensure client code isn't
inserting random views.
But. Currently a test inserts a random view to test focus traversal. To
properly test focus, it needs a FocusManager, but the test harness
doesn't use a Widget, so it doesn't have a FocusManager.
So add a Widget to the test harness. Turns out we don't actually need a
`TestDialogClientView`, or any of the protected methods in
DialogClientView it was using: make them all private.
BUG=671820
Review-Url: https://codereview.chromium.org/2705553002
Cr-Commit-Position: refs/heads/master@{#452321}
Committed: https://chromium.googlesource.com/chromium/src/+/d9c220fe3846b32984b6f4b9ca7c43cd62edd98f
Patch Set 1 #Patch Set 2 : squish examples - clang_format makes this poop #Patch Set 3 : DialogExample - TODO: Move content dep #Patch Set 4 : Cleanup example code before split #Patch Set 5 : Just the cleanups - rest moved to crrev/2706423002 #Patch Set 6 : Friendship, TestDialogClientView not needed #Patch Set 7 : Better comments, clearer diff #
Total comments: 2
Patch Set 8 : Fixes Windows DWM and Linux. Not ChromeOS or Windows RDP. #Patch Set 9 : More sensible checks #
Total comments: 6
Patch Set 10 : Remove unncessary child check #
Dependent Patchsets: Messages
Total messages: 54 (48 generated)
The CQ bit was checked by tapted@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by tapted@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tapted@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tapted@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 #4 (id:60001) has been deleted
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #4 (id:140001) has been deleted
The CQ bit was checked by tapted@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 checked by tapted@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...
Description was changed from ========== Cleanups for DialogClientView, DialogClientViewTest BUG=671820 ========== to ========== Cleanups for DialogClientView, DialogClientViewTest. This is a precursor for http://crrev.com/2706423002 with some orthogonal stuff to make review easier. We want to use a LayoutManager for DialogClientView. This requires DCV to know about, and manage, all of its direct child views. Start by consolidating "CreateExtraView()" and other view creation into a "SetupViews()" method, and add a DCHECK to ensure client code isn't inserting random views. But. Currently a test inserts a random view to test focus traversal. To properly test focus, it needs a FocusManager, but the test harness doesn't use a Widget, so it doesn't have a FocusManager. So add a Widget to the test. Turns out we don't actually need a `TestDialogClientView`, or any of the protected methods in DialogClientView it was using: make them all private. BUG=671820 ==========
Description was changed from ========== Cleanups for DialogClientView, DialogClientViewTest. This is a precursor for http://crrev.com/2706423002 with some orthogonal stuff to make review easier. We want to use a LayoutManager for DialogClientView. This requires DCV to know about, and manage, all of its direct child views. Start by consolidating "CreateExtraView()" and other view creation into a "SetupViews()" method, and add a DCHECK to ensure client code isn't inserting random views. But. Currently a test inserts a random view to test focus traversal. To properly test focus, it needs a FocusManager, but the test harness doesn't use a Widget, so it doesn't have a FocusManager. So add a Widget to the test. Turns out we don't actually need a `TestDialogClientView`, or any of the protected methods in DialogClientView it was using: make them all private. BUG=671820 ========== to ========== Cleanups for DialogClientView, DialogClientViewTest. This is a precursor for http://crrev.com/2706423002 with some orthogonal stuff to make review easier. We want to use a LayoutManager for DialogClientView. This requires DCV to know about, and manage, all of its direct child views. Start by consolidating "CreateExtraView()" and other view creation into a "SetupViews()" method, and add a DCHECK to ensure client code isn't inserting random views. But. Currently a test inserts a random view to test focus traversal. To properly test focus, it needs a FocusManager, but the test harness doesn't use a Widget, so it doesn't have a FocusManager. So add a Widget to the test harness. Turns out we don't actually need a `TestDialogClientView`, or any of the protected methods in DialogClientView it was using: make them all private. BUG=671820 ==========
Patchset #8 (id:240001) has been deleted
Patchset #6 (id:200001) has been deleted
Description was changed from ========== Cleanups for DialogClientView, DialogClientViewTest. This is a precursor for http://crrev.com/2706423002 with some orthogonal stuff to make review easier. We want to use a LayoutManager for DialogClientView. This requires DCV to know about, and manage, all of its direct child views. Start by consolidating "CreateExtraView()" and other view creation into a "SetupViews()" method, and add a DCHECK to ensure client code isn't inserting random views. But. Currently a test inserts a random view to test focus traversal. To properly test focus, it needs a FocusManager, but the test harness doesn't use a Widget, so it doesn't have a FocusManager. So add a Widget to the test harness. Turns out we don't actually need a `TestDialogClientView`, or any of the protected methods in DialogClientView it was using: make them all private. BUG=671820 ========== to ========== Cleanups for DialogClientView, DialogClientViewTest. This is a precursor for http://crrev.com/2706423002 with some orthogonal stuff to make review there easier. We want to use a LayoutManager for DialogClientView. This requires DCV to know about, and manage, all of its direct child views. Start by consolidating "CreateExtraView()" and other view creation into a "SetupViews()" method, and add a DCHECK to ensure client code isn't inserting random views. But. Currently a test inserts a random view to test focus traversal. To properly test focus, it needs a FocusManager, but the test harness doesn't use a Widget, so it doesn't have a FocusManager. So add a Widget to the test harness. Turns out we don't actually need a `TestDialogClientView`, or any of the protected methods in DialogClientView it was using: make them all private. BUG=671820 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
tapted@chromium.org changed reviewers: + sky@chromium.org
Hi sky, please take a look. (Note this was split from http://crrev.com/2706423002 "Use GridLayout in DialogClientView" which might add a bit more context). https://codereview.chromium.org/2705553002/diff/260001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/2705553002/diff/260001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:251: CreateExtraView(); (not needed since UpdateDialogButtons() will invoke it via SetupViews()) https://codereview.chromium.org/2705553002/diff/260001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:311: SetupFocusChain(); CreateExtraView() moves into SetupViews() verbatim except this line is dropped: UpdateDialogButtons() always calls SetupFocusChain() after SetupViews().
The CQ bit was checked by tapted@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: 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 tapted@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_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by tapted@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 #9 (id:300001) has been deleted
hm - some bots didn't like me (see notes below). I think I've found the right fix in patchset 9. https://codereview.chromium.org/2705553002/diff/160002/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2705553002/diff/160002/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:82: EXPECT_EQ(client_bounds.width(), this->width()); Using a Widget means it gets a NonClientFrameView, which encounters some platform-specific code. When a non-native frame gets used, the first two checks above pass, but other two fail because the client view is not at the origin. For the origin, it was always just checking that (0,0) == (0,0) which is not very interesting. Made that explicit, and made it check the width instead of .right(). https://codereview.chromium.org/2705553002/diff/160002/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:241: EXPECT_EQ(gfx::Size(), client_view()->size()); This fails for a similar reason (but also it's never been very interesting since the preferred size is 0x0 since there are no buttons). So now it checks that instead.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM https://codereview.chromium.org/2705553002/diff/160002/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2705553002/diff/160002/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:125: (ok_button_ ? ok_button_->GetPreferredSize().width() : 0) + IMO this code would be more readable if it was a bunch of conditionals, e.g.: int width = 0; if (ok_button_) width += ok_button_->GetPreferredSize().width(); ... It will likely span more lines, but I think it results in easier to read code. I think it makes the last line easier to read too, e.g.: if (ShouldShow(extra_view_)) { width += extra_view_->GetPreferredSize().width + GetExtraViewSpacing(); } https://codereview.chromium.org/2705553002/diff/160002/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:229: } else if (!details.is_add && child != this) { child != this isn't really needed here.
https://codereview.chromium.org/2705553002/diff/160002/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2705553002/diff/160002/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:125: (ok_button_ ? ok_button_->GetPreferredSize().width() : 0) + On 2017/02/22 17:29:48, sky wrote: > IMO this code would be more readable if it was a bunch of conditionals, e.g.: > > int width = 0; > if (ok_button_) > width += ok_button_->GetPreferredSize().width(); > ... > > It will likely span more lines, but I think it results in easier to read code. I > think it makes the last line easier to read too, e.g.: > > if (ShouldShow(extra_view_)) { > width += extra_view_->GetPreferredSize().width + GetExtraViewSpacing(); > } I'm just going to delete it all in https://codereview.chromium.org/2706423002/ - it will be glorious :). (i.e. GridLayout is much better equipped to calculate GetPreferredSize() - `GetExtraViewSpacing` was the only thing I still needed) https://codereview.chromium.org/2705553002/diff/160002/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:229: } else if (!details.is_add && child != this) { On 2017/02/22 17:29:48, sky wrote: > child != this isn't really needed here. Removed (follow-up doesn't need it either)
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2705553002/#ps330001 (title: "Remove unncessary child check")
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": 330001, "attempt_start_ts": 1487805674350300,
"parent_rev": "5bdcd6fff79307e080a87ee417960f2e2141dc24", "commit_rev":
"d9c220fe3846b32984b6f4b9ca7c43cd62edd98f"}
Message was sent while issue was closed.
Description was changed from ========== Cleanups for DialogClientView, DialogClientViewTest. This is a precursor for http://crrev.com/2706423002 with some orthogonal stuff to make review there easier. We want to use a LayoutManager for DialogClientView. This requires DCV to know about, and manage, all of its direct child views. Start by consolidating "CreateExtraView()" and other view creation into a "SetupViews()" method, and add a DCHECK to ensure client code isn't inserting random views. But. Currently a test inserts a random view to test focus traversal. To properly test focus, it needs a FocusManager, but the test harness doesn't use a Widget, so it doesn't have a FocusManager. So add a Widget to the test harness. Turns out we don't actually need a `TestDialogClientView`, or any of the protected methods in DialogClientView it was using: make them all private. BUG=671820 ========== to ========== Cleanups for DialogClientView, DialogClientViewTest. This is a precursor for http://crrev.com/2706423002 with some orthogonal stuff to make review there easier. We want to use a LayoutManager for DialogClientView. This requires DCV to know about, and manage, all of its direct child views. Start by consolidating "CreateExtraView()" and other view creation into a "SetupViews()" method, and add a DCHECK to ensure client code isn't inserting random views. But. Currently a test inserts a random view to test focus traversal. To properly test focus, it needs a FocusManager, but the test harness doesn't use a Widget, so it doesn't have a FocusManager. So add a Widget to the test harness. Turns out we don't actually need a `TestDialogClientView`, or any of the protected methods in DialogClientView it was using: make them all private. BUG=671820 Review-Url: https://codereview.chromium.org/2705553002 Cr-Commit-Position: refs/heads/master@{#452321} Committed: https://chromium.googlesource.com/chromium/src/+/d9c220fe3846b32984b6f4b9ca7c... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:330001) as https://chromium.googlesource.com/chromium/src/+/d9c220fe3846b32984b6f4b9ca7c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
