Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 months ago by tapted
Modified:
7 months, 2 weeks 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 #

Messages

Total messages: 88 (69 generated)
tapted
Hi Peter, please take a look. Notes - I had an alternative approach in Patchset ...
7 months, 3 weeks 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, ...
7 months, 3 weeks 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 ...
7 months, 3 weeks 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 ...
7 months, 3 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 ...
7 months, 3 weeks 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. ...
7 months, 3 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: > ...
7 months, 3 weeks 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 ...
7 months, 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 ...
7 months, 3 weeks 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 ...
7 months, 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. ...
7 months, 3 weeks 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 ...
7 months, 3 weeks 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 ...
7 months, 3 weeks 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.
7 months, 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 ...
7 months, 3 weeks 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: > ...
7 months, 2 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
7 months, 2 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
7 months, 2 weeks ago (2017-03-06 04:04:31 UTC) #87
Peter Kasting
7 months, 2 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 81bcdb8aa