Chromium Code Reviews
Help | Chromium Project | Sign in
(1)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months ago by tapted (OOO until 2017-05-01)
Modified:
1 month, 3 weeks ago
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 #

Messages

Total messages: 88 (69 generated)
tapted (OOO until 2017-05-01)
Hi Peter, please take a look. Notes - I had an alternative approach in Patchset ...
2 months ago (2017-02-24 06:09:47 UTC) #36
tapted (OOO until 2017-05-01)
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, ...
2 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 ...
2 months ago (2017-02-25 06:04:08 UTC) #40
tapted (OOO until 2017-05-01)
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 ...
1 month, 4 weeks 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 ...
1 month, 4 weeks ago (2017-02-28 03:33:24 UTC) #53
tapted (OOO until 2017-05-01)
It's time for a rebase (might not get to that tonight), but everything else addressed. ...
1 month, 4 weeks 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: > ...
1 month, 4 weeks ago (2017-02-28 09:30:46 UTC) #59
Bret Sepulveda
LGTM. Peter asked me to be a second set of eyes on this. This is ...
1 month, 3 weeks 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 ...
1 month, 3 weeks ago (2017-03-01 00:35:31 UTC) #66
Bret Sepulveda
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 ...
1 month, 3 weeks 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. ...
1 month, 3 weeks ago (2017-03-01 01:52:35 UTC) #68
tapted (OOO until 2017-05-01)
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 ...
1 month, 3 weeks ago (2017-03-01 10:44:58 UTC) #73
tapted (OOO until 2017-05-01)
On 2017/03/01 10:44:58, tapted wrote: > PTAL (added a test) (also note that to retain ...
1 month, 3 weeks ago (2017-03-01 10:48:53 UTC) #74
Bret Sepulveda
On 2017/03/01 10:44:58, tapted wrote: > PTAL (added a test) Nice test, still lgtm.
1 month, 3 weeks 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 ...
1 month, 3 weeks ago (2017-03-02 23:09:35 UTC) #76
tapted (OOO until 2017-05-01)
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: > ...
1 month, 3 weeks 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
1 month, 3 weeks 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
1 month, 3 weeks ago (2017-03-06 04:04:31 UTC) #87
Peter Kasting
1 month, 3 weeks 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 .
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cc6ac46