Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
454173002:
Fixed BubbleBorder sizing problems & added unit tests. (Closed)
DescriptionFixed BubbleBorder sizing problems & added unit tests.
BUG=395622
TEST=automated
Committed: https://crrev.com/8bbc59b53df13d6e88cb4f97ced210324b745356
Cr-Commit-Position: refs/heads/master@{#292145}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Combined test fixtures and rewrote tests to be more easily maintained. #
Total comments: 22
Patch Set 3 : Reworked BubbleBorderTests to be data driven. #
Total comments: 48
Patch Set 4 : Simplified BubbleBorderTests. #
Total comments: 9
Patch Set 5 : Reverted BubbleBorderTest.GetBoundsOriginTest. #
Total comments: 8
Patch Set 6 : Minor nit fixes. #
Total comments: 2
Patch Set 7 : Merge with tot. #Patch Set 8 : Replaced kMidAnchor? with kAnchor.CenterPoint()?(). #Patch Set 9 : Fixed construction order of the TrayBubbleView/TrayBubbleWrapper in WebNotificationBubbleWrapper co… #
Total comments: 1
Messages
Total messages: 32 (0 generated)
msw@ can you please review this :)
You can replace all the TEST= lines with TEST=automated. https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... File ui/views/bubble/bubble_border.cc (right): https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... ui/views/bubble/bubble_border.cc:281: if (arrow_paint_type_ == PAINT_NONE || !has_arrow(arrow_)) Wow, nice catch! https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... File ui/views/bubble/bubble_border_unittest.cc (right): https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... ui/views/bubble/bubble_border_unittest.cc:206: class BubbleBorderGetBoundsSizeTest : public views::ViewsTestBase { Merge the two test classes, they're similar enough. (optionally replace the BubbleBorderTest typedef) https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... ui/views/bubble/bubble_border_unittest.cc:208: const BubbleBorder::Arrow kHorizontalArrow = BubbleBorder::TOP_CENTER; nit: inline these constants, they're only used once each. https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... ui/views/bubble/bubble_border_unittest.cc:217: virtual gfx::Size GetSize(gfx::Size contents_size) { Make these member functions protected and non-virtual. https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... ui/views/bubble/bubble_border_unittest.cc:218: DCHECK(bubble_border_); Remove DCHECKs before accessing the member. https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... ui/views/bubble/bubble_border_unittest.cc:226: 0 /* color */)); nit: just use SK_ColorWHITE or similar and remove the comment. https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... ui/views/bubble/bubble_border_unittest.cc:251: EXPECT_EQ(gfx::Size(45, 29), GetSize(kSmallSize)); These values won't be robust to bubble asset changes, etc. Can you break them up into components that would be easier to update, then ensure that the resulting sizes match some combination, like: const int kBorder = x; ... EXPECT_EQ(2 * kBorder + kSmallSize.width(), GetSize(kSmallSize).width()) https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... ui/views/bubble/bubble_border_unittest.cc:338: virtual gfx::Point GetOrigin(gfx::Rect anchor, gfx::Size contents_size) { nit: remove |contents_size| and send |kContentsSize| to GetBounds. https://codereview.chromium.org/454173002/diff/1/ui/views/bubble/bubble_borde... ui/views/bubble/bubble_border_unittest.cc:353: EXPECT_EQ(gfx::Point(78, 96), GetOrigin(kEmptyAnchor, kContentsSize)); Ditto; can you make this more robust with something like: const int kAnchorOffset = x; ... EXPECT_EQ(kAnchorOffset + kEmptyAnchor.x(), GetOrigin(kEmptyAnchor).x());
msw@, can you take another look :) https://chromiumcodereview.appspot.com/454173002/diff/1/ui/views/bubble/bubbl... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/1/ui/views/bubble/bubbl... ui/views/bubble/bubble_border_unittest.cc:206: class BubbleBorderGetBoundsSizeTest : public views::ViewsTestBase { On 2014/08/11 22:26:33, msw wrote: > Merge the two test classes, they're similar enough. > (optionally replace the BubbleBorderTest typedef) Done. https://chromiumcodereview.appspot.com/454173002/diff/1/ui/views/bubble/bubbl... ui/views/bubble/bubble_border_unittest.cc:208: const BubbleBorder::Arrow kHorizontalArrow = BubbleBorder::TOP_CENTER; On 2014/08/11 22:26:32, msw wrote: > nit: inline these constants, they're only used once each. Done. https://chromiumcodereview.appspot.com/454173002/diff/1/ui/views/bubble/bubbl... ui/views/bubble/bubble_border_unittest.cc:217: virtual gfx::Size GetSize(gfx::Size contents_size) { On 2014/08/11 22:26:33, msw wrote: > Make these member functions protected and non-virtual. Done. https://chromiumcodereview.appspot.com/454173002/diff/1/ui/views/bubble/bubbl... ui/views/bubble/bubble_border_unittest.cc:218: DCHECK(bubble_border_); On 2014/08/11 22:26:32, msw wrote: > Remove DCHECKs before accessing the member. Done. https://chromiumcodereview.appspot.com/454173002/diff/1/ui/views/bubble/bubbl... ui/views/bubble/bubble_border_unittest.cc:226: 0 /* color */)); On 2014/08/11 22:26:33, msw wrote: > nit: just use SK_ColorWHITE or similar and remove the comment. Done. https://chromiumcodereview.appspot.com/454173002/diff/1/ui/views/bubble/bubbl... ui/views/bubble/bubble_border_unittest.cc:251: EXPECT_EQ(gfx::Size(45, 29), GetSize(kSmallSize)); On 2014/08/11 22:26:32, msw wrote: > These values won't be robust to bubble asset changes, etc. Can you break them up > into components that would be easier to update, then ensure that the resulting > sizes match some combination, like: > const int kBorder = x; > ... > EXPECT_EQ(2 * kBorder + kSmallSize.width(), GetSize(kSmallSize).width()) Done. https://chromiumcodereview.appspot.com/454173002/diff/1/ui/views/bubble/bubbl... ui/views/bubble/bubble_border_unittest.cc:338: virtual gfx::Point GetOrigin(gfx::Rect anchor, gfx::Size contents_size) { On 2014/08/11 22:26:33, msw wrote: > nit: remove |contents_size| and send |kContentsSize| to GetBounds. Done. https://chromiumcodereview.appspot.com/454173002/diff/1/ui/views/bubble/bubbl... ui/views/bubble/bubble_border_unittest.cc:353: EXPECT_EQ(gfx::Point(78, 96), GetOrigin(kEmptyAnchor, kContentsSize)); On 2014/08/11 22:26:33, msw wrote: > Ditto; can you make this more robust with something like: > const int kAnchorOffset = x; > ... > EXPECT_EQ(kAnchorOffset + kEmptyAnchor.x(), GetOrigin(kEmptyAnchor).x()); Done.
I truly appreciate your help with this fix and your rigorous testing. Unfortunately, I have some heavy-handed comments that should yield fewer changes to the BubbleBorder class surface and more maintainable test code. If you want to land the fix first and pursue tests in a follow-up CL, that would be totally fine. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... File ui/views/bubble/bubble_border.h (right): https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:161: // Get the arrow thickness. Expanding unit tests shouldn't expand BubbleBorder's API, especially the public surface. Instead of adding all these public accessors that aren't otherwise needed, you should add a private BorderImages* GetImagesForTest() accessor that just returns images_ (for direct access to BorderImages metrics), and a new private GetArrowSize, then add FRIEND_TEST_ALL_PREFIXES for the needed tests. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:174: int GetBorderExteriorThickness() const; nit: revert this name change when you remove the accessors. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:199: static const int kStroke; statics should be declared before functions as per the style-guide. Move this between "friend class BubbleBorderTest" and "...GetSizeForContentsSize..." with a blank line above and one below). https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:19: const gfx::Rect kGetOriginAnchor = gfx::Rect(100, 100, 20, 20); nit: rename this kAnchor. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:20: const gfx::Size kGetOriginContentsSize = gfx::Size(50, 50); nit: remove this and just use kMediumSize. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:77: // Gets an origin point for the fixed kGetOriginAnchor and This helper can be re-written as a static for individual test |border|s: static gfx::Point GetOrigin(const BubbleBorder& border) { border.GetBounds(kAnchor, kMediumSize); } (it could also be a file-local static in an anon namespace if you prefer) https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:95: bubble_border_.reset(new BubbleBorder(arrow, Have individual tests just do this as needed: const gfx::Size size(border->GetSizeForContentsSize(kMediumSize)); const int arrow_offset = border->GetArrowOffset(size); https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:103: scoped_ptr<BubbleBorder> bubble_border_; Remove this and wrappers; let each test construct and use a BubbleBorder: BubbleBorder border(arrow, BubbleBorder::NO_SHADOW, SK_ColorWHITE); <... use |border| directly ...> Add const kShadow and kColor (like kStroke) for |border|s, if you wish. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:308: ASSERT_TRUE(HasArrow()); Nit move asserts like this directly after the Border construction. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:354: TEST_F(BubbleBorderGetSizeForContentsSizeTest, WithHorizontalBottomArrow) { All the expected values should be the same for a top/bottom, left/right arrow, and none/float pairs, and there are other strong similarities between various cases. Can you remove some cases or consolidate the tests to avoid some duplication? Maybe something like this pattern, which I've used with success elsewhere: struct TestCase { BubbleBorder::Arrow arrow; BubbleBorder::ArrowPaintType paint_arrow; gfx::Size content; gfx::Size expected; }; const gfx::Size kSmallHorizArrow = ...; const gfx::Size kSmallVertArrow = ...; const gfx::Size kSmallNoArrow = ...; TestCase cases[] = { { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_NORMAL, kSmall, kSmallHorizArrow }, { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_TRANSPARENT, kSmall, kSmallHorizArrow }, { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_NONE, kSmall, kSmallNoArrow }, { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_NORMAL, kSmall, kSmallHorizArrow }, { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_TRANSPARENT, kSmall, kSmallHorizArrow }, { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_NONE, kSmall, kSmallNoArrow }, ... <Similar cases for vertical arrows, then ditto for all kMedium vert/horiz cases> }; for(size_t i = 0; i < arraysize(cases); ++i) { SCOPED_TRACE(...) border.set_arrow(cases[i].arrow); border.set_paint_arrow(cases[i].paint_arrow); EXPECT_EQ(expected, border.GetSizeForContentsSize(cases[i].content)); } https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:590: TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsHorizontalLeft) { Ditto request to remove similar test cases or consolidate a pattern.
Responded to some comments, will wait for some agreement on approach before fixing all comments. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... File ui/views/bubble/bubble_border.h (right): https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:161: // Get the arrow thickness. On 2014/08/19 21:35:39, msw wrote: > Expanding unit tests shouldn't expand BubbleBorder's API, especially the public > surface. Instead of adding all these public accessors that aren't otherwise > needed, you should add a private BorderImages* GetImagesForTest() accessor that > just returns images_ (for direct access to BorderImages metrics), and a new > private GetArrowSize, then add FRIEND_TEST_ALL_PREFIXES for the needed tests. The reason I didn't add a BorderImages* GetImagesForTest() originally is because the images_ type 'BorderImages' is a struct defined in the .cc file and not accessible in the tests. I thought about moving the definition to the .h file but it was in the 'internal' namespace and I wasn't sure if that was the right thing to do or not. If I moved it to the .h file it would also require #includes for gfx::ImageSkia. I was unfamiliar with the FRIEND_TEST_ALL_PREFIXES, thanks for pointing it out :) Initially I wanted to create an abstraction for the BorderImages type so that I could inject a test double that didn't actually depend on the ResourceBundle and didn't actually require a Painter, but breaking the Painter dependency would have required inverting the painting ownership from the BubbleBorder to the BorderImages. Ultimately I had decided this was too large a change for right now and had a questionable amount of return on investment. Perhaps the BorderImages definition should be added to an internal.h file which can be included in the tests. Thoughts? https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:354: TEST_F(BubbleBorderGetSizeForContentsSizeTest, WithHorizontalBottomArrow) { On 2014/08/19 21:35:40, msw wrote: > All the expected values should be the same for a top/bottom, left/right arrow, > and none/float pairs, and there are other strong similarities between various > cases. Can you remove some cases or consolidate the tests to avoid some > duplication? Maybe something like this pattern, which I've used with success > elsewhere: > > struct TestCase { > BubbleBorder::Arrow arrow; > BubbleBorder::ArrowPaintType paint_arrow; > gfx::Size content; > gfx::Size expected; > }; > > const gfx::Size kSmallHorizArrow = ...; > const gfx::Size kSmallVertArrow = ...; > const gfx::Size kSmallNoArrow = ...; > > TestCase cases[] = { > { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_NORMAL, kSmall, > kSmallHorizArrow }, > { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_TRANSPARENT, kSmall, > kSmallHorizArrow }, > { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_NONE, kSmall, kSmallNoArrow > }, > { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_NORMAL, kSmall, > kSmallHorizArrow }, > { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_TRANSPARENT, kSmall, > kSmallHorizArrow }, > { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_NONE, kSmall, > kSmallNoArrow }, > ... > <Similar cases for vertical arrows, then ditto for all kMedium vert/horiz > cases> > }; > > for(size_t i = 0; i < arraysize(cases); ++i) { > SCOPED_TRACE(...) > border.set_arrow(cases[i].arrow); > border.set_paint_arrow(cases[i].paint_arrow); > EXPECT_EQ(expected, border.GetSizeForContentsSize(cases[i].content)); > } Interesting, I stayed away from this approach because I was not familiar with the SCOPED_TRACE(...) macro and hated how you couldn't tell from the EXPECT_* output which test actually failed. This is very useful to know about, thanks!! :D
Thanks again for working on this testing. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... File ui/views/bubble/bubble_border.h (right): https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:161: // Get the arrow thickness. On 2014/08/20 15:01:28, bruthig wrote: > On 2014/08/19 21:35:39, msw wrote: > > Expanding unit tests shouldn't expand BubbleBorder's API, especially the > public > > surface. Instead of adding all these public accessors that aren't otherwise > > needed, you should add a private BorderImages* GetImagesForTest() accessor > that > > just returns images_ (for direct access to BorderImages metrics), and a new > > private GetArrowSize, then add FRIEND_TEST_ALL_PREFIXES for the needed tests. > > The reason I didn't add a BorderImages* GetImagesForTest() originally is because > the images_ type 'BorderImages' is a struct defined in the .cc file and not > accessible in the tests. I thought about moving the definition to the .h file > but it was in the 'internal' namespace and I wasn't sure if that was the right > thing to do or not. If I moved it to the .h file it would also require > #includes for gfx::ImageSkia. > > I was unfamiliar with the FRIEND_TEST_ALL_PREFIXES, thanks for pointing it out > :) > > Initially I wanted to create an abstraction for the BorderImages type so that I > could inject a test double that didn't actually depend on the ResourceBundle and > didn't actually require a Painter, but breaking the Painter dependency would > have required inverting the painting ownership from the BubbleBorder to the > BorderImages. Ultimately I had decided this was too large a change for right > now and had a questionable amount of return on investment. > > Perhaps the BorderImages definition should be added to an internal.h file which > can be included in the tests. > > Thoughts? Yeah, move the internal::BorderImages decl to this bubble_border.h. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:354: TEST_F(BubbleBorderGetSizeForContentsSizeTest, WithHorizontalBottomArrow) { On 2014/08/20 15:01:28, bruthig wrote: > On 2014/08/19 21:35:40, msw wrote: > > All the expected values should be the same for a top/bottom, left/right arrow, > > and none/float pairs, and there are other strong similarities between various > > cases. Can you remove some cases or consolidate the tests to avoid some > > duplication? Maybe something like this pattern, which I've used with success > > elsewhere: > > > > struct TestCase { > > BubbleBorder::Arrow arrow; > > BubbleBorder::ArrowPaintType paint_arrow; > > gfx::Size content; > > gfx::Size expected; > > }; > > > > const gfx::Size kSmallHorizArrow = ...; > > const gfx::Size kSmallVertArrow = ...; > > const gfx::Size kSmallNoArrow = ...; > > > > TestCase cases[] = { > > { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_NORMAL, kSmall, > > kSmallHorizArrow }, > > { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_TRANSPARENT, kSmall, > > kSmallHorizArrow }, > > { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_NONE, kSmall, > kSmallNoArrow > > }, > > { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_NORMAL, kSmall, > > kSmallHorizArrow }, > > { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_TRANSPARENT, kSmall, > > kSmallHorizArrow }, > > { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_NONE, kSmall, > > kSmallNoArrow }, > > ... > > <Similar cases for vertical arrows, then ditto for all kMedium vert/horiz > > cases> > > }; > > > > for(size_t i = 0; i < arraysize(cases); ++i) { > > SCOPED_TRACE(...) > > border.set_arrow(cases[i].arrow); > > border.set_paint_arrow(cases[i].paint_arrow); > > EXPECT_EQ(expected, border.GetSizeForContentsSize(cases[i].content)); > > } > > Interesting, I stayed away from this approach because I was not familiar with > the SCOPED_TRACE(...) macro and hated how you couldn't tell from the EXPECT_* > output which test actually failed. This is very useful to know about, thanks!! > :D Yeah, SCOPED_TRACE and looping over similar test cases is a great approach, you can find some examples in RenderTextTest and SearchProviderTest.
msw@ Tests have been reworked can you take another look :) https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... File ui/views/bubble/bubble_border.h (right): https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:161: // Get the arrow thickness. On 2014/08/20 20:41:02, msw wrote: > On 2014/08/20 15:01:28, bruthig wrote: > > On 2014/08/19 21:35:39, msw wrote: > > > Expanding unit tests shouldn't expand BubbleBorder's API, especially the > > public > > > surface. Instead of adding all these public accessors that aren't otherwise > > > needed, you should add a private BorderImages* GetImagesForTest() accessor > > that > > > just returns images_ (for direct access to BorderImages metrics), and a new > > > private GetArrowSize, then add FRIEND_TEST_ALL_PREFIXES for the needed > tests. > > > > The reason I didn't add a BorderImages* GetImagesForTest() originally is > because > > the images_ type 'BorderImages' is a struct defined in the .cc file and not > > accessible in the tests. I thought about moving the definition to the .h file > > but it was in the 'internal' namespace and I wasn't sure if that was the right > > thing to do or not. If I moved it to the .h file it would also require > > #includes for gfx::ImageSkia. > > > > I was unfamiliar with the FRIEND_TEST_ALL_PREFIXES, thanks for pointing it out > > :) > > > > Initially I wanted to create an abstraction for the BorderImages type so that > I > > could inject a test double that didn't actually depend on the ResourceBundle > and > > didn't actually require a Painter, but breaking the Painter dependency would > > have required inverting the painting ownership from the BubbleBorder to the > > BorderImages. Ultimately I had decided this was too large a change for right > > now and had a questionable amount of return on investment. > > > > Perhaps the BorderImages definition should be added to an internal.h file > which > > can be included in the tests. > > > > Thoughts? > > Yeah, move the internal::BorderImages decl to this bubble_border.h. Done. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:174: int GetBorderExteriorThickness() const; On 2014/08/19 21:35:39, msw wrote: > nit: revert this name change when you remove the accessors. Done. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:199: static const int kStroke; On 2014/08/19 21:35:39, msw wrote: > statics should be declared before functions as per the style-guide. Move this > between "friend class BubbleBorderTest" and "...GetSizeForContentsSize..." with > a blank line above and one below). Done. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:19: const gfx::Rect kGetOriginAnchor = gfx::Rect(100, 100, 20, 20); On 2014/08/19 21:35:40, msw wrote: > nit: rename this kAnchor. Done. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:20: const gfx::Size kGetOriginContentsSize = gfx::Size(50, 50); On 2014/08/19 21:35:40, msw wrote: > nit: remove this and just use kMediumSize. Done. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:354: TEST_F(BubbleBorderGetSizeForContentsSizeTest, WithHorizontalBottomArrow) { On 2014/08/20 20:41:02, msw wrote: > On 2014/08/20 15:01:28, bruthig wrote: > > On 2014/08/19 21:35:40, msw wrote: > > > All the expected values should be the same for a top/bottom, left/right > arrow, > > > and none/float pairs, and there are other strong similarities between > various > > > cases. Can you remove some cases or consolidate the tests to avoid some > > > duplication? Maybe something like this pattern, which I've used with success > > > elsewhere: > > > > > > struct TestCase { > > > BubbleBorder::Arrow arrow; > > > BubbleBorder::ArrowPaintType paint_arrow; > > > gfx::Size content; > > > gfx::Size expected; > > > }; > > > > > > const gfx::Size kSmallHorizArrow = ...; > > > const gfx::Size kSmallVertArrow = ...; > > > const gfx::Size kSmallNoArrow = ...; > > > > > > TestCase cases[] = { > > > { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_NORMAL, kSmall, > > > kSmallHorizArrow }, > > > { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_TRANSPARENT, kSmall, > > > kSmallHorizArrow }, > > > { BubbleBorder::TOP_CENTER, BubbleBorder::PAINT_NONE, kSmall, > > kSmallNoArrow > > > }, > > > { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_NORMAL, kSmall, > > > kSmallHorizArrow }, > > > { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_TRANSPARENT, kSmall, > > > kSmallHorizArrow }, > > > { BubbleBorder::BOTTOM_CENTER, BubbleBorder::PAINT_NONE, kSmall, > > > kSmallNoArrow }, > > > ... > > > <Similar cases for vertical arrows, then ditto for all kMedium > vert/horiz > > > cases> > > > }; > > > > > > for(size_t i = 0; i < arraysize(cases); ++i) { > > > SCOPED_TRACE(...) > > > border.set_arrow(cases[i].arrow); > > > border.set_paint_arrow(cases[i].paint_arrow); > > > EXPECT_EQ(expected, border.GetSizeForContentsSize(cases[i].content)); > > > } > > > > Interesting, I stayed away from this approach because I was not familiar with > > the SCOPED_TRACE(...) macro and hated how you couldn't tell from the EXPECT_* > > output which test actually failed. This is very useful to know about, > thanks!! > > :D > > Yeah, SCOPED_TRACE and looping over similar test cases is a great approach, you > can find some examples in RenderTextTest and SearchProviderTest. Done. https://chromiumcodereview.appspot.com/454173002/diff/20001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:590: TEST_F(BubbleBorderGetBoundsOriginTest, ArrowIsHorizontalLeft) { On 2014/08/19 21:35:40, msw wrote: > Ditto request to remove similar test cases or consolidate a pattern. Done.
https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... File ui/views/bubble/bubble_border.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.cc:10: #include "base/memory/scoped_ptr.h" nit: remove this it should be added to the header. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.cc:14: #include "ui/gfx/image/image_skia.h" nit: remove this, it's now in the header. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... File ui/views/bubble/bubble_border.h (right): https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:15: class ImageSkia; nit: remove forward decl. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:33: scoped_ptr<Painter> border_painter; nit: add a base/memory/scoped_ptr.h include https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:204: friend class BubbleBorderTest; Remove this (as per typedefing and/or moving kStroke use to test). https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:205: nit: remove blank line. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:217: internal::BorderImages* GetImagesForTest() const { nit: define this in the .cc file (to match CamelCase function name). https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:13: class BubbleBorderTest : public views::ViewsTestBase { Change this subclass to a typedef. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:15: const int kStroke = views::BubbleBorder::kStroke; Have GetBoundsOriginTest grab this instead. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:24: typedef BubbleBorderTest BubbleBorderGetBoundsOriginTest; Remove this. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:219: views::BubbleBorder bb(BubbleBorder::NONE, nit: abbreviations are discouraged, consider |border|. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:232: kImages->border_interior_thickness); nit: indent two more spaces. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:234: const gfx::Size kSmallVertArrow( nit: Since the content sizes are square, could this be simplified as kSmallVertArrow(kSmallHorizArrow.height(), kSmallHorizArrow.width()) (or just share one expected size and flip the width/height expectations based on is_arrow_on_horizontal). Ditto for Medium. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:236: + kImages->border_interior_thickness, nit: move '+' to line above, indent two more spaces. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:243: kMediumSize.width() + 2 * bb.GetBorderThickness(), Are the medium/small calculations different because the smaller bubble's content doesn't actually contribute to the bubble size? Can you add an short comment? https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:245: + kImages->arrow_thickness); nit: move '+' to line above, indent two more spaces. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:257: bool has_arrow; Remove this and the check; BubbleBorderTest.HasArrow covers this. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:266: // BubbleBorder::Arrow::TOP_CENTER nit: Remove these comments; they aren't as helpful as the others "// Content size: kSmallSize". https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:267: {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_NORMAL, nit: Sorry for iterating on my earlier suggested design, but it'd be better to make a test case simply contain the two expected sizes for the three paint types, and have the loop below check all three paint types for each test case. For example: { BubbleBorder::TOP_CENTER, kSmallSize, kSmallHorizArrow, kSmallNoArrow }, { BubbleBorder::BOTTOM_CENTER, kSmallSize, kSmallHorizArrow, kSmallNoArrow }, { BubbleBorder::LEFT_TOP, kSmallSize, kSmallVertArrow, kSmallNoArrow }, ... for (size_t i = 0; i < arraysize(cases); ++i) { ... bb.set_paint_arrow(BubbleBorder::PAINT_NORMAL); EXPECT_EQ(cases[i].expected_with_arrow, bb.GetSizeForContentsSize(cases[i].content)); bb.set_paint_arrow(BubbleBorder::PAINT_TRANSPARENT); EXPECT_EQ(cases[i].expected_with_arrow, bb.GetSizeForContentsSize(cases[i].content)); bb.set_paint_arrow(BubbleBorder::PAINT_NONE); EXPECT_EQ(cases[i].expected_without_arrow, bb.GetSizeForContentsSize(cases[i].content)); https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:269: {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_TRANSPARENT, nit: use spaces inside of curly braces. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:379: views::BubbleBorder bb(BubbleBorder::TOP_LEFT, ditto nit: |border| https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:405: const int kArrowOffsetForHorizCentre = kTotalSizeWithHorizArrow.width() / 2; nit: use the EN-US spelling "Center" https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:428: TestCase cases[] = { Ditto, if you see some way to simplify this test, it'd be appreciated. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:430: {BubbleBorder::TOP_LEFT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR, ditto nit: spaces inside braces.
msw@, I addressed most comments, a little unsure about the simplification for the BubbleBorderTest.GetBoundsOriginTest. Please see example and advise. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... File ui/views/bubble/bubble_border.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.cc:10: #include "base/memory/scoped_ptr.h" On 2014/08/22 20:54:44, msw wrote: > nit: remove this it should be added to the header. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.cc:14: #include "ui/gfx/image/image_skia.h" On 2014/08/22 20:54:44, msw wrote: > nit: remove this, it's now in the header. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... File ui/views/bubble/bubble_border.h (right): https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:15: class ImageSkia; On 2014/08/22 20:54:44, msw wrote: > nit: remove forward decl. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:33: scoped_ptr<Painter> border_painter; On 2014/08/22 20:54:44, msw wrote: > nit: add a base/memory/scoped_ptr.h include Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:204: friend class BubbleBorderTest; On 2014/08/22 20:54:44, msw wrote: > Remove this (as per typedefing and/or moving kStroke use to test). Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:205: On 2014/08/22 20:54:44, msw wrote: > nit: remove blank line. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border.h:217: internal::BorderImages* GetImagesForTest() const { On 2014/08/22 20:54:44, msw wrote: > nit: define this in the .cc file (to match CamelCase function name). Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:13: class BubbleBorderTest : public views::ViewsTestBase { On 2014/08/22 20:54:45, msw wrote: > Change this subclass to a typedef. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:15: const int kStroke = views::BubbleBorder::kStroke; On 2014/08/22 20:54:45, msw wrote: > Have GetBoundsOriginTest grab this instead. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:24: typedef BubbleBorderTest BubbleBorderGetBoundsOriginTest; On 2014/08/22 20:54:45, msw wrote: > Remove this. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:219: views::BubbleBorder bb(BubbleBorder::NONE, On 2014/08/22 20:54:45, msw wrote: > nit: abbreviations are discouraged, consider |border|. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:232: kImages->border_interior_thickness); On 2014/08/22 20:54:45, msw wrote: > nit: indent two more spaces. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:234: const gfx::Size kSmallVertArrow( On 2014/08/22 20:54:45, msw wrote: > nit: Since the content sizes are square, could this be simplified as > kSmallVertArrow(kSmallHorizArrow.height(), kSmallHorizArrow.width()) (or just > share one expected size and flip the width/height expectations based on > is_arrow_on_horizontal). Ditto for Medium. Actually I meant to change those sizes so they aren't square so as to avoid errors in the test. Fixed now and found 1 error! Simplifying only works for the kSmall case because the contents size is too small and a minimum value is used. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:236: + kImages->border_interior_thickness, On 2014/08/22 20:54:45, msw wrote: > nit: move '+' to line above, indent two more spaces. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:243: kMediumSize.width() + 2 * bb.GetBorderThickness(), On 2014/08/22 20:54:45, msw wrote: > Are the medium/small calculations different because the smaller bubble's content > doesn't actually contribute to the bubble size? Can you add an short comment? Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:245: + kImages->arrow_thickness); On 2014/08/22 20:54:45, msw wrote: > nit: move '+' to line above, indent two more spaces. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:257: bool has_arrow; On 2014/08/22 20:54:44, msw wrote: > Remove this and the check; BubbleBorderTest.HasArrow covers this. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:266: // BubbleBorder::Arrow::TOP_CENTER On 2014/08/22 20:54:44, msw wrote: > nit: Remove these comments; they aren't as helpful as the others "// Content > size: kSmallSize". Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:267: {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_NORMAL, On 2014/08/22 20:54:44, msw wrote: > nit: Sorry for iterating on my earlier suggested design, but it'd be better to > make a test case simply contain the two expected sizes for the three paint > types, and have the loop below check all three paint types for each test case. > For example: > { BubbleBorder::TOP_CENTER, kSmallSize, kSmallHorizArrow, kSmallNoArrow }, > { BubbleBorder::BOTTOM_CENTER, kSmallSize, kSmallHorizArrow, kSmallNoArrow }, > { BubbleBorder::LEFT_TOP, kSmallSize, kSmallVertArrow, kSmallNoArrow }, > ... > for (size_t i = 0; i < arraysize(cases); ++i) { > ... > bb.set_paint_arrow(BubbleBorder::PAINT_NORMAL); > EXPECT_EQ(cases[i].expected_with_arrow, > bb.GetSizeForContentsSize(cases[i].content)); > bb.set_paint_arrow(BubbleBorder::PAINT_TRANSPARENT); > EXPECT_EQ(cases[i].expected_with_arrow, > bb.GetSizeForContentsSize(cases[i].content)); > bb.set_paint_arrow(BubbleBorder::PAINT_NONE); > EXPECT_EQ(cases[i].expected_without_arrow, > bb.GetSizeForContentsSize(cases[i].content)); Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:269: {BubbleBorder::TOP_CENTER, true, BubbleBorder::PAINT_TRANSPARENT, On 2014/08/22 20:54:45, msw wrote: > nit: use spaces inside of curly braces. Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:379: views::BubbleBorder bb(BubbleBorder::TOP_LEFT, On 2014/08/22 20:54:45, msw wrote: > ditto nit: |border| Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:405: const int kArrowOffsetForHorizCentre = kTotalSizeWithHorizArrow.width() / 2; On 2014/08/22 20:54:45, msw wrote: > nit: use the EN-US spelling "Center" Done. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:428: TestCase cases[] = { On 2014/08/22 20:54:45, msw wrote: > Ditto, if you see some way to simplify this test, it'd be appreciated. The existing tests exhaust all the code paths through the function and are not exhaustive of all the input parameters. I can't think of a way to simplify it unless I add all input parameter combinations. See new patch for example. https://chromiumcodereview.appspot.com/454173002/diff/40001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:430: {BubbleBorder::TOP_LEFT, BubbleBorder::ALIGN_ARROW_TO_MID_ANCHOR, On 2014/08/22 20:54:44, msw wrote: > ditto nit: spaces inside braces. Done. https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:369: const int kTodo = 0; Let me know if you like this simplification and I will fill in the correct values for all the kTodo's.
https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... File ui/views/bubble/bubble_border.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... ui/views/bubble/bubble_border.cc:338: return images_; nit: indent only 2 spaces. https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... ui/views/bubble/bubble_border.cc:341: void BubbleBackground::Paint(gfx::Canvas* canvas, views::View* view) const { optional nit: reorder this definition to match its decl. https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:227: kSmallHorizArrow.width()); nit: indent to match open paren. https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:369: const int kTodo = 0; On 2014/08/22 22:55:04, bruthig wrote: > Let me know if you like this simplification and I will fill in the correct > values for all the kTodo's. This doesn't seem like a big win, I'll leave it up to you.
Just looking for clarification on one comment. Stay tuned for other fixes. https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... File ui/views/bubble/bubble_border.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... ui/views/bubble/bubble_border.cc:341: void BubbleBackground::Paint(gfx::Canvas* canvas, views::View* view) const { On 2014/08/23 00:29:12, msw wrote: > optional nit: reorder this definition to match its decl. Not exactly sure what you mean here? This is the inner class BubbleBackground's Paint which is declared at the bottom in the .h file.
https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... File ui/views/bubble/bubble_border.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... ui/views/bubble/bubble_border.cc:341: void BubbleBackground::Paint(gfx::Canvas* canvas, views::View* view) const { On 2014/08/23 00:39:45, bruthig wrote: > On 2014/08/23 00:29:12, msw wrote: > > optional nit: reorder this definition to match its decl. > > Not exactly sure what you mean here? This is the inner class BubbleBackground's > Paint which is declared at the bottom in the .h file. Ah, my mistake, ignore that comment.
msw@, can you take another look? https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... File ui/views/bubble/bubble_border.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... ui/views/bubble/bubble_border.cc:338: return images_; On 2014/08/23 00:29:12, msw wrote: > nit: indent only 2 spaces. Done. https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/60001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:227: kSmallHorizArrow.width()); On 2014/08/23 00:29:12, msw wrote: > nit: indent to match open paren. Done.
It's a shame that GetBoundsOriginTest is so complex, but this lgtm with nits. https://chromiumcodereview.appspot.com/454173002/diff/80001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/80001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:340: const int kMidAnchorWidth = kAnchor.width() / 2; nit: make this kMidAnchorX for simplicity, ditto for Height/Y. https://chromiumcodereview.appspot.com/454173002/diff/80001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:364: int expectedY; nit: |expected_x|, ditto for y. https://chromiumcodereview.appspot.com/454173002/diff/80001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:370: kAnchor.x() + kMidAnchorWidth - kArrowOffsetForNotCenter, nit: indent to align with "Bubble" above; ditto elsewhere.
Minor nit fixes, no review required. https://chromiumcodereview.appspot.com/454173002/diff/80001/ui/views/bubble/b... File ui/views/bubble/bubble_border_unittest.cc (right): https://chromiumcodereview.appspot.com/454173002/diff/80001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:340: const int kMidAnchorWidth = kAnchor.width() / 2; On 2014/08/25 18:01:45, msw wrote: > nit: make this kMidAnchorX for simplicity, ditto for Height/Y. Done. https://chromiumcodereview.appspot.com/454173002/diff/80001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:364: int expectedY; On 2014/08/25 18:01:45, msw wrote: > nit: |expected_x|, ditto for y. Done. https://chromiumcodereview.appspot.com/454173002/diff/80001/ui/views/bubble/b... ui/views/bubble/bubble_border_unittest.cc:370: kAnchor.x() + kMidAnchorWidth - kArrowOffsetForNotCenter, On 2014/08/25 18:01:45, msw wrote: > nit: indent to align with "Bubble" above; ditto elsewhere. Done.
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bruthig@chromium.org/454173002/100001
The CQ bit was unchecked by bruthig@chromium.org
https://codereview.chromium.org/454173002/diff/80001/ui/views/bubble/bubble_b... File ui/views/bubble/bubble_border_unittest.cc (right): https://codereview.chromium.org/454173002/diff/80001/ui/views/bubble/bubble_b... ui/views/bubble/bubble_border_unittest.cc:340: const int kMidAnchorWidth = kAnchor.width() / 2; On 2014/08/25 18:22:21, bruthig wrote: > On 2014/08/25 18:01:45, msw wrote: > > nit: make this kMidAnchorX for simplicity, ditto for Height/Y. > > Done. Actually, remove these and do these replacements below: s/kAnchor.x() + kMidAnchorX/kAnchor.CenterPoint().x()/ s/kAnchor.y() + kMidAnchorY/kAnchor.CenterPoint().y()/ https://codereview.chromium.org/454173002/diff/100001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border_unittest.cc (right): https://codereview.chromium.org/454173002/diff/100001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border_unittest.cc:371: kTopHorizArrowY }, nit: fits on the line above, ditto for some lines below.
- Merged with tip of tree - Replaced kMidAnchorX and kMidAnchorY - Fixed line wrappings https://codereview.chromium.org/454173002/diff/80001/ui/views/bubble/bubble_b... File ui/views/bubble/bubble_border_unittest.cc (right): https://codereview.chromium.org/454173002/diff/80001/ui/views/bubble/bubble_b... ui/views/bubble/bubble_border_unittest.cc:340: const int kMidAnchorWidth = kAnchor.width() / 2; On 2014/08/25 18:48:17, msw wrote: > On 2014/08/25 18:22:21, bruthig wrote: > > On 2014/08/25 18:01:45, msw wrote: > > > nit: make this kMidAnchorX for simplicity, ditto for Height/Y. > > > > Done. > > Actually, remove these and do these replacements below: > s/kAnchor.x() + kMidAnchorX/kAnchor.CenterPoint().x()/ > s/kAnchor.y() + kMidAnchorY/kAnchor.CenterPoint().y()/ Done. https://codereview.chromium.org/454173002/diff/100001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_border_unittest.cc (right): https://codereview.chromium.org/454173002/diff/100001/ui/views/bubble/bubble_... ui/views/bubble/bubble_border_unittest.cc:371: kTopHorizArrowY }, On 2014/08/25 18:48:17, msw wrote: > nit: fits on the line above, ditto for some lines below. Done.
lgtm
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bruthig@chromium.org/454173002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by bruthig@chromium.org
bruthig@chromium.org changed reviewers: + jennyz@chromium.org
jennyz@, can you review changes in web_notification_tray.cc? https://codereview.chromium.org/454173002/diff/160001/ash/system/web_notifica... File ash/system/web_notification/web_notification_tray.cc (right): https://codereview.chromium.org/454173002/diff/160001/ash/system/web_notifica... ash/system/web_notification/web_notification_tray.cc:95: bubble_view->SetArrowPaintType(views::BubbleBorder::PAINT_NONE); My addition of calling UpdateBubble() in TrayBubbleView::SetArrowPaintType(ArrowPaintType) was causing the WebNotificationTrayTest.WebNotificationPopupBubble test to seg fault. This was happening because a BubbleDelegateView was accessing a null container widget to compute it's bound before the widget was set.
lgtm
The CQ bit was checked by bruthig@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bruthig@chromium.org/454173002/160001
Message was sent while issue was closed.
Committed patchset #9 (160001) as 8ecab891fc6b510d526ec5592f4c0554b95bdb7f
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/8bbc59b53df13d6e88cb4f97ced210324b745356 Cr-Commit-Position: refs/heads/master@{#292145} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
