Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1774923002:
MacViews: Fix failing bubble frame view tests after r375729. (Closed)
|
Created:
4 years, 9 months ago by karandeepb Modified:
4 years, 9 months ago CC:
chromium-reviews, tfarina, msw+watch_chromium.org, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Fix failing bubble frame view tests after r375729.
r375729 changed all the bubble borders on MacViews to use
BubbleBorder::NO_ASSETS. This caused the following tests to regress-
BubbleFrameViewTest.GetMaximumSize
BubbleFrameViewTest.GetMinimumSize
BubbleFrameViewTest.GetPreferredSize
These tests regressed since the constants kExpectedBorderHeight and
kExpectedBorderWidth are set as per BubbleBorder::NO_SHADOWS. This CL fixes
these tests by getting the border insets programmatically, rather than depending
on fixed constants.
BUG=592856
Committed: https://crrev.com/59a5f4fd9d5b761d459878a1616a4c76bce36183
Cr-Commit-Position: refs/heads/master@{#381148}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Addressed review comments. #
Total comments: 2
Patch Set 3 : Rename constants. #
Total comments: 5
Patch Set 4 : Modify comment. #
Total comments: 4
Patch Set 5 : Address review comments. #Messages
Total messages: 19 (7 generated)
Description was changed from
==========
Bubble Frame View Test Regression.
==========
to
==========
MacViews: Fix failing bubble frame view tests after r375729.
r375729 changed all the bubble borders on MacViews to use
BubbleBorder::NO_ASSETS. This caused the following tests to regress-
BubbleFrameViewTest.GetMaximumSize
BubbleFrameViewTest.GetMinimumSize
BubbleFrameViewTest.GetPreferredSize
These tests regressed since the constants kExpectedBorderHeight and
kExpectedBorderWidth are set as per BubbleBorder::NO_SHADOWS. This CL fixes
these tests by getting the border insets programmatically, rather than depending
on fixed constants.
BUG=592856
==========
karandeepb@chromium.org changed reviewers: + tapted@chromium.org
PTAL Trent. Thanks. https://codereview.chromium.org/1774923002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view_unittest.cc (left): https://codereview.chromium.org/1774923002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:34: const int kExpectedBorderHeight = 29; The constants kExpectedBorderHeight and kExpectedBorderWidth also accounted for other non-client areas like title bar, footnote etc., in addition to the bubble border. See BubbleFrameView::GetSizeForClientSize(..).
https://codereview.chromium.org/1774923002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view_unittest.cc (left): https://codereview.chromium.org/1774923002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:421: gfx::Size preferred_size = frame.GetPreferredSize(); what about something like gfx::Rect preferred_size = gfx::Rect(frame.GetPreferredSize()); preferred_size.Inset(frame.bubble_border()->GetInsets()); // .. would that allow the expectations to remain the same, after adjusting the constants of kExpectedBorderWidth/Height?
PTAL Trent. Thanks. https://codereview.chromium.org/1774923002/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view_unittest.cc (left): https://codereview.chromium.org/1774923002/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view_unittest.cc:421: gfx::Size preferred_size = frame.GetPreferredSize(); On 2016/03/08 23:38:09, tapted wrote: > what about something like > > gfx::Rect preferred_size = gfx::Rect(frame.GetPreferredSize()); > preferred_size.Inset(frame.bubble_border()->GetInsets()); > // .. > > would that allow the expectations to remain the same, after adjusting the > constants of kExpectedBorderWidth/Height? Done.
lgtm % nit https://codereview.chromium.org/1774923002/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/1774923002/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:35: const int kExpectedNonClientWidth = 12; nit: I actually prefer kExpectedBorderWidth/Height - the non-client width could be the entire frame width rather than just the thickness of the border.
karandeepb@chromium.org changed reviewers: + msw@chromium.org
+msw@ for owner's review. https://codereview.chromium.org/1774923002/diff/20001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/1774923002/diff/20001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:35: const int kExpectedNonClientWidth = 12; On 2016/03/09 00:19:43, tapted wrote: > nit: I actually prefer kExpectedBorderWidth/Height - the non-client width could > be the entire frame width rather than just the thickness of the border. Done. Though it can be slightly confusing that they don't take the bubble border into account.
https://codereview.chromium.org/1774923002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/1774923002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:34: // The following do not take bubble border into consideration. Maybe describe what border this *does* account for? (what border does BubbleFrameView have if not a BubbleBorder?) (is this just the size added for the frame surrounding the content?) https://codereview.chromium.org/1774923002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:425: preferred_rect.Inset(frame.bubble_border()->GetInsets()); So this ignores the bubble border size? (gets frame size w/o border?)
PTAL msw@. Thanks. https://codereview.chromium.org/1774923002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/1774923002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:34: // The following do not take bubble border into consideration. On 2016/03/14 17:16:25, msw wrote: > Maybe describe what border this *does* account for? > (what border does BubbleFrameView have if not a BubbleBorder?) > (is this just the size added for the frame surrounding the content?) Have clarified the comment. Yeah this is the size added for the frame surrounding the content. Maybe we can have a better name for the constants? I had used kExpectedNonClientWidth/Height earlier, but Trent pointed out that it may mean the entire frame width/height. https://codereview.chromium.org/1774923002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:425: preferred_rect.Inset(frame.bubble_border()->GetInsets()); On 2016/03/14 17:16:25, msw wrote: > So this ignores the bubble border size? (gets frame size w/o border?) Yeah. Either the bubble border insets can be "added" to the expected size or "subtracted" from the actual size.
lgtm with nits and optional renaming https://codereview.chromium.org/1774923002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/1774923002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:34: // The following do not take bubble border into consideration. On 2016/03/14 23:35:39, karandeepb wrote: > On 2016/03/14 17:16:25, msw wrote: > > Maybe describe what border this *does* account for? > > (what border does BubbleFrameView have if not a BubbleBorder?) > > (is this just the size added for the frame surrounding the content?) > > Have clarified the comment. Yeah this is the size added for the frame > surrounding the content. Maybe we can have a better name for the constants? I > had used kExpectedNonClientWidth/Height earlier, but Trent pointed out that it > may mean the entire frame width/height. NonClient seems ok to me, or maybe kExpectedAdditionalWidth/Height? (it's at least clearer now that you revised the comment) https://codereview.chromium.org/1774923002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/1774923002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:34: // These account for non-client areas like like title bar, footnote etc. However nit: 'like the', instead of 'like like' https://codereview.chromium.org/1774923002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:35: // these do not take bubble border into consideration. nit: 'the bubble border'
https://codereview.chromium.org/1774923002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/1774923002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:34: // These account for non-client areas like like title bar, footnote etc. However On 2016/03/15 00:00:49, msw wrote: > nit: 'like the', instead of 'like like' Done. https://codereview.chromium.org/1774923002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:35: // these do not take bubble border into consideration. On 2016/03/15 00:00:49, msw wrote: > nit: 'the bubble border' Done.
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1774923002/#ps80001 (title: "Address review comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1774923002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1774923002/80001
Message was sent while issue was closed.
Description was changed from
==========
MacViews: Fix failing bubble frame view tests after r375729.
r375729 changed all the bubble borders on MacViews to use
BubbleBorder::NO_ASSETS. This caused the following tests to regress-
BubbleFrameViewTest.GetMaximumSize
BubbleFrameViewTest.GetMinimumSize
BubbleFrameViewTest.GetPreferredSize
These tests regressed since the constants kExpectedBorderHeight and
kExpectedBorderWidth are set as per BubbleBorder::NO_SHADOWS. This CL fixes
these tests by getting the border insets programmatically, rather than depending
on fixed constants.
BUG=592856
==========
to
==========
MacViews: Fix failing bubble frame view tests after r375729.
r375729 changed all the bubble borders on MacViews to use
BubbleBorder::NO_ASSETS. This caused the following tests to regress-
BubbleFrameViewTest.GetMaximumSize
BubbleFrameViewTest.GetMinimumSize
BubbleFrameViewTest.GetPreferredSize
These tests regressed since the constants kExpectedBorderHeight and
kExpectedBorderWidth are set as per BubbleBorder::NO_SHADOWS. This CL fixes
these tests by getting the border insets programmatically, rather than depending
on fixed constants.
BUG=592856
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from
==========
MacViews: Fix failing bubble frame view tests after r375729.
r375729 changed all the bubble borders on MacViews to use
BubbleBorder::NO_ASSETS. This caused the following tests to regress-
BubbleFrameViewTest.GetMaximumSize
BubbleFrameViewTest.GetMinimumSize
BubbleFrameViewTest.GetPreferredSize
These tests regressed since the constants kExpectedBorderHeight and
kExpectedBorderWidth are set as per BubbleBorder::NO_SHADOWS. This CL fixes
these tests by getting the border insets programmatically, rather than depending
on fixed constants.
BUG=592856
==========
to
==========
MacViews: Fix failing bubble frame view tests after r375729.
r375729 changed all the bubble borders on MacViews to use
BubbleBorder::NO_ASSETS. This caused the following tests to regress-
BubbleFrameViewTest.GetMaximumSize
BubbleFrameViewTest.GetMinimumSize
BubbleFrameViewTest.GetPreferredSize
These tests regressed since the constants kExpectedBorderHeight and
kExpectedBorderWidth are set as per BubbleBorder::NO_SHADOWS. This CL fixes
these tests by getting the border insets programmatically, rather than depending
on fixed constants.
BUG=592856
Committed: https://crrev.com/59a5f4fd9d5b761d459878a1616a4c76bce36183
Cr-Commit-Position: refs/heads/master@{#381148}
==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/59a5f4fd9d5b761d459878a1616a4c76bce36183 Cr-Commit-Position: refs/heads/master@{#381148} |
