Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
150603004:
Fixed rounding issue on scrollbar rasterization. (Closed)
|
Created:
6 years, 10 months ago by bokan Modified:
6 years, 10 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFixed rounding issue on scrollbar rasterization.
Changed the painting method to translate the canvas back to the
origin before performing the scale to prevent losing pixels due
to rounding. Also made the it take the layer-space rect as a
parameter, instead of trying to reconstruct it from the
(possibly-clamped) content-rect, and calculate the layer-space
to content-space scaling factor directly from rects.
BUG=336534
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252254
Patch Set 1 #
Total comments: 2
Patch Set 2 : #Patch Set 3 : Added test #
Total comments: 6
Patch Set 4 : #
Total comments: 1
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 8
Patch Set 7 : Fixed scale #Patch Set 8 : Fixed Test #
Total comments: 10
Patch Set 9 : #
Total comments: 10
Patch Set 10 : #Patch Set 11 : Style fix #
Total comments: 2
Patch Set 12 : Removed ClearRenderSurface() #Patch Set 13 : Fixed break on Android tests #
Messages
Total messages: 40 (0 generated)
https://codereview.chromium.org/150603004/diff/1/cc/layers/painted_scrollbar_... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/1/cc/layers/painted_scrollbar_... cc/layers/painted_scrollbar_layer.cc:250: float adjustmentX = round(location_.x() * contents_scale_x()); Using location_ here looks wrong, the input |rect| is not always based on location_, how about when it's the OriginThumbRect()?
https://codereview.chromium.org/150603004/diff/1/cc/layers/painted_scrollbar_... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/1/cc/layers/painted_scrollbar_... cc/layers/painted_scrollbar_layer.cc:250: float adjustmentX = round(location_.x() * contents_scale_x()); On 2014/01/31 18:58:43, danakj wrote: > Using location_ here looks wrong, the input |rect| is not always based on > location_, how about when it's the OriginThumbRect()? yikes! You're right, I'll think about this some more...
What's the best way to test this? Seems like we'd want some kind of pixel test so that implies layout test, but this issue needs the compositor so we'd need a virtual test suite with the appropriate flags turned on. James, how is the rest of the pinch code tested?
On 2014/02/05 23:25:53, bokan wrote: > What's the best way to test this? Seems like we'd want some kind of pixel test > so that implies layout test, but this issue needs the compositor so we'd need a > virtual test suite with the appropriate flags turned on. James, how is the rest > of the pinch code tested? I would do this as a unit test. You can get some ideas here: https://codereview.chromium.org/15294018/diff/9001/cc/layers/scrollbar_layer_... Scrollbars and pixel tests are generally anathema to each other :-)
On 2014/02/06 00:11:48, wjmaclean wrote: > On 2014/02/05 23:25:53, bokan wrote: > > What's the best way to test this? Seems like we'd want some kind of pixel test > > so that implies layout test, but this issue needs the compositor so we'd need > a > > virtual test suite with the appropriate flags turned on. James, how is the > rest > > of the pinch code tested? > > I would do this as a unit test. You can get some ideas here: > > https://codereview.chromium.org/15294018/diff/9001/cc/layers/scrollbar_layer_... > > Scrollbars and pixel tests are generally anathema to each other :-) I think if we want to have a test for this bug there's no way around a pixel test (of some sort). We need to test that the painted output is actually correct. The test you linked just checks texture sizes. In this case, the texture dimensions would have been fine, it was an incorrectly truncated clip that caused the painter not to fill in part of the texture.
On 2014/02/07 14:37:43, bokan wrote: > On 2014/02/06 00:11:48, wjmaclean wrote: > > On 2014/02/05 23:25:53, bokan wrote: > > > What's the best way to test this? Seems like we'd want some kind of pixel > test > > > so that implies layout test, but this issue needs the compositor so we'd > need > > a > > > virtual test suite with the appropriate flags turned on. James, how is the > > rest > > > of the pinch code tested? > > > > I would do this as a unit test. You can get some ideas here: > > > > > https://codereview.chromium.org/15294018/diff/9001/cc/layers/scrollbar_layer_... > > > > Scrollbars and pixel tests are generally anathema to each other :-) > > I think if we want to have a test for this bug there's no way around a pixel > test (of some sort). We need to test that the painted output is actually > correct. The test you linked just checks texture sizes. In this case, the > texture dimensions would have been fine, it was an incorrectly truncated clip > that caused the painter not to fill in part of the texture. But then can't you just check the clip rect in the unit test? I'm not saying we can't have a pixel test, just that if it's not absolutely necessary, then we should avoid it.
On 2014/02/07 15:46:39, wjmaclean wrote: > On 2014/02/07 14:37:43, bokan wrote: > > On 2014/02/06 00:11:48, wjmaclean wrote: > > > On 2014/02/05 23:25:53, bokan wrote: > > > > What's the best way to test this? Seems like we'd want some kind of pixel > > test > > > > so that implies layout test, but this issue needs the compositor so we'd > > need > > > a > > > > virtual test suite with the appropriate flags turned on. James, how is the > > > rest > > > > of the pinch code tested? > > > > > > I would do this as a unit test. You can get some ideas here: > > > > > > > > > https://codereview.chromium.org/15294018/diff/9001/cc/layers/scrollbar_layer_... > > > > > > Scrollbars and pixel tests are generally anathema to each other :-) > > > > I think if we want to have a test for this bug there's no way around a pixel > > test (of some sort). We need to test that the painted output is actually > > correct. The test you linked just checks texture sizes. In this case, the > > texture dimensions would have been fine, it was an incorrectly truncated clip > > that caused the painter not to fill in part of the texture. > > But then can't you just check the clip rect in the unit test? I'm not saying we > can't have a pixel test, just that if it's not absolutely necessary, then we > should avoid it. I guess for this bug I could just make sure ScrollbarLayerRectToContentRect() returns the enclosedRect, though to actually avoid losing a pixel it'd need to also make sure the rasterization method used the enclosingRect - but that's internal to the method. Question: What is content_bound()? In ScrollbarLayerRectToContentRect() where we clamp the scaled rect size to content_bound, should that be a DCHECK instead (and clamp in Release builds)? I think the reason for the black line was that the rect was being clamped.
Ok, figured out a good way to test it (gMock is magical). PTAL
On 2014/02/07 23:21:16, bokan wrote: > Ok, figured out a good way to test it (gMock is magical). PTAL Cool :-) lgtm
Dana, PTAL when you get a chance (at this one and https://codereview.chromium.org/140413007/)
https://codereview.chromium.org/150603004/diff/150001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/150001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:172: gfx::Rect expanded_rect = gfx::ScaleToEnclosedRect( I don't believe this is correct. Say you have a scale of 1.5 and a width of 11. Then your content bounds width will be 17. If you take the enclosed here, you'll get only 16, so your width won't fill the entire content_bounds(). This is why we clamp to the content_bounds() after doing Enclosing on line 176. https://codereview.chromium.org/150603004/diff/150001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:253: gfx::Rect layer_rect = gfx::ScaleToEnclosingRect( It seems like this one is the one we should switch to enclosed? Or maybe you can explain what I'm missing there? Maybe we need to scale the skcanvas to the |rect| instead of using the contents_scale_x/y directly? https://codereview.chromium.org/150603004/diff/150001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/150603004/diff/150001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:838: TestScale(gfx::Rect(1117, 0, 15, 677), 7.301320); 7.301320f https://codereview.chromium.org/150603004/diff/150001/cc/test/fake_painted_sc... File cc/test/fake_painted_scrollbar_layer.h (right): https://codereview.chromium.org/150603004/diff/150001/cc/test/fake_painted_sc... cc/test/fake_painted_scrollbar_layer.h:22: FakeScrollbar* fake_scrollbar = 0); no default values allowed in chromium. you'll need to update the callsites or add a second CreateWithScrollbar(). And NULL instead of 0 please.
https://codereview.chromium.org/150603004/diff/150001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/150001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:253: gfx::Rect layer_rect = gfx::ScaleToEnclosingRect( On 2014/02/10 19:17:28, danakj wrote: > It seems like this one is the one we should switch to enclosed? Or maybe you can > explain what I'm missing there? > > Maybe we need to scale the skcanvas to the |rect| instead of using the > contents_scale_x/y directly? A possible nice change would be to pass in the layer rect instead of having to do this math here again, since the caller already had the layer rect.
I've changed the rasterization method to additionally take the layer_rect, avoiding the truncation issues all together. The "of-by-one" was still occurring though, I believe due to Skia rounding the scaled values rather than truncating (there's no docs for Skia so it's hard to tell but from what I can gather looking at the Skia code and running tests that my conclusion). Translating by the rounded amount fixes it. The problem now is that testing this is a little harder. I could check that the skCanvas passed to PaintPart has the correct translation on it but I thought I'd check if this is the right way to go before I do. Thanks https://codereview.chromium.org/150603004/diff/150001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/150001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:253: gfx::Rect layer_rect = gfx::ScaleToEnclosingRect( On 2014/02/10 19:17:28, danakj wrote: > It seems like this one is the one we should switch to enclosed? Or maybe you can > explain what I'm missing there? > > Maybe we need to scale the skcanvas to the |rect| instead of using the > contents_scale_x/y directly? If the one above is enclosed then it can be clipped which means that layer_rect * contents_scale_x != rect which is assumed here. I've changed to pass in the layer rect as you suggested. https://codereview.chromium.org/150603004/diff/270001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/270001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:173: layer_rect, contents_scale_x(), contents_scale_y()); Not that it matters since scale_x == scale_y, but I assume this was a typo?
Updated with previous feedback. The test ensures the SkCanvas's transformation matrix uses the rounded translation values.
still lgtm, see nits below. https://codereview.chromium.org/150603004/diff/320001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/150603004/diff/320001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:776: Delete extra line space? https://codereview.chromium.org/150603004/diff/320001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:880: int x = round(scrollbar_rect.x() * test_scale); Is it worth adding a comment here as to why you need this? Also, the names x,y could be more descriptive.
Dana, PTAL. https://codereview.chromium.org/150603004/diff/320001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/150603004/diff/320001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:776: On 2014/02/12 18:38:53, wjmaclean wrote: > Delete extra line space? Done. https://codereview.chromium.org/150603004/diff/320001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:880: int x = round(scrollbar_rect.x() * test_scale); On 2014/02/12 18:38:53, wjmaclean wrote: > Is it worth adding a comment here as to why you need this? Also, the names x,y > could be more descriptive. Done.
https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:256: // rounded origin coordinates Can you explain this logic? I was expecting you want change the scale slightly so that we draw to fill the skcanvas bitmap fully. Here it seems like you're instead just shifting the result left to the origin. But then we'll have a partial pixel on the right of the canvas that isn't painted still? How come that's okay? https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:257: float roundedX = round(layer_rect.x() * contents_scale_x()); rounded_x https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:258: float roundedY = round(layer_rect.y() * contents_scale_y()); rounded_y https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.h (right): https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.h:81: const gfx::Rect& rect, call this one content_rect so we can give it some context?
https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:256: // rounded origin coordinates On 2014/02/13 19:16:22, danakj wrote: > Can you explain this logic? I was expecting you want change the scale slightly > so that we draw to fill the skcanvas bitmap fully. > > Here it seems like you're instead just shifting the result left to the origin. > But then we'll have a partial pixel on the right of the canvas that isn't > painted still? How come that's okay? So it looked fine in practice, and I thought it was because the cases where we rounded were also the cases where we clamping the scaled_rect to content_bounds removed the last column from the bitmap. However, when I tried to prove it to myself I found that occasionally that's not true, and examining the memory in the bitmap confirmed that the right side was in fact clipped (though it's not visible, maybe the window border overlaps it?). tldr; I now scale the canvas such that the width/height is exactly the bitmap width/height and translate accordingly. Test is also better since it checks that all four corners are actually painted. https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:257: float roundedX = round(layer_rect.x() * contents_scale_x()); On 2014/02/13 19:16:22, danakj wrote: > rounded_x Done. https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:258: float roundedY = round(layer_rect.y() * contents_scale_y()); On 2014/02/13 19:16:22, danakj wrote: > rounded_y Done. https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.h (right): https://codereview.chromium.org/150603004/diff/420001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.h:81: const gfx::Rect& rect, On 2014/02/13 19:16:22, danakj wrote: > call this one content_rect so we can give it some context? Done.
Thanks! Some naming suggestions, and some more questions https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:242: float PaintedScrollbarLayer::calculateIdealScale( CalculateScaleToBitmap? make this a file-static method? It doesn't appear to benefit from being a member of the class. https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:246: int target_size) const { bitmap_size? https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:247: const float EPSILLON = 0.1f; kEpsilon https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:258: // Of course, due to the precission loss of floating point arithmetic, we'll precision https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:259: // likely be off by some epsillon which leads to edge cases when we're epsilon https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:260: // right around 0.5. To be safe, we adjust the scale by an epsillon depending epsilon https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:265: float scaled_layer_edge = layer_edge * content_scale; Can you explain this bit here? I'm a little confused by it. You're using the left/top edge to adjust the scale factor by 0.1/layer_size.. I'm not clear what situation you're trying to deal with here? https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:300: skcanvas.translate(SkFloatToScalar(-rounded_x), SkFloatToScalar(-rounded_y)); Ok so, I'm not sure why you need this translate. This is basically trying to pixel-snap the left/top edge of the thing you're drawing? But things that start at 0 will be scaled to 0 still, and then shifted left slightly?
Sorry about the review churn. I took a step back and realized I was over complicating this. I think the new patch actually makes sense. https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... File cc/layers/painted_scrollbar_layer.cc (right): https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:265: float scaled_layer_edge = layer_edge * content_scale; On 2014/02/14 20:38:31, danakj wrote: > Can you explain this bit here? I'm a little confused by it. You're using the > left/top edge to adjust the scale factor by 0.1/layer_size.. I'm not clear what > situation you're trying to deal with here? Sorry, I've been massively over-thinking this. The problem I was trying to solve was: Suppose left_edge*scale lands exactly on 10.5, it rounds up to 11. Say bitmap_size is 5, but scale*layer_size=4.9999 due to floating point rounding. The right edge will land on 15.49999 which will get rounded down to 15, 15-11=4 so we lost a pixel. In trying to explain my solution I realized it's wrong. All this was being complicated by the fact that we were scaling something that's not at the origin. The *obvious* solution is to translate to origin first, then scale. I've done that in a new patch, all this went away. https://codereview.chromium.org/150603004/diff/620001/cc/layers/painted_scrol... cc/layers/painted_scrollbar_layer.cc:300: skcanvas.translate(SkFloatToScalar(-rounded_x), SkFloatToScalar(-rounded_y)); On 2014/02/14 20:38:31, danakj wrote: > Ok so, I'm not sure why you need this translate. This is basically trying to > pixel-snap the left/top edge of the thing you're drawing? > > But things that start at 0 will be scaled to 0 still, and then shifted left > slightly? The PaintPart method for the track draws directly to the x/y offset of the scrollbar. We want the bitmap origin to be the scrollbar's top right corner so we need this translation to transform from unscaled-page coordinates into scaled scrollbar coordinates. In the new patch, I've flipped the transforms here so we translate to the origin before scaling. All this rounding nonsense is no longer necessary.
Ok thanks! this is much nicer. one more round of questions about the test code changes. https://codereview.chromium.org/150603004/diff/720001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/150603004/diff/720001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:787: FakePaintedScrollbarLayer::Create(true, false, layer_tree_root->id()); can you use temp bools here to give these boolean literals names? naming them the same as the function arguments would be nice. https://codereview.chromium.org/150603004/diff/720001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:817: UIResourceBitmap* bmp = layer_tree_host_->ui_resource_bitmap( |bitmap| https://codereview.chromium.org/150603004/diff/720001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:822: AutoLockUIResourceBitmap lockedBmp(*bmp); |locked_bitmap| https://codereview.chromium.org/150603004/diff/720001/cc/test/fake_scrollbar.cc File cc/test/fake_scrollbar.cc (right): https://codereview.chromium.org/150603004/diff/720001/cc/test/fake_scrollbar.... cc/test/fake_scrollbar.cc:72: canvas->drawRect(RectToSkRect(TrackRect()), paint); Hm.. why isn't this able to just paint the |content_rect| then? Should we be doing this for all parts not just TRACK? https://codereview.chromium.org/150603004/diff/720001/cc/test/fake_scrollbar.h File cc/test/fake_scrollbar.h (right): https://codereview.chromium.org/150603004/diff/720001/cc/test/fake_scrollbar.... cc/test/fake_scrollbar.h:39: SkColor get_color() const { return SK_ColorBLACK | fill_color_; } name this paint_fill_color() ?
https://codereview.chromium.org/150603004/diff/720001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/150603004/diff/720001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:787: FakePaintedScrollbarLayer::Create(true, false, layer_tree_root->id()); On 2014/02/18 20:43:30, danakj wrote: > can you use temp bools here to give these boolean literals names? naming them > the same as the function arguments would be nice. Done. https://codereview.chromium.org/150603004/diff/720001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:817: UIResourceBitmap* bmp = layer_tree_host_->ui_resource_bitmap( On 2014/02/18 20:43:30, danakj wrote: > |bitmap| Done. https://codereview.chromium.org/150603004/diff/720001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:822: AutoLockUIResourceBitmap lockedBmp(*bmp); On 2014/02/18 20:43:30, danakj wrote: > |locked_bitmap| Done. https://codereview.chromium.org/150603004/diff/720001/cc/test/fake_scrollbar.cc File cc/test/fake_scrollbar.cc (right): https://codereview.chromium.org/150603004/diff/720001/cc/test/fake_scrollbar.... cc/test/fake_scrollbar.cc:72: canvas->drawRect(RectToSkRect(TrackRect()), paint); On 2014/02/18 20:43:30, danakj wrote: > Hm.. why isn't this able to just paint the |content_rect| then? Should we be > doing this for all parts not just TRACK? The real ScrollbarImpl's PaintPart method only uses content_rect to draw the thumb. Everything else (there's really only TRACK and THUMB) uses the scrollbar's TrackRect() so this represents the real scrollbar painting code more accurately. (In fact, using content_rect here makes the test pass even if the bug is still in the code) I've changed the clear method below to fillRect the content_rect to be consistent with the real PaintPart for the thumb. https://codereview.chromium.org/150603004/diff/720001/cc/test/fake_scrollbar.h File cc/test/fake_scrollbar.h (right): https://codereview.chromium.org/150603004/diff/720001/cc/test/fake_scrollbar.... cc/test/fake_scrollbar.h:39: SkColor get_color() const { return SK_ColorBLACK | fill_color_; } On 2014/02/18 20:43:30, danakj wrote: > name this paint_fill_color() ? Done.
Thanks, LGTM https://codereview.chromium.org/150603004/diff/850001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/150603004/diff/850001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:847: scrollbar_layer->ClearRenderSurface(); Why is this needed? I dont see CalcDrawProps happening at all here.
https://codereview.chromium.org/150603004/diff/850001/cc/layers/scrollbar_lay... File cc/layers/scrollbar_layer_unittest.cc (right): https://codereview.chromium.org/150603004/diff/850001/cc/layers/scrollbar_lay... cc/layers/scrollbar_layer_unittest.cc:847: scrollbar_layer->ClearRenderSurface(); On 2014/02/18 21:55:55, danakj wrote: > Why is this needed? I dont see CalcDrawProps happening at all here. Removed. I had this left over from the test I copied. I thought I tried removing it but it caused a crash...it was probably a different call.
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bokan@chromium.org/150603004/900001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bokan@chromium.org/150603004/900001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bokan@chromium.org/150603004/900001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bokan@chromium.org/150603004/900001
The CQ bit was unchecked by bokan@chromium.org
The CQ bit was checked by bokan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bokan@chromium.org/150603004/1130004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bokan@chromium.org/150603004/1130004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bokan@chromium.org/150603004/1130004
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bokan@chromium.org/150603004/1130004
Message was sent while issue was closed.
Change committed as 252254 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
