Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1442683002:
Fix views::View::BoundsChanged (Closed)
DescriptionFix views::View::BoundsChanged
View::Layout is not invoked if only origin has been changed.
R=sadrul@chromium.org,sky@chromium.org
BUG=none
TEST=See unittest.
Committed: https://crrev.com/b5412060fc9373b1f48a2799523a167ba4fcef63
Cr-Commit-Position: refs/heads/master@{#360551}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fix test #
Total comments: 4
Patch Set 3 : Fix 2 #
Messages
Total messages: 18 (3 generated)
Under what circumstances in this causing problems? I'm hesitant to change this as it may result in a lot more unnecessary calls to layout.
The CQ bit was checked by flint@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442683002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442683002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2015/11/12 15:58:56, sky wrote: > Under what circumstances in this causing problems? I'm hesitant to change this > as it may result in a lot more unnecessary calls to layout. Additional layout is called only when |needs_layout_ == true| and in this case the call is mandatory. Two cases when layout is required but is not called: https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... ui/views/view_unittest.cc:4130: parent->SetBounds(10, 0, 100, 100); Layout of parent is invalidated during parent->AddChildView(child) call: https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s... parent->SetBounds does not call its Layout because in the bounds of parent only origin has changed. As a result, bounds of child will not be updated. https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... ui/views/view_unittest.cc:4140: parent->SetBounds(20, 0, 100, 100); The child view needs layout (i.e. its contents became bigger) and calls InvalidateLayout. But parent's Layout is not called during parent->SetBounds because only parent's bounds' origin changed. So child's bounds will not be changed. And Layout of child will not be called too. If a view invalidates layout, we have to call its Layout. Because of this, now View::SetBoundsRect calls Layout even if bounds were not changed (e.g. when it handles DragNDrop): https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s... But it does not call Layout if the bounds' origin changed, and bounds' size did not change.
I get what the code does, my question is where are you encountering this bug in practice? On Fri, Nov 13, 2015 at 2:05 AM, <flint@yandex-team.ru> wrote: > On 2015/11/12 15:58:56, sky wrote: >> >> Under what circumstances in this causing problems? I'm hesitant to change >> this >> as it may result in a lot more unnecessary calls to layout. > > > Additional layout is called only when |needs_layout_ == true| and in this > case > the call is mandatory. > > Two cases when layout is required but is not called: > > > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc > File ui/views/view_unittest.cc (right): > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... > ui/views/view_unittest.cc:4130: parent->SetBounds(10, 0, 100, 100); > Layout of parent is invalidated during parent->AddChildView(child) call: > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s... > parent->SetBounds does not call its Layout because in the bounds of > parent only origin has changed. As a result, bounds of child will not be > updated. > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... > ui/views/view_unittest.cc:4140: parent->SetBounds(20, 0, 100, 100); > The child view needs layout (i.e. its contents became bigger) and calls > InvalidateLayout. But parent's Layout is not called during > parent->SetBounds because only parent's bounds' origin changed. So > child's bounds will not be changed. And Layout of child will not be > called too. > > If a view invalidates layout, we have to call its Layout. Because of > this, now View::SetBoundsRect calls Layout even if bounds were not > changed (e.g. when it handles DragNDrop): > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s... > But it does not call Layout if the bounds' origin changed, and bounds' > size did not change. > > https://codereview.chromium.org/1442683002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/11/13 16:02:41, sky wrote: > I get what the code does, my question is where are you encountering > this bug in practice? > > On Fri, Nov 13, 2015 at 2:05 AM, <mailto:flint@yandex-team.ru> wrote: > > On 2015/11/12 15:58:56, sky wrote: > >> > >> Under what circumstances in this causing problems? I'm hesitant to change > >> this > >> as it may result in a lot more unnecessary calls to layout. > > > > > > Additional layout is called only when |needs_layout_ == true| and in this > > case > > the call is mandatory. > > > > Two cases when layout is required but is not called: > > > > > > > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc > > File ui/views/view_unittest.cc (right): > > > > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... > > ui/views/view_unittest.cc:4130: parent->SetBounds(10, 0, 100, 100); > > Layout of parent is invalidated during parent->AddChildView(child) call: > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s... > > parent->SetBounds does not call its Layout because in the bounds of > > parent only origin has changed. As a result, bounds of child will not be > > updated. > > > > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... > > ui/views/view_unittest.cc:4140: parent->SetBounds(20, 0, 100, 100); > > The child view needs layout (i.e. its contents became bigger) and calls > > InvalidateLayout. But parent's Layout is not called during > > parent->SetBounds because only parent's bounds' origin changed. So > > child's bounds will not be changed. And Layout of child will not be > > called too. > > > > If a view invalidates layout, we have to call its Layout. Because of > > this, now View::SetBoundsRect calls Layout even if bounds were not > > changed (e.g. when it handles DragNDrop): > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&s... > > But it does not call Layout if the bounds' origin changed, and bounds' > > size did not change. > > > > https://codereview.chromium.org/1442683002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > I initially encountered this problem in non-chromium code. The situation is reproduced in the test case |ViewTest.InvalidateLayout|. Nevertheless, this bug can affect chromium code (i.e. |LabelButton| calls |InvalidateLayout| or |PreferredSizeChanged|, but layout of the button owner may not call layout of the button, so bounds of |LabelButton::label_| and |LabelButton::image_| may not get updated).
Ok, fair enough. https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... ui/views/view_unittest.cc:4116: bool layout_was_called = false; Make private (style guide says tests can use protected members, not public). Also, end with _ https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... ui/views/view_unittest.cc:4118: }; private: DISALLOW... https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... ui/views/view_unittest.cc:4120: TEST_F(ViewTest, SimpleInvalidateLayout) { Please add description of what assertions your test is verifying. Especially when the name is very generic. Same comment for test below. https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... ui/views/view_unittest.cc:4121: scoped_ptr<CustomView> parent(new CustomView); Why use a scoped_ptr here? https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... ui/views/view_unittest.cc:4149: scoped_ptr<CustomView> parent(new CustomView); Same commeont about scoped_ptr.
Originally I made two test cases to illustrate two similar scenarios. Final test case is sufficient for final CL. Also I moved code needed for the test to run into |TestView|. I decided not to put |did_layout_| into private section for consistency with current code. > Ok, fair enough. > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc > File ui/views/view_unittest.cc (right): > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... > ui/views/view_unittest.cc:4116: bool layout_was_called = false; > Make private (style guide says tests can use protected members, not public). > Also, end with _ > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... > ui/views/view_unittest.cc:4118: }; > private: DISALLOW... > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... > ui/views/view_unittest.cc:4120: TEST_F(ViewTest, SimpleInvalidateLayout) { > Please add description of what assertions your test is verifying. Especially > when the name is very generic. Same comment for test below. > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... > ui/views/view_unittest.cc:4121: scoped_ptr<CustomView> parent(new CustomView); > Why use a scoped_ptr here? > > https://codereview.chromium.org/1442683002/diff/1/ui/views/view_unittest.cc#n... > ui/views/view_unittest.cc:4149: scoped_ptr<CustomView> parent(new CustomView); > Same commeont about scoped_ptr.
https://codereview.chromium.org/1442683002/diff/20001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/1442683002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:244: void Layout() override; inline the implementation like the rest. https://codereview.chromium.org/1442683002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:296: TEST_F(ViewTest, LayoutCalledAfterLayoutIsInvalidated) { Be more descriptive, eg LayoutCalledInvalidateAndOriginChanges
https://codereview.chromium.org/1442683002/diff/20001/ui/views/view_unittest.cc File ui/views/view_unittest.cc (right): https://codereview.chromium.org/1442683002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:244: void Layout() override; On 2015/11/18 16:55:50, sky wrote: > inline the implementation like the rest. Done. https://codereview.chromium.org/1442683002/diff/20001/ui/views/view_unittest.... ui/views/view_unittest.cc:296: TEST_F(ViewTest, LayoutCalledAfterLayoutIsInvalidated) { On 2015/11/18 16:55:50, sky wrote: > Be more descriptive, eg LayoutCalledInvalidateAndOriginChanges Done.
LGTM
The CQ bit was checked by flint@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442683002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442683002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b5412060fc9373b1f48a2799523a167ba4fcef63 Cr-Commit-Position: refs/heads/master@{#360551} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
