|
|
DescriptionUse 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)
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 ========== Use GridLayout in DialogClientView. BUG=671820 ========== to ========== Use GridLayout in DialogClientView. 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. BUG=671820 ==========
Description was changed from ========== Use GridLayout in DialogClientView. 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. BUG=671820 ========== to ========== Use GridLayout in DialogClientView. 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). BUG=671820 ==========
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_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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_chromeos_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_...)
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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #4 (id:100001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #3 (id:120001) 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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Patchset #4 (id:160001) has been deleted
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...
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 ========== Use GridLayout in DialogClientView. 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). BUG=671820 ========== to ========== Use GridLayout in DialogClientView. 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's not 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 ==========
tapted@chromium.org changed reviewers: + pkasting@chromium.org
Description was changed from ========== Use GridLayout in DialogClientView. 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's not 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 ========== to ========== Use GridLayout in DialogClientView. 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 ==========
Description was changed from ========== Use GridLayout in DialogClientView. 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 ========== to ========== 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 ==========
Patchset #6 (id:220001) has been deleted
Hi Peter, please take a look. Notes - I had an alternative approach in Patchset 4 that didn't involve |button_container_view_| but LayoutManagers don't have good support for propagating Min/Max sizes - it ended up a mess and I kept finding corner cases. - There's a `TODO` which is to modify GridLayout to support a "maximium" size by which to link columns - that's in http://crrev.com/2625083003 (just needs tests), but this change is much scarier since so much logic is being purged from DialogClientView, so I wanted to focus on this. - Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (left): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:222: DCHECK(child == contents_view() || child == ok_button_ || Ah, this DCHECK I added to get a jump on dialogs that might be inserting views in the wrong place, which would have been an issue if the layout was in |this|, but now that the layout is in a child view it's not really relevant. (so this can stay or go).
https://codereview.chromium.org/2706423002/diff/240001/ui/views/examples/dial... File ui/views/examples/dialog_example.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/examples/dial... ui/views/examples/dialog_example.cc:273: ResizeDialog(); I'm vaguely surprised this is necessary, and doesn't happen automatically during creation via ContentsChanged() or something. (Haven't looked closely, maybe this is reasonable.) https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:44: // Arrange two sizes vertically and return the bounding box. Nit: Descriptive ("Arranges"), not imperative. Maybe "Returns the bounding box required to contain |above| and |below|, placed one atop the other." This also suggests the function should be GetBoundingSizeForVerticalStack() or something similar that implies what it actually returns, and doesn't imply it positions something. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:45: gfx::Size ArrangeVertically(const gfx::Size& above, const gfx::Size& below) { Nit: Trivial, but it technically doesn't matter that one is above and one is below; they could be in the other order. I'd maybe name them |size1| and |size2| :) https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:59: return static_cast<DialogClientView*>(View::parent()); Nit: It would be safer to take the DialogClientView* that parents us as a constructor param and store it. That would prevent anyone from changing layout in a way that makes the ButtonRowContainer a child of a child of the DialogClientView, or something, leading to really weird failures. This would also be useful for my next thought. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:64: parent()->ChildPreferredSizeChanged(child); Nit: Optional, but seems like the only reason we need to bubble these two events is for the parent to do "if (child_ == extra_view) SomethingInvolvingLayout();" I wonder if that functionality shouldn't be in here instead. The idea would be that the parts of layout affecting the non-button-row, which would be our parent's responsibility, wouldn't need to run when this changed; whereas only the layout for this container itself would be affected when something about the extra view changes. Really, the question here is whether we can cleave more functionality out of the parent and put it here instead (basically, almost everything you're adding in this CL). https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:91: delete extra_view_; I'm confused. Visibility and being part of the view hierarachy are not normally in lockstep (invisible views can be part of the hierarchy). Why would we have a non-null extra_view_ that's not a child view? https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:148: // The button layout doesn't currently have a maximum size. Maybe it should? Then we could probably just use the helper here the same way we do above, unless it's important to return 0 when the client view has an empty size but there is a button row. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:149: DCHECK(gfx::Size() == button_row_container_->GetMaximumSize()); Nit: DCHECK(button_row_container_->GetMaximumSize().IsEmpty())? https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:151: if (max_size != gfx::Size()) Nit: !IsEmpty()? https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:191: // references to deleted Views. Is this important? Are there races or something that result in bustage if we don't null these out? It seems suspicious that we need most of this code at all. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:334: insets.Set(top, insets.left(), insets.bottom(), insets.right()); Nit: We should probably just add per-side setters for gfx::Insets... https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:376: // First column is only used if there is an extra view. I feel like with some cleverness this block and the ones below could all be put into a loop and we could eliminate some of the constants above. But I am tired enough not to want to spend time figuring out how to do it in detail. Something about creating an array of bools like {ShouldShow(extra_view_), ok_button_ && cancel_button_, ???} and then just looping through these, setting the link values to the current column in the GridLayout (which I dunno if it even has an accessor for, but maybe it should). https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:400: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { Should we just go ahead and link the column sizes in pre-MD too? Yeah, it's a behavior change, but seems like one we could maybe just make. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:82: // Update the dialog button |member| according to the DialogDelegate. Nit: Updates Updates like how? Do you need to say what |type| is used for? https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:89: // Returns the 0-3 Views in the order they should appear in the button row. Nit: the 0-3 Views -> any button views (Seems best not to explicitly say "0-3" here even though in practice that's true.) https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:90: std::vector<View*> ButtonRowViews(); Nit: GetButtonRowViews()? https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:96: // All buttons are removed from the hierarchy to be added by the layout. Nit: I don't really understand this sentence. Not enough context without explicitly reading the code. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:122: // Prevents ViewHierarchyChanged() from clearing out data members. Nit: "When true, ..." https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:123: bool in_setup_views_ = false; Nit: Seems like this should be named by what we want it do (e.g. |preserve_data_members_|) instead of where it's set. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:111: void SetSizeConstraints(const gfx::Size& min, Nit: Suffix these args with _size https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:310: const gfx::Size buttons = client_view()->GetPreferredSize(); Nit: Use a name like |buttons_size|, since |buttons| alone sounds like a vector. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:315: EXPECT_EQ(gfx::Size(), client_view()->GetMaximumSize()); Nit: IsEmpty()?
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...
Patchset #7 (id:260001) has been deleted
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2706423002/diff/240001/ui/views/examples/dial... File ui/views/examples/dialog_example.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/examples/dial... ui/views/examples/dialog_example.cc:273: ResizeDialog(); On 2017/02/25 06:04:07, Peter Kasting wrote: > I'm vaguely surprised this is necessary, and doesn't happen automatically during > creation via ContentsChanged() or something. (Haven't looked closely, maybe > this is reasonable.) Well this was fun to trace :/. It boils down to a combination of things: 1. RootView::Layout() is called twice during Widget::Init (once before and once after the "initial" bounds are set) 2. In DialogExample, RootView::Layout() eventually asks for views::Label::GetPreferredSize() 3. In the first call to RootView::Layout(), the RootView may have a size of 1x1 (There are cases on Linux and Mac where it's invalid to have a 0x0 window -- see e.g. NativeFrameView::GetWindowBoundsForClientBounds() below) 4. This causes Label to be sized from 0x0 to 1x1 (even though its PreferredSize is bigger) 5. Then RootView::Layout() is called a second time. Label gets asked again what its preferred size is and it now says 0x0: this is because Label::GetPreferredSize() asks gfx::RenderText for "how many lines would fit in 1x1", and RenderText says zero. [If, instead, RenderText is asked "how many lines would fit in 0x0" then it thinks the label wants layout, and so returns the actual lines of text.] 1., 3. and 5. are all kinda bugs. 5. is a known problem Label::GetTextSize/PreferredSize has TODOs and cites crbug.com/468494, crbug.com/467526 3. is hard to fix since it depends on OS constraints 1. looks straightforward to fix, and probably gains us a performance boost by not doing 2 Layouts during Widget::Init -> split this out to https://codereview.chromium.org/2712383002 and removed the `ResizeDialog()` call here. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:44: // Arrange two sizes vertically and return the bounding box. On 2017/02/25 06:04:07, Peter Kasting wrote: > Nit: Descriptive ("Arranges"), not imperative. > > Maybe "Returns the bounding box required to contain |above| and |below|, placed > one atop the other." > > This also suggests the function should be GetBoundingSizeForVerticalStack() or > something similar that implies what it actually returns, and doesn't imply it > positions something. Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:45: gfx::Size ArrangeVertically(const gfx::Size& above, const gfx::Size& below) { On 2017/02/25 06:04:07, Peter Kasting wrote: > Nit: Trivial, but it technically doesn't matter that one is above and one is > below; they could be in the other order. I'd maybe name them |size1| and > |size2| :) Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:59: return static_cast<DialogClientView*>(View::parent()); On 2017/02/25 06:04:07, Peter Kasting wrote: > Nit: It would be safer to take the DialogClientView* that parents us as a > constructor param and store it. That would prevent anyone from changing layout > in a way that makes the ButtonRowContainer a child of a child of the > DialogClientView, or something, leading to really weird failures. > > This would also be useful for my next thought. Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:64: parent()->ChildPreferredSizeChanged(child); On 2017/02/25 06:04:07, Peter Kasting wrote: > Nit: Optional, but seems like the only reason we need to bubble these two events > is for the parent to do "if (child_ == extra_view) SomethingInvolvingLayout();" > > I wonder if that functionality shouldn't be in here instead. The idea would be > that the parts of layout affecting the non-button-row, which would be our > parent's responsibility, wouldn't need to run when this changed; whereas only > the layout for this container itself would be affected when something about the > extra view changes. > > Really, the question here is whether we can cleave more functionality out of the > parent and put it here instead (basically, almost everything you're adding in > this CL). This crossed my mind too.. but I decided not to attempt it because all the other lifetime management is the responsibility of DialogClientView - e.g. DCV provides accessors for ok_button/cancel_button. I'm tempted to try this in a follow-up, but I'm not certain it will improve things :/ https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:91: delete extra_view_; On 2017/02/25 06:04:07, Peter Kasting wrote: > I'm confused. > > Visibility and being part of the view hierarachy are not normally in lockstep > (invisible views can be part of the hierarchy). > > Why would we have a non-null extra_view_ that's not a child view? This is to satisfy some existing use-cases for DialogClientView. GridLayout doesn't consider visibility when doing layout, but the old DCV logic did. Since DCV client code doesn't "own" the ExtraView once created, if the dialog later decides it doesn't really want an ExtraView, it currently makes it hidden. When that happens, the extra view shouldn't affect layout. The alternative to just to plonk the extra view as a child (bypassing the LayoutManager), even though it is hidden doesn't feel right. E.g. it can't actually be added to |button_row_container_| -- the GridLayout will object with `grid_layout.cc(766)] Check failed: host_ == host && adding_view_.` So we could add it as a sibling of |button_row_container_|, but if we're moving it between hierarchies we might as well just move it out.. Haven't come up with a neater solution to this :/ https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:148: // The button layout doesn't currently have a maximum size. On 2017/02/25 06:04:07, Peter Kasting wrote: > Maybe it should? Then we could probably just use the helper here the same way > we do above, unless it's important to return 0 when the client view has an empty > size but there is a button row. Maximum size would need slightly different logic (i.e. std::min instead of std::max). Also there's more possibility for a constraints violation then. E.g. We'd have to think about what happens if the contents_view() minimum width is larger than the button row maximum width. (there is already the possible violation where the contents_view() maximum width is smaller than the button row minimum width: contents_view() currently gets ignored in this case - but it's unlikely to arise: contents_views() that care about this tend to be webview dialogs which don't have native buttons). https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:149: DCHECK(gfx::Size() == button_row_container_->GetMaximumSize()); On 2017/02/25 06:04:07, Peter Kasting wrote: > Nit: DCHECK(button_row_container_->GetMaximumSize().IsEmpty())? The maximum size can be constrained in a single direction by leaving one axis 0 - IsEmpty() would return true in this case, so gfx::Size() gives a stronger check (note DCHECK_EQ didn't work here because ostream). Updated the comment. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:151: if (max_size != gfx::Size()) On 2017/02/25 06:04:07, Peter Kasting wrote: > Nit: !IsEmpty()? Changed this to if (max_size.height() != 0) and added a comment. (the width constraint [or lack of it] should just pass through without affecting things). added a DLOG for the possible constraints violation.. WDYT? https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:191: // references to deleted Views. On 2017/02/25 06:04:07, Peter Kasting wrote: > Is this important? Are there races or something that result in bustage if we > don't null these out? It seems suspicious that we need most of this code at > all. It looks like this trend started a long time ago while fixing http://crbug.com/241451 . That was a use-after-free triggered from a DialogClientView method which no longer exists (OnWillChangeFocus). I think it is safe to remove the lines that clear out ok_button_ and cancel_button_. It's probably OK to remove extra_view_ too -- it's dereferenced a bit more often and DCV has less control over it. E.g. a dialog may decide to `delete extra_view;` -- currently that works (and would even allow a new extra view to be created..) There is a test that would fail for this though. It does: TEST_F(DialogClientViewTest, RemoveAndUpdateButtons) { // Removing buttons from another context should clear the local pointer. SetDialogButtons(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL); delete client_view()->ok_button(); So we would have to decide that that test case is no longer valid - maybe something better for a follow-up. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:334: insets.Set(top, insets.left(), insets.bottom(), insets.right()); On 2017/02/25 06:04:07, Peter Kasting wrote: > Nit: We should probably just add per-side setters for gfx::Insets... I hunted through codesearch, and did some "bruteforce" refactoring (renamed Set to SetAll). But surprisingly.. this seems to be the only caller to Set(..) that wants to change only one side. Other use cases are for mirroring (e.g. for rtl), or often to set a *single* side with the rest all being set explicitly to zero (i.e. when overriding some default Insets). Since this occurrence has a TODO(..) deleteme, it might be a rare enough occurrence to only want to set one side that we would rather callers have to think what they really should do with the other sides as well.. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:376: // First column is only used if there is an extra view. On 2017/02/25 06:04:07, Peter Kasting wrote: > I feel like with some cleverness this block and the ones below could all be put > into a loop and we could eliminate some of the constants above. But I am tired > enough not to want to spend time figuring out how to do it in detail. Something > about creating an array of bools like {ShouldShow(extra_view_), ok_button_ && > cancel_button_, ???} and then just looping through these, setting the link > values to the current column in the GridLayout (which I dunno if it even has an > accessor for, but maybe it should). I'd toyed with this a bit.. The ButtonRowViews() approach was as far as I could go before the code made me unhappy - the logic here is subtle, so it feels good to me to play it out in a sequence. GridLayout doesn't really have any accessors (for _anything_ -> all you can get is a ColumnSet* from an id, and all you can get from a ColumnSet* is its id and the number of columns in it). Exposing a current column could be tricky because there's some subtlety around the skipping of padding columns (e.g. does the skipped column advance at the end of AddView() or at the start of AddView()?) https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:400: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { On 2017/02/25 06:04:08, Peter Kasting wrote: > Should we just go ahead and link the column sizes in pre-MD too? > > Yeah, it's a behavior change, but seems like one we could maybe just make. Hm - that scares me a little. It's not just that some dialogs may get suddenly wide due to long strings. But also some dialogs will become inconsistent with DialogClientView-based dialogs. E.g. collected cookies view manages its own buttons. Also more visible things like the `add bookmark` bubble return `NONE` from DialogDelegate::GetDialogButtons() to do their own management of buttons. (for those it should be a lot simpler though! They are both already using GridLayout - they just need to link columns) https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:82: // Update the dialog button |member| according to the DialogDelegate. On 2017/02/25 06:04:08, Peter Kasting wrote: > Nit: Updates > > Updates like how? > > Do you need to say what |type| is used for? Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:89: // Returns the 0-3 Views in the order they should appear in the button row. On 2017/02/25 06:04:08, Peter Kasting wrote: > Nit: the 0-3 Views -> any button views > > (Seems best not to explicitly say "0-3" here even though in practice that's > true.) Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:90: std::vector<View*> ButtonRowViews(); On 2017/02/25 06:04:08, Peter Kasting wrote: > Nit: GetButtonRowViews()? Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:96: // All buttons are removed from the hierarchy to be added by the layout. On 2017/02/25 06:04:08, Peter Kasting wrote: > Nit: I don't really understand this sentence. Not enough context without > explicitly reading the code. Changed to "After calling this, no button row Views will be in the view hierarchy." https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:122: // Prevents ViewHierarchyChanged() from clearing out data members. On 2017/02/25 06:04:08, Peter Kasting wrote: > Nit: "When true, ..." Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:123: bool in_setup_views_ = false; On 2017/02/25 06:04:08, Peter Kasting wrote: > Nit: Seems like this should be named by what we want it do (e.g. > |preserve_data_members_|) instead of where it's set. Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:111: void SetSizeConstraints(const gfx::Size& min, On 2017/02/25 06:04:08, Peter Kasting wrote: > Nit: Suffix these args with _size Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:310: const gfx::Size buttons = client_view()->GetPreferredSize(); On 2017/02/25 06:04:08, Peter Kasting wrote: > Nit: Use a name like |buttons_size|, since |buttons| alone sounds like a vector. Done. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:315: EXPECT_EQ(gfx::Size(), client_view()->GetMaximumSize()); On 2017/02/25 06:04:08, Peter Kasting wrote: > Nit: IsEmpty()? Same as the other ones - I want to check that both width and height are zero (rather than either). Added a comment. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:348: } // namespace views This is the dubious Layout call: void RootView::SetContentsView(View* contents_view) { DCHECK(contents_view && GetWidget()->native_widget()) << "Can't be called until after the native widget is created!"; // The ContentsView must be set up _after_ the window is created so that its // Widget pointer is valid. SetLayoutManager(new FillLayout); if (has_children()) RemoveAllChildViews(true); AddChildView(contents_view); // Force a layout now, since the attached hierarchy won't be ready for the // containing window's bounds. Note that we call Layout directly rather than // calling the widget's size changed handler, since the RootView's bounds may // not have changed, which will cause the Layout not to be done otherwise. Layout(); } Here's some more cruft around the weird 1x1 contents bounds during init: gfx::Rect NativeFrameView::GetWindowBoundsForClientBounds( const gfx::Rect& client_bounds) const { #if defined(OS_WIN) return views::GetWindowBoundsForClientBounds( static_cast<View*>(const_cast<NativeFrameView*>(this)), client_bounds); #else // Enforce minimum size (1, 1) in case that |client_bounds| is passed with // empty size. gfx::Rect window_bounds = client_bounds; if (window_bounds.IsEmpty()) window_bounds.set_size(gfx::Size(1,1)); return window_bounds; #endif } [since http://crrev.com/271086 ]. that "window_bounds.set_size(gfx::Size(1,1));" causes views::Label in dialog_example.cc to obtain a size of 1x1. When that happens, GetWindowBoundsForClientBounds is called from gfx::Size NonClientView::GetPreferredSize() const { // TODO(pkasting): This should probably be made to look similar to // GetMinimumSize() below. This will require implementing GetPreferredSize() // better in the various frame views. gfx::Rect client_bounds(gfx::Point(), client_view_->GetPreferredSize()); return GetWindowBoundsForClientBounds(client_bounds).size(); }
https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:64: parent()->ChildPreferredSizeChanged(child); On 2017/02/27 10:04:19, tapted wrote: > On 2017/02/25 06:04:07, Peter Kasting wrote: > > Nit: Optional, but seems like the only reason we need to bubble these two > events > > is for the parent to do "if (child_ == extra_view) > SomethingInvolvingLayout();" > > > > I wonder if that functionality shouldn't be in here instead. The idea would > be > > that the parts of layout affecting the non-button-row, which would be our > > parent's responsibility, wouldn't need to run when this changed; whereas only > > the layout for this container itself would be affected when something about > the > > extra view changes. > > > > Really, the question here is whether we can cleave more functionality out of > the > > parent and put it here instead (basically, almost everything you're adding in > > this CL). > > This crossed my mind too.. but I decided not to attempt it because all the other > lifetime management is the responsibility of DialogClientView - e.g. DCV > provides accessors for ok_button/cancel_button. I'm tempted to try this in a > follow-up, but I'm not certain it will improve things :/ I'm fine leaving it to a followup. I think it's worth trying, but it will be a fairly large change. I don't expect there to be less code overall when it's done, but I do think it would be nice for the scope of various overrides to be clearer (DialogClientView itself will be doing less, which will make it easier to reason about what it needs to do). https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:91: delete extra_view_; On 2017/02/27 10:04:19, tapted wrote: > On 2017/02/25 06:04:07, Peter Kasting wrote: > > I'm confused. > > > > Visibility and being part of the view hierarachy are not normally in lockstep > > (invisible views can be part of the hierarchy). > > > > Why would we have a non-null extra_view_ that's not a child view? > > This is to satisfy some existing use-cases for DialogClientView. GridLayout > doesn't consider visibility when doing layout, but the old DCV logic did. Since > DCV client code doesn't "own" the ExtraView once created, if the dialog later > decides it doesn't really want an ExtraView, it currently makes it hidden. Where does this happen? Basically, I'm trying to figure out the control flow in the current code that will allow |extra_view_| to be non-null yet not a child view. I can't find it. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:151: if (max_size != gfx::Size()) On 2017/02/27 10:04:19, tapted wrote: > On 2017/02/25 06:04:07, Peter Kasting wrote: > > Nit: !IsEmpty()? > > Changed this to > > if (max_size.height() != 0) > > and added a comment. (the width constraint [or lack of it] should just pass > through without affecting things). > > added a DLOG for the possible constraints violation.. WDYT? I don't think we should add a DLOG, because it's not clear how you'll collect the data or take action, and also because it makes it unclear how bad this is in practice or whether we even expect it to occur. So if you're really worried this could occur in practice and we need to find it and fix it if so, CHECK, and then file a bug to remove the CHECK if it doesn't find anything for a while. If you think this can't occur in practice and we need to assure readers of that, DCHECK. If it can't occur in practice and readers need not consider the issue, remove this entirely. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:191: // references to deleted Views. On 2017/02/27 10:04:19, tapted wrote: > On 2017/02/25 06:04:07, Peter Kasting wrote: > > Is this important? Are there races or something that result in bustage if we > > don't null these out? It seems suspicious that we need most of this code at > > all. > > It looks like this trend started a long time ago while fixing > http://crbug.com/241451 . That was a use-after-free triggered from a > DialogClientView method which no longer exists (OnWillChangeFocus). > > I think it is safe to remove the lines that clear out ok_button_ and > cancel_button_. It's probably OK to remove extra_view_ too -- it's dereferenced > a bit more often and DCV has less control over it. E.g. a dialog may decide to > `delete extra_view;` -- currently that works (and would even allow a new extra > view to be created..) > > There is a test that would fail for this though. It does: > > TEST_F(DialogClientViewTest, RemoveAndUpdateButtons) { > // Removing buttons from another context should clear the local pointer. > SetDialogButtons(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL); > delete client_view()->ok_button(); > > So we would have to decide that that test case is no longer valid - maybe > something better for a follow-up. Hmm, I didn't realize we had public accessors for these buttons. Given that we do, I don't think we can remove this block entirely, unless we can kill those accessors entirely, at least for non-test code. They're mostly used in test code today, and maybe it would be OK to have them work unintuitively when they're test-only, but DialogDelegate::GetInitiallyFocusedView() uses them, and I think that use is probably both appropriate and mandates that we keep this. |extra_view_| has to be kept too, I think; we can't avoid nulling it out here if we're going to do things like pointer-compare it in ChildPreferredSizeChanged(), lest we want to risk really weird bugs. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:334: insets.Set(top, insets.left(), insets.bottom(), insets.right()); On 2017/02/27 10:04:19, tapted wrote: > On 2017/02/25 06:04:07, Peter Kasting wrote: > > Nit: We should probably just add per-side setters for gfx::Insets... > > I hunted through codesearch, and did some "bruteforce" refactoring (renamed Set > to SetAll). But surprisingly.. this seems to be the only caller to Set(..) that > wants to change only one side. Hmm. I just wanted to set one side the other day, and found a way to go another route instead because this didn't exist :( > Since this occurrence has a TODO(..) deleteme, it might be a rare enough > occurrence to only want to set one side that we would rather callers have to > think what they really should do with the other sides as well.. I'm willing to live without it for now. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:376: // First column is only used if there is an extra view. On 2017/02/27 10:04:19, tapted wrote: > On 2017/02/25 06:04:07, Peter Kasting wrote: > > I feel like with some cleverness this block and the ones below could all be > put > > into a loop and we could eliminate some of the constants above. But I am > tired > > enough not to want to spend time figuring out how to do it in detail. > Something > > about creating an array of bools like {ShouldShow(extra_view_), ok_button_ && > > cancel_button_, ???} and then just looping through these, setting the link > > values to the current column in the GridLayout (which I dunno if it even has > an > > accessor for, but maybe it should). > > I'd toyed with this a bit.. The ButtonRowViews() approach was as far as I could > go before the code made me unhappy - the logic here is subtle, so it feels good > to me to play it out in a sequence. OK. > GridLayout doesn't really have any accessors (for _anything_ -> all you can get > is a ColumnSet* from an id, and all you can get from a ColumnSet* is its id and > the number of columns in it). Exposing a current column could be tricky because > there's some subtlety around the skipping of padding columns (e.g. does the > skipped column advance at the end of AddView() or at the start of AddView()?) Probably moot. But, the "current column" should always point to where the next non-skipped item will be added, rather than wherever the last non-skipped item was added. So, "end", IIUC. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:400: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { On 2017/02/27 10:04:19, tapted wrote: > On 2017/02/25 06:04:08, Peter Kasting wrote: > > Should we just go ahead and link the column sizes in pre-MD too? > > > > Yeah, it's a behavior change, but seems like one we could maybe just make. > > Hm - that scares me a little. It's not just that some dialogs may get suddenly > wide due to long strings. Well, we're only enlarging linked widths when they're "near" in size, right? So hopefully the scope of the enlarging is limited. > But also some dialogs will become inconsistent with > DialogClientView-based dialogs. E.g. collected cookies view manages its own > buttons. I don't know that I follow. Are you saying that this will cause size linking for dialogs that use DialogClientView, but since not every dialog uses DialogClientView, that means we'll have behavioral inconsistency? If so, we have to fix that in Harmony anyway, right? Either by making these actually use DialogClientView (preferable), or manually reimplementing this everywhere that does this (ugh). At which point we can also Just Fix all those places for pre-Harmony. Or not; I'm willing to live with an inconsistency like this for a few releases pre-Harmony, as it's minor enough I doubt any users would even notice. > Also more visible things like the `add bookmark` bubble return `NONE` > from DialogDelegate::GetDialogButtons() to do their own management of buttons. > > (for those it should be a lot simpler though! They are both already using > GridLayout - they just need to link columns) Feel free to file bugs on making these dialogs be "real dialogs that don't manage their own buttons" unless it seems like a bad idea. I'd like to get to a point where all our dialogs are using a single common dialog framework, and then if we change a behavior like this in one place, it changes everywhere. I haven't looked at these dialogs as recently as you so I don't know if maybe there's a good reason why they simply can't do that. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:96: // All buttons are removed from the hierarchy to be added by the layout. On 2017/02/27 10:04:19, tapted wrote: > On 2017/02/25 06:04:08, Peter Kasting wrote: > > Nit: I don't really understand this sentence. Not enough context without > > explicitly reading the code. > > Changed to "After calling this, no button row Views will be in the view > hierarchy." Is that accurate, though? Maybe you mean "...any necessary button row Views will be children of |button_row_container_| rather than |this|."? (They're still in the hierarchy in that case, just indirectly.) https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:150: DCHECK(gfx::Size() == button_row_container_->GetMaximumSize()); Nit: Since you declared kUnconstrained, maybe DCHECK_EQ(gfx::Size(kUnconstrained, kUnconstrained), button_row_container_->GetMaximumSize()); ? This also makes the code self documenting enough not to need the comment. https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:268: if (*member) { Nit: This is the rare case when I would find it clearer to not early-return, like so: // Create the button if it doesn't already exist. if (!*member) { ... *member = button; } // The button is now guaranteed to exist, so update its appearance. delegate->UpdateButton(button, type); } https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:273: const base::string16 title = delegate->GetDialogButtonLabel(type); Do we need to compute the correct button label, or can we just use string16()? UpdateButton() is going to update the label anyway, so calculating this kind of implies we have to calculate it when I think we don't. https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:278: PlatformStyle::kDialogDefaultButtonCanBeCancel); Similarly, it seems like UpdateButton() should get all this correct. If it doesn't, it seems like maybe it's a bug in UpdateButton(). https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:82: // Create and/or update the dialog button of |type| according to the Creates, updates (descriptive, not imperative; see http://google.github.io/styleguide/cppguide.html#Function_Comments ) https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:84: // the provided data |member| if non-null, otherwise updates it. This last sentence is still confusing to me. From reading the code, it seems like this whole comment could maybe be: Creates, deletes, or updates the appearance of the button of type |type| (which must be pointed to by |member|). Which action is chosen is based on whether DialogDelegate::GetDialogButtons() includes |type|, and whether |member| points to a button that already exists.
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
It's time for a rebase (might not get to that tonight), but everything else addressed. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:64: parent()->ChildPreferredSizeChanged(child); On 2017/02/28 03:33:23, Peter Kasting wrote: > On 2017/02/27 10:04:19, tapted wrote: > > On 2017/02/25 06:04:07, Peter Kasting wrote: > > > Nit: Optional, but seems like the only reason we need to bubble these two > > events > > > is for the parent to do "if (child_ == extra_view) > > SomethingInvolvingLayout();" > > > > > > I wonder if that functionality shouldn't be in here instead. The idea would > > be > > > that the parts of layout affecting the non-button-row, which would be our > > > parent's responsibility, wouldn't need to run when this changed; whereas > only > > > the layout for this container itself would be affected when something about > > the > > > extra view changes. > > > > > > Really, the question here is whether we can cleave more functionality out of > > the > > > parent and put it here instead (basically, almost everything you're adding > in > > > this CL). > > > > This crossed my mind too.. but I decided not to attempt it because all the > other > > lifetime management is the responsibility of DialogClientView - e.g. DCV > > provides accessors for ok_button/cancel_button. I'm tempted to try this in a > > follow-up, but I'm not certain it will improve things :/ > > I'm fine leaving it to a followup. I think it's worth trying, but it will be a > fairly large change. > > I don't expect there to be less code overall when it's done, but I do think it > would be nice for the scope of various overrides to be clearer (DialogClientView > itself will be doing less, which will make it easier to reason about what it > needs to do). Acknowledged. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:91: delete extra_view_; On 2017/02/28 03:33:23, Peter Kasting wrote: > On 2017/02/27 10:04:19, tapted wrote: > > On 2017/02/25 06:04:07, Peter Kasting wrote: > > > I'm confused. > > > > > > Visibility and being part of the view hierarachy are not normally in > lockstep > > > (invisible views can be part of the hierarchy). > > > > > > Why would we have a non-null extra_view_ that's not a child view? > > > > This is to satisfy some existing use-cases for DialogClientView. GridLayout > > doesn't consider visibility when doing layout, but the old DCV logic did. > Since > > DCV client code doesn't "own" the ExtraView once created, if the dialog later > > decides it doesn't really want an ExtraView, it currently makes it hidden. > > Where does this happen? > > Basically, I'm trying to figure out the control flow in the current code that > will allow |extra_view_| to be non-null yet not a child view. I can't find it. Most overrides of DCV::CreateExtraView() hold on to the View* that they return, so at any later time they can do extra_view->SetVisible(false); That will trigger a call into ChildVisibilityChanged(..), which does UpdateDialogButtons() and ShouldShow(extra_view_) will now report "no", so won't add it as a child view. But! Writing this out just made me realise that if the client later decides to do extra_view->SetVisible(true) This won't be picked up any more since the extra view is no longer in the hierarchy ಠ_ಠ. Ugh. http://crbug.com/165193 is the bug that precipitated hiding of extra_view. But that was for RequestAutocomplete, which was removed in http://crrev.com/391821 So.. I thought maybe we can just remove this logic: If you provide an ExtraView, it must remain visible (or you can just `delete` it too - that would probably work better). But DesktopMediaPickerDialogView is now also using this functionality for |audio_share_checkbox_|, so I think this needs to stay, but I need to fix the sequence of SetVisible(false) -> SetVisible(true). (Done+Tested). https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:151: if (max_size != gfx::Size()) On 2017/02/28 03:33:23, Peter Kasting wrote: > On 2017/02/27 10:04:19, tapted wrote: > > On 2017/02/25 06:04:07, Peter Kasting wrote: > > > Nit: !IsEmpty()? > > > > Changed this to > > > > if (max_size.height() != 0) > > > > and added a comment. (the width constraint [or lack of it] should just pass > > through without affecting things). > > > > added a DLOG for the possible constraints violation.. WDYT? > > I don't think we should add a DLOG, because it's not clear how you'll collect > the data or take action, and also because it makes it unclear how bad this is in > practice or whether we even expect it to occur. > > So if you're really worried this could occur in practice and we need to find it > and fix it if so, CHECK, and then file a bug to remove the CHECK if it doesn't > find anything for a while. If you think this can't occur in practice and we > need to assure readers of that, DCHECK. If it can't occur in practice and > readers need not consider the issue, remove this entirely. Yeah the DLOG would only be an indicator for developers. It's expensive. I think it can occur in practice, but I don't think we have reliable control over it, since the ContentsView() might be a webview. I'm happy to remove it. It's more of a note to say "there are cases where constraints can't be satisfied", which can be a comment too. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:191: // references to deleted Views. On 2017/02/28 03:33:23, Peter Kasting wrote: > On 2017/02/27 10:04:19, tapted wrote: > > On 2017/02/25 06:04:07, Peter Kasting wrote: > > > Is this important? Are there races or something that result in bustage if > we > > > don't null these out? It seems suspicious that we need most of this code at > > > all. > > > > It looks like this trend started a long time ago while fixing > > http://crbug.com/241451 . That was a use-after-free triggered from a > > DialogClientView method which no longer exists (OnWillChangeFocus). > > > > I think it is safe to remove the lines that clear out ok_button_ and > > cancel_button_. It's probably OK to remove extra_view_ too -- it's > dereferenced > > a bit more often and DCV has less control over it. E.g. a dialog may decide to > > `delete extra_view;` -- currently that works (and would even allow a new extra > > view to be created..) > > > > There is a test that would fail for this though. It does: > > > > TEST_F(DialogClientViewTest, RemoveAndUpdateButtons) { > > // Removing buttons from another context should clear the local pointer. > > SetDialogButtons(ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL); > > delete client_view()->ok_button(); > > > > So we would have to decide that that test case is no longer valid - maybe > > something better for a follow-up. > > Hmm, I didn't realize we had public accessors for these buttons. Given that we > do, I don't think we can remove this block entirely, unless we can kill those > accessors entirely, at least for non-test code. They're mostly used in test > code today, and maybe it would be OK to have them work unintuitively when > they're test-only, but DialogDelegate::GetInitiallyFocusedView() uses them, and > I think that use is probably both appropriate and mandates that we keep this. > > |extra_view_| has to be kept too, I think; we can't avoid nulling it out here if > we're going to do things like pointer-compare it in ChildPreferredSizeChanged(), > lest we want to risk really weird bugs. Acknowledged. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:376: // First column is only used if there is an extra view. On 2017/02/28 03:33:23, Peter Kasting wrote: > On 2017/02/27 10:04:19, tapted wrote: > > On 2017/02/25 06:04:07, Peter Kasting wrote: > > > I feel like with some cleverness this block and the ones below could all be > > put > > > into a loop and we could eliminate some of the constants above. But I am > > tired > > > enough not to want to spend time figuring out how to do it in detail. > > Something > > > about creating an array of bools like {ShouldShow(extra_view_), ok_button_ > && > > > cancel_button_, ???} and then just looping through these, setting the link > > > values to the current column in the GridLayout (which I dunno if it even has > > an > > > accessor for, but maybe it should). > > > > I'd toyed with this a bit.. The ButtonRowViews() approach was as far as I > could > > go before the code made me unhappy - the logic here is subtle, so it feels > good > > to me to play it out in a sequence. > > OK. I got an idea for this. (extra_view_ gets in the way a bit for the reasons above, but there is a loop now :) https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:400: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { On 2017/02/28 03:33:23, Peter Kasting wrote: > On 2017/02/27 10:04:19, tapted wrote: > > On 2017/02/25 06:04:08, Peter Kasting wrote: > > > Should we just go ahead and link the column sizes in pre-MD too? > > > > > > Yeah, it's a behavior change, but seems like one we could maybe just make. > > > > Hm - that scares me a little. It's not just that some dialogs may get suddenly > > wide due to long strings. > > Well, we're only enlarging linked widths when they're "near" in size, right? So > hopefully the scope of the enlarging is limited. That does tend to be the case. I couldn't find any _really_ long strings. "Create a new profile" was the longest, in a dialog with 3 buttons, so it can get quite wide. 950 pixels across in Bulgarian. I haven't landed the ignore-buttons-wider-than-xx-px change yet. If I get that in first this won't be so bad (but so close to branch point, I'd want to get that other CL in first :). I'm happy to wait for that and remove this line. Although one concern is that there's a regression in M58 for a dialog currently, and the CL in review to fix it at https://codereview.chromium.org/2692113002/ is probably not needed (and has downsides) -- I think this CL fixes the root cause by being able to properly handle min/max/preferred sizes. [haven't had time to patch this CL in on a Windows box with a bluetooth adapter yet to verify that theory..] > > > But also some dialogs will become inconsistent with > > DialogClientView-based dialogs. E.g. collected cookies view manages its own > > buttons. > > I don't know that I follow. Are you saying that this will cause size linking > for dialogs that use DialogClientView, but since not every dialog uses > DialogClientView, that means we'll have behavioral inconsistency? yup - exactly. > > If so, we have to fix that in Harmony anyway, right? Either by making these > actually use DialogClientView (preferable), or manually reimplementing this > everywhere that does this (ugh). > > At which point we can also Just Fix all those places for pre-Harmony. Or not; > I'm willing to live with an inconsistency like this for a few releases > pre-Harmony, as it's minor enough I doubt any users would even notice. sgtm > > > Also more visible things like the `add bookmark` bubble return `NONE` > > from DialogDelegate::GetDialogButtons() to do their own management of buttons. > > > > (for those it should be a lot simpler though! They are both already using > > GridLayout - they just need to link columns) > > Feel free to file bugs on making these dialogs be "real dialogs that don't > manage their own buttons" unless it seems like a bad idea. I'd like to get to a > point where all our dialogs are using a single common dialog framework, and then > if we change a behavior like this in one place, it changes everywhere. > > I haven't looked at these dialogs as recently as you so I don't know if maybe > there's a good reason why they simply can't do that. totally agree :). collected cookies [padlock -> click "XX in use"] has two button rows, one in the centre of the dialog, so it would be a challenge. Add bookmark bubble effectively has its "extra view" as the middle button on all platforms, so it would need a tweak.. but that actually got me thinking about GetButtonRowViews() again - maybe clients could override that, and/or perhaps we could add ui::DIALOG_BUTTON_AUXILLARY - which made me think of a way we could tighten up the logic around that. https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:96: // All buttons are removed from the hierarchy to be added by the layout. On 2017/02/28 03:33:23, Peter Kasting wrote: > On 2017/02/27 10:04:19, tapted wrote: > > On 2017/02/25 06:04:08, Peter Kasting wrote: > > > Nit: I don't really understand this sentence. Not enough context without > > > explicitly reading the code. > > > > Changed to "After calling this, no button row Views will be in the view > > hierarchy." > > Is that accurate, though? Maybe you mean "...any necessary button row Views > will be children of |button_row_container_| rather than |this|."? (They're > still in the hierarchy in that case, just indirectly.) Nah, in this case SetupLayout() is relying on SetupViews() to leave everything out of the view heirarchy, so it can add them all to the Layout (GridLayout always adds the child view to the layout host as well as adding the view to the layout, and DCHECKs if you try it any other way). (note that only SetupLayout() invokes SetupViews() -- it could all just be inlined there, but functional decomposition is nice too :). https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:150: DCHECK(gfx::Size() == button_row_container_->GetMaximumSize()); On 2017/02/28 03:33:23, Peter Kasting wrote: > Nit: Since you declared kUnconstrained, maybe > > DCHECK_EQ(gfx::Size(kUnconstrained, kUnconstrained), > button_row_container_->GetMaximumSize()); > > ? This also makes the code self documenting enough not to need the comment. Done. https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:268: if (*member) { On 2017/02/28 03:33:23, Peter Kasting wrote: > Nit: This is the rare case when I would find it clearer to not early-return, > like so: > > // Create the button if it doesn't already exist. > if (!*member) { > ... > *member = button; > } > > // The button is now guaranteed to exist, so update its appearance. > delegate->UpdateButton(button, type); > } > Done. https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:273: const base::string16 title = delegate->GetDialogButtonLabel(type); On 2017/02/28 03:33:23, Peter Kasting wrote: > Do we need to compute the correct button label, or can we just use string16()? > UpdateButton() is going to update the label anyway, so calculating this kind of > implies we have to calculate it when I think we don't. Unfortunately UpdateButton can be overridden, one override doesn't update the title (or call super::UpdateButton) in all cases. https://cs.chromium.org/chromium/src/chrome/browser/ui/views/global_error_bub... https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:278: PlatformStyle::kDialogDefaultButtonCanBeCancel); On 2017/02/28 03:33:23, Peter Kasting wrote: > Similarly, it seems like UpdateButton() should get all this correct. If it > doesn't, it seems like maybe it's a bug in UpdateButton(). Under non-MD, is_default gets used to determine whether the button class is views::BlueButton or views::LabelButton, so we need to calculate it here too (of course we will eventually want to delete views::BlueButton..) Added a TODO https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... File ui/views/window/dialog_client_view.h (right): https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:82: // Create and/or update the dialog button of |type| according to the On 2017/02/28 03:33:24, Peter Kasting wrote: > Creates, updates (descriptive, not imperative; see > http://google.github.io/styleguide/cppguide.html#Function_Comments ) Done. https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.h:84: // the provided data |member| if non-null, otherwise updates it. On 2017/02/28 03:33:23, Peter Kasting wrote: > This last sentence is still confusing to me. > > From reading the code, it seems like this whole comment could maybe be: > > Creates, deletes, or updates the appearance of the button of type |type| (which > must be pointed to by |member|). Which action is chosen is based on whether > DialogDelegate::GetDialogButtons() includes |type|, and whether |member| points > to a button that already exists. Done. https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:292: client_view()->SetBoundsRect(gfx::Rect(gfx::Point(0, 0), no_extra_view_size)); I just removed this bit - I don't think it makes sense, since it is explicitly violating both the dialog preferred size and the fixed/minimum size of the StaticSizedView in |extra_view_|. The resulting layout would be rubbish.
https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/240001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:400: if (ui::MaterialDesignController::IsSecondaryUiMaterial()) { On 2017/02/28 06:52:42, tapted wrote: > On 2017/02/28 03:33:23, Peter Kasting wrote: > > On 2017/02/27 10:04:19, tapted wrote: > > > On 2017/02/25 06:04:08, Peter Kasting wrote: > > > > Should we just go ahead and link the column sizes in pre-MD too? > > > > > > > > Yeah, it's a behavior change, but seems like one we could maybe just make. > > > > > > Hm - that scares me a little. It's not just that some dialogs may get > suddenly > > > wide due to long strings. > > > > Well, we're only enlarging linked widths when they're "near" in size, right? > So > > hopefully the scope of the enlarging is limited. > > That does tend to be the case. I couldn't find any _really_ long strings. > "Create a new profile" was the longest, in a dialog with 3 buttons, so it can > get quite wide. 950 pixels across in Bulgarian. Man, we really shouldn't have a button saying "Create a new profile". "Create", or maybe "Create profile". If that's the original English string, can you file a bug for whatever that dialog is requesting tighter wording? We have an actual tech writer now who can solve this sort of thing :) > collected cookies [padlock -> click "XX in use"] has two > button rows, one in the centre of the dialog, so it would be a challenge. Can the bottom one be the dialog's "button row" and the middle one be a custom set of buttons the dialog manages completely on its own? > Add bookmark bubble effectively has its "extra view" as the middle button on all > platforms, so it would need a tweak.. but that actually got me thinking about > GetButtonRowViews() again - maybe clients could override that, and/or perhaps we > could add ui::DIALOG_BUTTON_AUXILLARY - which made me think of a way we could > tighten up the logic around that. Yes, I think in general we should not use the "extra view" concept for a third button -- I'd rather do something like your BUTTON_AUXILIARY idea (and maybe we should rename _OK and _CANCEL to something like _DEFAULT and... uh... _SECONDARY?), and let the couple of dialogs that want three buttons use them that way, and use "extra view" only for the non-button case. (Which hopefully turns into the "never" case in Harmony, since Alan said that we should redesign dialogs that put something in that corner as we get to them.) That'd make it possible to make this layout code quite a bit simpler. It's getting beyond the scope of this CL. I don't care much what order all this happens in. Feel free to file bugs, or if you don't get to it I will tomorrow if I can, since this really seems worth doing. https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/300001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:273: const base::string16 title = delegate->GetDialogButtonLabel(type); On 2017/02/28 06:52:42, tapted wrote: > On 2017/02/28 03:33:23, Peter Kasting wrote: > > Do we need to compute the correct button label, or can we just use string16()? > > > UpdateButton() is going to update the label anyway, so calculating this kind > of > > implies we have to calculate it when I think we don't. > > Unfortunately UpdateButton can be overridden, one override doesn't update the > title (or call super::UpdateButton) in all cases. > > https://cs.chromium.org/chromium/src/chrome/browser/ui/views/global_error_bub... That just looks like a bug we should fix (though possibly benign in practice depending on whether any buttons exist when |error_| isn't set).
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: This issue passed the CQ dry run.
bsep@chromium.org changed reviewers: + bsep@chromium.org
LGTM. Peter asked me to be a second set of eyes on this. This is a really good refactor, much more readable now. https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:257: if ((delegate->GetDialogButtons() & type) == 0) { nit: ui::DialogButton::NONE rather than 0 https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:346: } // namespace views no new test for linking the button sizes?
https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:257: if ((delegate->GetDialogButtons() & type) == 0) { On 2017/03/01 00:29:50, Bret Sepulveda wrote: > nit: ui::DialogButton::NONE rather than 0 Dunno if I would make that change. I think we're trying to check "is this button not present" rather than "are no buttons present" (which is really the value that NONE is supposed to have). One can imagine making NONE a nonzero value that is nevertheless bitwise-exclusive with the other button values, in which case using NONE here would be wrong. Maybe the code would be clearer as "if (!(delegate->GetDialogButtons() & type))"?
https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:257: if ((delegate->GetDialogButtons() & type) == 0) { On 2017/03/01 00:35:31, Peter Kasting wrote: > On 2017/03/01 00:29:50, Bret Sepulveda wrote: > > nit: ui::DialogButton::NONE rather than 0 > > Dunno if I would make that change. I think we're trying to check "is this > button not present" rather than "are no buttons present" (which is really the > value that NONE is supposed to have). One can imagine making NONE a nonzero > value that is nevertheless bitwise-exclusive with the other button values, in > which case using NONE here would be wrong. > > Maybe the code would be clearer as "if (!(delegate->GetDialogButtons() & > type))"? Oh you're right, I misread this line. I think Peter's suggestion is a little clearer but I think it's fine either way.
LGTM https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:154: // Note not all constraints can be met. E.g. It's possible here for Nit: It's -> it's https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:264: // This should only need to assign a newly constructed Button to |*member|. Nit: It wasn't instantly clear to me that this was an aspirational (rather than accurate) comment. Might want to prefix with "In theory, ..." or something. https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:315: return {{first, second, third}}; Huh, what's the second set of {} do? https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:384: for (size_t view_index = 0; view_index < kNumButtons; ++view_index) { :D! https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:396: column_set->LinkColumnSizes(link[1], link[2], -1); Interestingly, there seems to be no way to unlink column sizes -- once linked, they'll stay linked. Does that pose a problem if children toggle visibility? (This also implies we could save a line by doing this, but maybe it's too clever:) column_set->LinkColumnSizes(link[1], link[2], -1); if (views[0] && !CustomButton::AsCustomButton(views[0])) column_set->LinkColumnSizes(link[0], link[1], -1);
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: This issue passed the CQ dry run.
PTAL (added a test) https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:154: // Note not all constraints can be met. E.g. It's possible here for On 2017/03/01 01:52:35, Peter Kasting wrote: > Nit: It's -> it's Done. https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:257: if ((delegate->GetDialogButtons() & type) == 0) { On 2017/03/01 00:39:33, Bret Sepulveda wrote: > On 2017/03/01 00:35:31, Peter Kasting wrote: > > On 2017/03/01 00:29:50, Bret Sepulveda wrote: > > > nit: ui::DialogButton::NONE rather than 0 > > > > Dunno if I would make that change. I think we're trying to check "is this > > button not present" rather than "are no buttons present" (which is really the > > value that NONE is supposed to have). One can imagine making NONE a nonzero > > value that is nevertheless bitwise-exclusive with the other button values, in > > which case using NONE here would be wrong. > > > > Maybe the code would be clearer as "if (!(delegate->GetDialogButtons() & > > type))"? > > Oh you're right, I misread this line. I think Peter's suggestion is a little > clearer but I think it's fine either way. Done. (went with `if (!(delegate->GetDialogButtons() & type))` :) https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:264: // This should only need to assign a newly constructed Button to |*member|. On 2017/03/01 01:52:35, Peter Kasting wrote: > Nit: It wasn't instantly clear to me that this was an aspirational (rather than > accurate) comment. Might want to prefix with "In theory, ..." or something. Done. and added "See http://crbug.com/697303 ." https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:315: return {{first, second, third}}; On 2017/03/01 01:52:34, Peter Kasting wrote: > Huh, what's the second set of {} do? Makes a compile warning go away :) [1]. (std::array isn't initialized from std::initializer_list - it doesn't have any constructors - so this is really initializing array.data_ rather than the array type) [1] error: suggest braces around initialization of subobject [-Werror,-Wmissing-braces] (also apparently "double-braces required in C++11 (not in C++14)" - so I guess someone decided the double-braces were redundant too). https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:396: column_set->LinkColumnSizes(link[1], link[2], -1); On 2017/03/01 01:52:35, Peter Kasting wrote: > Interestingly, there seems to be no way to unlink column sizes -- once linked, > they'll stay linked. Does that pose a problem if children toggle visibility? Nah - visibility changes invoke a fresh SetupLayout(), and the `SetLayoutManager(layout)` at the top will completely destroy any old layout configuration. > > (This also implies we could save a line by doing this, but maybe it's too > clever:) > > column_set->LinkColumnSizes(link[1], link[2], -1); > if (views[0] && !CustomButton::AsCustomButton(views[0])) > column_set->LinkColumnSizes(link[0], link[1], -1); Yah, that might be depending too much on internals. (without other information, I think I'd read that as wanting to link [0] and [1] "instead of" [1] and [2], rather than being transitive/"as well as"). https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... File ui/views/window/dialog_client_view_unittest.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:346: } // namespace views On 2017/03/01 00:29:50, Bret Sepulveda wrote: > no new test for linking the button sizes? Done.
On 2017/03/01 10:44:58, tapted wrote: > PTAL (added a test) (also note that to retain sanity in this busy time, I'm planning on landing this after branch - but let me know if that would get in the way or block other things in the pipeline)
On 2017/03/01 10:44:58, tapted wrote: > PTAL (added a test) Nice test, still lgtm.
LGTM https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... File ui/views/window/dialog_client_view.cc (right): https://codereview.chromium.org/2706423002/diff/340001/ui/views/window/dialog... ui/views/window/dialog_client_view.cc:315: return {{first, second, third}}; On 2017/03/01 10:44:58, tapted wrote: > On 2017/03/01 01:52:34, Peter Kasting wrote: > > Huh, what's the second set of {} do? > > Makes a compile warning go away :) [1]. (std::array isn't initialized from > std::initializer_list - it doesn't have any constructors - so this is really > initializing array.data_ rather than the array type) > > [1] error: suggest braces around initialization of subobject > [-Werror,-Wmissing-braces] > > (also apparently "double-braces required in C++11 (not in C++14)" - so I guess > someone decided the double-braces were redundant too). Interesting. 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:73: return DialogDelegate::GetDialogButtonLabel(button); Nit: Optional fractionally-less-verbose form: return (button == ui::DIALOG_BUTTON_CANCEL && !cancel_label_.empty()) ? cancel_label_ : DialogDelegate::GetDialogButtonLabel(button); https://codereview.chromium.org/2706423002/diff/360001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:135: cancel_label_ = base::ASCIIToUTF16("Cancel Cancel"); Nit: Maybe make this string even longer to reduce the likelihood that it falls under the minimum cancel width if we enlarge that in the future? https://codereview.chromium.org/2706423002/diff/360001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:376: EXPECT_GT(cancel_button_width, ok_button_only_width); Nit: If you're just checking that they're different, use EXPECT_NE https://codereview.chromium.org/2706423002/diff/360001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:397: delete extra_button; You delete |extra_button| here but not |boring_view| below. Is that because while something is set as the extra view, it's owned by the dialog, but resetting that eliminates ownership? * If no, then you have a mem leak or double free. * If yes, then it seems like SetExtraView() should delete the old extra view, not just drop it on the floor. And it seems like you shouldn't actually free this until after calling SetExtraView() below, because technically there's a window where you've deleted an object the other code expects to be alive. * In general, it would be nice for SetExtraView() to take a scoped_ptr or something if it's going to take ownership. General API fixes here are beyond the scope of this patch, but I would try to make the code here consistent, and consider whether it's worth doing the rest in a followup.
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...
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:73: return DialogDelegate::GetDialogButtonLabel(button); On 2017/03/02 23:09:34, Peter Kasting wrote: > Nit: Optional fractionally-less-verbose form: > > return (button == ui::DIALOG_BUTTON_CANCEL && !cancel_label_.empty()) > ? cancel_label_ > : DialogDelegate::GetDialogButtonLabel(button); Done. https://codereview.chromium.org/2706423002/diff/360001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:135: cancel_label_ = base::ASCIIToUTF16("Cancel Cancel"); On 2017/03/02 23:09:35, Peter Kasting wrote: > Nit: Maybe make this string even longer to reduce the likelihood that it falls > under the minimum cancel width if we enlarge that in the future? Done. https://codereview.chromium.org/2706423002/diff/360001/ui/views/window/dialog... ui/views/window/dialog_client_view_unittest.cc:376: EXPECT_GT(cancel_button_width, ok_button_only_width); On 2017/03/02 23:09:34, Peter Kasting wrote: > Nit: If you're just checking that they're different, use EXPECT_NE Updated the comment - the cancel button needs to be the "one" that is bigger, otherwise the constants would need to be reversed (because ok would dominate instead). -> // Ensure the single buttons have different preferred widths when alone, and // that the Cancel button is bigger (so that it dominates the size). 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/02 23:09:34, Peter Kasting wrote: > You delete |extra_button| here but not |boring_view| below. Is that because > while something is set as the extra view, it's owned by the dialog, but > resetting that eliminates ownership? yes. views::View's destructor always ensures it gets removed from the view hierarchy, and `delete view;` is a common pattern for doing this. E.g. views::ScrollView's destructor does this. also app_list in a few places (e.g. AppsGridView). E.g. there's no need to do something like `parent()->RemoveChildView(view)` to ensure the ownership change is properly communicated (and doing that would in fact require a separate delete anyway). > > * If no, then you have a mem leak or double free. > * If yes, then it seems like SetExtraView() should delete the old extra view, > not just drop it on the floor. And it seems like you shouldn't actually free > this until after calling SetExtraView() below, because technically there's a > window where you've deleted an object the other code expects to be alive. > * In general, it would be nice for SetExtraView() to take a scoped_ptr or > something if it's going to take ownership. views::View and scoped_ptr don't mix well :/. In the test harness, SetExtraView() doesn't hold on to the view in the test harness (and DialogClientView doesn't have an accessor, so it's not able to delete the old extra_view). SetExtraView()'s role is to trigger a call to the CreateExtraView() override which transfers ownership to DialogClientViewTest (and it has checks to ensure that CreateExtraView() is, in fact, called by DialogClientView). > > General API fixes here are beyond the scope of this patch, but I would try to > make the code here consistent, and consider whether it's worth doing the rest in > a followup. I've added a comment. // Remove |extra_button| from the View hierarchy so that it can be replaced. 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). And DCV is already written to support this without any lifetime problems: it clears out references before the destructor completes. I'll be formalising this a bit while fixing http://crbug.com/697296. That is, I don't like the `ShouldShow(extra_view)` stuff: View visibility should not affect Layout (and if a dialog *does* want to affect Layout, it should just not provide an extraview, or it should `delete extra_view;` if for some reason it's unable to do that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsep@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2706423002/#ps380001 (title: "respond to unittest comments")
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": 380001, "attempt_start_ts": 1488772745715400, "parent_rev": "5f0498f32748dd816a021ac5cd3c919693b679b7", "commit_rev": "cc8ff78f49b8fe8265a15ed227ec273c08dd695a"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/cc8ff78f49b8fe8265a15ed227ec... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:380001) as https://chromium.googlesource.com/chromium/src/+/cc8ff78f49b8fe8265a15ed227ec...
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 . |