Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(30)

Issue 2706423002: Use GridLayout for DialogClientView's button row. (Closed)

Created:
3 years, 10 months ago by tapted
Modified:
3 years, 9 months ago
Reviewers:
Bret, Peter Kasting
CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use GridLayout for DialogClientView's button row. This allows the button sizes to be linked, which is desired for Harmony. Also GridLayout allows a lot of bespoke arithmetic to be removed from DialogClientView. It also fixes some obscure glitches that can be triggered in dialog_example.cc (e.g. failing to propagate GetMinimumSize() for resizable dialogs, and failing to reliably incorporate contents_view() height when showing non-bubble dialogs). Add a test for this: DialogClientViewTest.MinMaxPreferredSize. This would previously fail, but it wasn't terrible since dialogs with content views that really care about their size constraints tend to be WebUI which don't have buttons anyway (so ClientView's defaults worked OK). BUG=671820 Review-Url: https://codereview.chromium.org/2706423002 Cr-Commit-Position: refs/heads/master@{#454823} Committed: https://chromium.googlesource.com/chromium/src/+/cc8ff78f49b8fe8265a15ed227ec273c08dd695a

Patch Set 1 : Rebase on crrev/2705553002 #

Patch Set 2 : clearer logic #

Patch Set 3 : Rebase, fix GetMinimumSize #

Patch Set 4 : Fix overloaded virtual #

Patch Set 5 : Overhaul! Use a subview. So nice. #

Patch Set 6 : Add a juicy test #

Total comments: 62

Patch Set 7 : respond to comments #

Patch Set 8 : Rollback delete extra_view_ experiment #

Total comments: 14

Patch Set 9 : respond to comments #

Patch Set 10 : rebase for r452937 #

Total comments: 16

Patch Set 11 : Add DialogClientViewTest.LinkedWidths #

Total comments: 9

Patch Set 12 : respond to unittest comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+405 lines, -254 lines) Patch
M ui/views/examples/dialog_example.cc View 1 2 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M ui/views/window/dialog_client_view.h View 1 2 3 4 5 6 7 8 4 chunks +27 lines, -23 lines 0 comments Download
M ui/views/window/dialog_client_view.cc View 1 2 3 4 5 6 7 8 9 10 8 chunks +222 lines, -209 lines 0 comments Download
M ui/views/window/dialog_client_view_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 9 chunks +152 lines, -22 lines 0 comments Download

Messages

Total messages: 88 (69 generated)
tapted
Hi Peter, please take a look. Notes - I had an alternative approach in Patchset ...
3 years, 10 months ago (2017-02-24 06:09:47 UTC) #36
tapted
https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog_client_view.cc#oldcode222 ui/views/window/dialog_client_view.cc:222: DCHECK(child == contents_view() || child == ok_button_ || Ah, ...
3 years, 10 months ago (2017-02-24 06:55:10 UTC) #39
Peter Kasting
https://codereview.chromium.org/2706423002/diff/240001/ui/views/examples/dialog_example.cc File ui/views/examples/dialog_example.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/examples/dialog_example.cc#newcode273 ui/views/examples/dialog_example.cc:273: ResizeDialog(); I'm vaguely surprised this is necessary, and doesn't ...
3 years, 10 months ago (2017-02-25 06:04:08 UTC) #40
tapted
https://codereview.chromium.org/2706423002/diff/240001/ui/views/examples/dialog_example.cc File ui/views/examples/dialog_example.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/examples/dialog_example.cc#newcode273 ui/views/examples/dialog_example.cc:273: ResizeDialog(); On 2017/02/25 06:04:07, Peter Kasting wrote: > I'm ...
3 years, 9 months ago (2017-02-27 10:04:20 UTC) #52
Peter Kasting
https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog_client_view.cc#newcode64 ui/views/window/dialog_client_view.cc:64: parent()->ChildPreferredSizeChanged(child); On 2017/02/27 10:04:19, tapted wrote: > On 2017/02/25 ...
3 years, 9 months ago (2017-02-28 03:33:24 UTC) #53
tapted
It's time for a rebase (might not get to that tonight), but everything else addressed. ...
3 years, 9 months ago (2017-02-28 06:52:43 UTC) #58
Peter Kasting
https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog_client_view.cc#newcode400 ui/views/window/dialog_client_view.cc:400: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { On 2017/02/28 06:52:42, tapted wrote: > ...
3 years, 9 months ago (2017-02-28 09:30:46 UTC) #59
Bret
LGTM. Peter asked me to be a second set of eyes on this. This is ...
3 years, 9 months ago (2017-03-01 00:29:50 UTC) #65
Peter Kasting
https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc#newcode257 ui/views/window/dialog_client_view.cc:257: if ((delegate->GetDialogButtons() & type) == 0) { On 2017/03/01 ...
3 years, 9 months ago (2017-03-01 00:35:31 UTC) #66
Bret
https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc#newcode257 ui/views/window/dialog_client_view.cc:257: if ((delegate->GetDialogButtons() & type) == 0) { On 2017/03/01 ...
3 years, 9 months ago (2017-03-01 00:39:33 UTC) #67
Peter Kasting
LGTM https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc#newcode154 ui/views/window/dialog_client_view.cc:154: // Note not all constraints can be met. ...
3 years, 9 months ago (2017-03-01 01:52:35 UTC) #68
tapted
PTAL (added a test) https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc#newcode154 ui/views/window/dialog_client_view.cc:154: // Note not all constraints ...
3 years, 9 months ago (2017-03-01 10:44:58 UTC) #73
tapted
On 2017/03/01 10:44:58, tapted wrote: > PTAL (added a test) (also note that to retain ...
3 years, 9 months ago (2017-03-01 10:48:53 UTC) #74
Bret
On 2017/03/01 10:44:58, tapted wrote: > PTAL (added a test) Nice test, still lgtm.
3 years, 9 months ago (2017-03-02 01:57:57 UTC) #75
Peter Kasting
LGTM https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog_client_view.cc#newcode315 ui/views/window/dialog_client_view.cc:315: return {{first, second, third}}; On 2017/03/01 10:44:58, tapted ...
3 years, 9 months ago (2017-03-02 23:09:35 UTC) #76
tapted
https://codereview.chromium.org/2706423002/diff/360001/ui/views/window/dialog_client_view_unittest.cc File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2706423002/diff/360001/ui/views/window/dialog_client_view_unittest.cc#newcode73 ui/views/window/dialog_client_view_unittest.cc:73: return DialogDelegate::GetDialogButtonLabel(button); On 2017/03/02 23:09:34, Peter Kasting wrote: > ...
3 years, 9 months ago (2017-03-06 03:51:49 UTC) #79
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2706423002/380001
3 years, 9 months ago (2017-03-06 03:59:21 UTC) #84
commit-bot: I haz the power
Committed patchset #12 (id:380001) as https://chromium.googlesource.com/chromium/src/+/cc8ff78f49b8fe8265a15ed227ec273c08dd695a
3 years, 9 months ago (2017-03-06 04:04:31 UTC) #87
Peter Kasting
3 years, 9 months ago (2017-03-06 07:36:27 UTC) #88
Message was sent while issue was closed.
https://codereview.chromium.org/2706423002/diff/360001/ui/views/window/dialog...
File ui/views/window/dialog_client_view_unittest.cc (right):

https://codereview.chromium.org/2706423002/diff/360001/ui/views/window/dialog...
ui/views/window/dialog_client_view_unittest.cc:397: delete extra_button;
On 2017/03/06 03:51:48, tapted wrote:
> I think |delete view;| is the accepted pattern among views code to do that
task
> (although it did make me wonder about it when I first saw it).

FWIW, I find the pattern that views code takes ownership sometimes, but not
always, and lets other people remove views by deleting pointers (as well as by
calling RemoveChildView), confusing and dangerous.

See also https://bugs.chromium.org/p/chromium/issues/detail?id=648382 .

Powered by Google App Engine
This is Rietveld 408576698