Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
417153002:
Avoid passing uninitialized value to markRectAsNonOpaque. (Closed)
DescriptionAvoid passing uninitialized value to markRectAsNonOpaque.
While applying opaque region for layer if device clip is not a rect
we don't alter the opaque rect, and if we have a non-opaque
preserving transfer mode along with it, we mark the opaque rect
as empty.
BUG=390999
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179741
Patch Set 1 #Patch Set 2 : review comments addressed. #
Total comments: 4
Patch Set 3 : review comments + test #
Total comments: 5
Patch Set 4 : Tests Updated. #
Total comments: 8
Patch Set 5 : review comments addressed. #
Total comments: 6
Patch Set 6 : tests updated. #
Total comments: 8
Patch Set 7 : review comments addressed. #
Total comments: 10
Patch Set 8 : updated tests. #
Messages
Total messages: 42 (1 generated)
Linux_msan_chrome nags about this uninitialized value being sent, hence the changes. PTAL, thanks.
On 2014/07/25 11:35:38, sohanjg wrote: > Linux_msan_chrome nags about this uninitialized value being sent, hence the > changes. > PTAL, thanks. Can you just init the Rect to empty instead?
On 2014/07/27 11:28:37, danakj_OOO_until_Aug_3 wrote:
> On 2014/07/25 11:35:38, sohanjg wrote:
> > Linux_msan_chrome nags about this uninitialized value being sent, hence the
> > changes.
> > PTAL, thanks.
>
> Can you just init the Rect to empty instead?
Maybe we just shouldn't be making this call if deviceClipIsARect is false, since
the rect is
never initialized in that case. i.e., move the
if (!deviceClipIsARect)
return;
above the if-clause at 342. Would that work?
On 2014/07/27 15:31:45, Stephen White wrote: > On 2014/07/27 11:28:37, danakj_OOO_until_Aug_3 wrote: > > On 2014/07/25 11:35:38, sohanjg wrote: > > > Linux_msan_chrome nags about this uninitialized value being sent, hence the > > > changes. > > > PTAL, thanks. > > > > Can you just init the Rect to empty instead? > > Maybe we just shouldn't be making this call if deviceClipIsARect is false, since > the rect is > never initialized in that case. i.e., move the > > if (!deviceClipIsARect) > return; > > above the if-clause at 342. Would that work? Looking closer, it should actually go even higher, above the first access to deviceClipRect at 334. But if it's important that the rect be marked non-opaque in this case (when we have a non-opaque-preserving xfermode and we don't have a rect clip) we should probably just mark the whole device bounds as non-opaque. At least that's my guess.
On 2014/07/27 15:45:12, Stephen White wrote: > On 2014/07/27 15:31:45, Stephen White wrote: > > On 2014/07/27 11:28:37, danakj_OOO_until_Aug_3 wrote: > > > On 2014/07/25 11:35:38, sohanjg wrote: > > > > Linux_msan_chrome nags about this uninitialized value being sent, hence > the > > > > changes. > > > > PTAL, thanks. > > > > > > Can you just init the Rect to empty instead? > > > > Maybe we just shouldn't be making this call if deviceClipIsARect is false, > since > > the rect is > > never initialized in that case. i.e., move the > > > > if (!deviceClipIsARect) > > return; > > > > above the if-clause at 342. Would that work? > > Looking closer, it should actually go even higher, above the first access to > deviceClipRect at 334. > > But if it's important that the rect be marked non-opaque in this case (when we > have a non-opaque-preserving > xfermode and we don't have a rect clip) we should probably just mark the whole > device bounds as non-opaque. > At least that's my guess. I agree. I'd say three things 1. GetDeviceClipAsRect should return an empty Rect if it returns false. 2. The early out if the Rect is empty should only be if deviceClipIsARect is true 3. If we have a none opaque a preserving transfer mode and deviceClipIsARect is false, we should markAllNonOpaque. 4. We should unit test this scenario.
On 2014/07/27 16:27:20, danakj_OOO_until_Aug_3 wrote: > On 2014/07/27 15:45:12, Stephen White wrote: > > On 2014/07/27 15:31:45, Stephen White wrote: > > > On 2014/07/27 11:28:37, danakj_OOO_until_Aug_3 wrote: > > > > On 2014/07/25 11:35:38, sohanjg wrote: > > > > > Linux_msan_chrome nags about this uninitialized value being sent, hence > > the > > > > > changes. > > > > > PTAL, thanks. > > > > > > > > Can you just init the Rect to empty instead? > > > > > > Maybe we just shouldn't be making this call if deviceClipIsARect is false, > > since > > > the rect is > > > never initialized in that case. i.e., move the > > > > > > if (!deviceClipIsARect) > > > return; > > > > > > above the if-clause at 342. Would that work? > > > > Looking closer, it should actually go even higher, above the first access to > > deviceClipRect at 334. > > > > But if it's important that the rect be marked non-opaque in this case (when we > > have a non-opaque-preserving > > xfermode and we don't have a rect clip) we should probably just mark the whole > > device bounds as non-opaque. > > At least that's my guess. > > I agree. I'd say three things > 1. GetDeviceClipAsRect should return an empty Rect if it returns false. > 2. The early out if the Rect is empty should only be if deviceClipIsARect is > true > 3. If we have a none opaque a preserving transfer mode and deviceClipIsARect is > false, we should markAllNonOpaque. > 4. We should unit test this scenario. Can you please take a look. Can you please suggest, where we can add test for this code, I am not too familiar with it. Thanks.
Existing tests are here https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://codereview.chromium.org/417153002/diff/20001/Source/platform/graphics... File Source/platform/graphics/skia/OpaqueRegionSkia.cpp (right): https://codereview.chromium.org/417153002/diff/20001/Source/platform/graphics... Source/platform/graphics/skia/OpaqueRegionSkia.cpp:336: if (deviceClipRect.isEmpty() && deviceClipIsARect) reverse this order https://codereview.chromium.org/417153002/diff/20001/Source/platform/graphics... Source/platform/graphics/skia/OpaqueRegionSkia.cpp:345: markAllAsNonOpaque(); if it is a rect, you need to still do what we did before. if it's not you need to return after marking all.
Please take a look, thanks. https://codereview.chromium.org/417153002/diff/20001/Source/platform/graphics... File Source/platform/graphics/skia/OpaqueRegionSkia.cpp (right): https://codereview.chromium.org/417153002/diff/20001/Source/platform/graphics... Source/platform/graphics/skia/OpaqueRegionSkia.cpp:336: if (deviceClipRect.isEmpty() && deviceClipIsARect) On 2014/07/28 14:03:57, danakj_OOO_until_Aug_3 wrote: > reverse this order Done. https://codereview.chromium.org/417153002/diff/20001/Source/platform/graphics... Source/platform/graphics/skia/OpaqueRegionSkia.cpp:345: markAllAsNonOpaque(); On 2014/07/28 14:03:56, danakj_OOO_until_Aug_3 wrote: > if it is a rect, you need to still do what we did before. > > if it's not you need to return after marking all. Done.
https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1047: SkBitmap bitmap; What makes the device clip not a Rect in this test? https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... File Source/platform/graphics/skia/OpaqueRegionSkia.cpp (left): https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... Source/platform/graphics/skia/OpaqueRegionSkia.cpp:345: if (!deviceClipIsARect) We've lost this early out in some cases now. Can you fix that and add a test case for it?
https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1047: SkBitmap bitmap; On 2014/07/30 13:31:25, danakj_OOO_until_Aug_3 wrote: > What makes the device clip not a Rect in this test? This is still a Rect, i wonder how can we set device clip as non rect ? This test will only cover markRectAsNonOpaque(deviceClipRect) code, and not the markAllAsNonOpaque() code.
Set the clip with a path? On Jul 30, 2014 3:41 PM, <sohan.jyoti@samsung.com> wrote: > > https://codereview.chromium.org/417153002/diff/40001/ > Source/platform/graphics/GraphicsContextTest.cpp > File Source/platform/graphics/GraphicsContextTest.cpp (right): > > https://codereview.chromium.org/417153002/diff/40001/ > Source/platform/graphics/GraphicsContextTest.cpp#newcode1047 > Source/platform/graphics/GraphicsContextTest.cpp:1047: SkBitmap bitmap; > On 2014/07/30 13:31:25, danakj_OOO_until_Aug_3 wrote: > >> What makes the device clip not a Rect in this test? >> > > This is still a Rect, i wonder how can we set device clip as non rect ? > This test will only cover markRectAsNonOpaque(deviceClipRect) code, and > not the markAllAsNonOpaque() code. > > https://codereview.chromium.org/417153002/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Yea, setting device clip with path works well. Please take a look. Thanks. https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1047: SkBitmap bitmap; On 2014/07/30 13:41:54, sohanjg wrote: > On 2014/07/30 13:31:25, danakj_OOO_until_Aug_3 wrote: > > What makes the device clip not a Rect in this test? > > This is still a Rect, i wonder how can we set device clip as non rect ? > This test will only cover markRectAsNonOpaque(deviceClipRect) code, and not the > markAllAsNonOpaque() code. Done. https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... File Source/platform/graphics/skia/OpaqueRegionSkia.cpp (left): https://codereview.chromium.org/417153002/diff/40001/Source/platform/graphics... Source/platform/graphics/skia/OpaqueRegionSkia.cpp:345: if (!deviceClipIsARect) On 2014/07/30 13:31:26, danakj_OOO_until_Aug_3 wrote: > We've lost this early out in some cases now. Can you fix that and add a test > case for it? Done.
danakj@, senorblanco@ if you may please provide your feedback on this. thanks!
On 2014/08/04 16:20:20, sohanjg wrote: > danakj@, senorblanco@ if you may please provide your feedback on this. thanks! Looks good, but I'll leave for Dana to take a look.
https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1052: SkCanvas canvas(50, 50); can you make the canvas bigger than the path's bounding box, or make the path smaller than the canvas? https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1059: // Set Opaque Rect. Say why not what, all of these comments just say what the code is doing. Instead they should explain why they are doing what they are doing. https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1060: context.fillRect(FloatRect(30, 30, 90, 90), opaque, CompositeSourceOver); isn't this outside the bounds of the canvas?
https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... File Source/platform/graphics/RegionTracker.cpp (left): https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... Source/platform/graphics/RegionTracker.cpp:367: if (!deviceClipIsARect) What if !deviceClipIsARect but outsideSourceOpaqueRectPreservesOpaque? We need this early out in that case still.
please take a look,thanks. https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1052: SkCanvas canvas(50, 50); On 2014/08/05 13:53:28, danakj wrote: > can you make the canvas bigger than the path's bounding box, or make the path > smaller than the canvas? Done. https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1059: // Set Opaque Rect. On 2014/08/05 13:53:28, danakj wrote: > Say why not what, all of these comments just say what the code is doing. Instead > they should explain why they are doing what they are doing. Done. https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1060: context.fillRect(FloatRect(30, 30, 90, 90), opaque, CompositeSourceOver); On 2014/08/05 13:53:28, danakj wrote: > isn't this outside the bounds of the canvas? Done. I was trying to avoid markRectAsNonOpaque early-outs and get a opaque region, but that's not imp in this test i gess. https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... File Source/platform/graphics/RegionTracker.cpp (left): https://codereview.chromium.org/417153002/diff/60001/Source/platform/graphics... Source/platform/graphics/RegionTracker.cpp:367: if (!deviceClipIsARect) On 2014/08/05 13:54:15, danakj wrote: > What if !deviceClipIsARect but outsideSourceOpaqueRectPreservesOpaque? We need > this early out in that case still. Done. added a test too.
https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1060: context.fillRect(FloatRect(30, 30, 90, 90), opaque, CompositeSourceOver); Can you EXPECT_EQ the opaqueRegion after this, and verify it's not empty so we see the code below actually doing something? https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1086: context2.fillRect(FloatRect(30, 30, 90, 90), opaque, CompositeSourceOver); Can you EXPECT_EQ the opaqueRegion after this, and verify it's not empty so we see the code below actually doing something? Also how does this part differ from the above other than using a bitmap? Do we need both or is just this one enough?
https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1086: context2.fillRect(FloatRect(30, 30, 90, 90), opaque, CompositeSourceOver); On 2014/08/05 15:26:54, danakj wrote: > Can you EXPECT_EQ the opaqueRegion after this, and verify it's not empty so we > see the code below actually doing something? > > Also how does this part differ from the above other than using a bitmap? Do we > need both or is just this one enough? this part has device clip as 'rect', we would need to test that too, right ? or it can be ignored ?
https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1086: context2.fillRect(FloatRect(30, 30, 90, 90), opaque, CompositeSourceOver); On 2014/08/05 16:48:12, sohanjg wrote: > On 2014/08/05 15:26:54, danakj wrote: > > Can you EXPECT_EQ the opaqueRegion after this, and verify it's not empty so we > > see the code below actually doing something? > > > > Also how does this part differ from the above other than using a bitmap? Do we > > need both or is just this one enough? > > this part has device clip as 'rect', we would need to test that too, right ? or > it can be ignored ? Oh, this is a different context, i missed that "2". Can you split this into a separate test case?
please take a look, thanks! https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1060: context.fillRect(FloatRect(30, 30, 90, 90), opaque, CompositeSourceOver); On 2014/08/05 15:26:54, danakj wrote: > Can you EXPECT_EQ the opaqueRegion after this, and verify it's not empty so we > see the code below actually doing something? Done. https://codereview.chromium.org/417153002/diff/80001/Source/platform/graphics... Source/platform/graphics/GraphicsContextTest.cpp:1086: context2.fillRect(FloatRect(30, 30, 90, 90), opaque, CompositeSourceOver); On 2014/08/05 17:18:34, danakj wrote: > On 2014/08/05 16:48:12, sohanjg wrote: > > On 2014/08/05 15:26:54, danakj wrote: > > > Can you EXPECT_EQ the opaqueRegion after this, and verify it's not empty so > we > > > see the code below actually doing something? > > > > > > Also how does this part differ from the above other than using a bitmap? Do > we > > > need both or is just this one enough? > > > > this part has device clip as 'rect', we would need to test that too, right ? > or > > it can be ignored ? > > Oh, this is a different context, i missed that "2". Can you split this into a > separate test case? Done.
Thanks, sorry, I think the tests are close not need a bit of care to ensure they are really testing what they claim to be. https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1074: If the clip was a rect, what about this begin/end layer would have made a non-empty opaque rect? https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1107: // we will intersect device clip rect with src opaque rect. Can you change the inputs so that we see the intersection happen by some numbers changing? I don't see any clip here at all?
https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1074: On 2014/08/06 13:53:34, danakj wrote: > If the clip was a rect, what about this begin/end layer would have made a > non-empty opaque rect? yes, for rect clip, we would have got a non-empty opaque rect, but we would need a new context for that. shud we add 1 more test for that ?
https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1074: On 2014/08/06 14:09:26, sohanjg wrote: > On 2014/08/06 13:53:34, danakj wrote: > > If the clip was a rect, what about this begin/end layer would have made a > > non-empty opaque rect? > > yes, for rect clip, we would have got a non-empty opaque rect, but we would need > a new context for that. > shud we add 1 more test for that ? I don't understand, the opaque rect is non-empty? But I see no fillRect or anything of the sort here to drwa something opaque? Why do you need another context?
https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1074: On 2014/08/06 14:17:28, danakj wrote: > On 2014/08/06 14:09:26, sohanjg wrote: > > On 2014/08/06 13:53:34, danakj wrote: > > > If the clip was a rect, what about this begin/end layer would have made a > > > non-empty opaque rect? > > > > yes, for rect clip, we would have got a non-empty opaque rect, but we would > need > > a new context for that. > > shud we add 1 more test for that ? > > I don't understand, the opaque rect is non-empty? But I see no fillRect or > anything of the sort here to drwa something opaque? Why do you need another > context? hmm..once we set context.clipPath, it seems we cannot have any opaque region even if do fillRect here. To get a opaque region we need a new context, with a rect device clip here.
https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1074: On 2014/08/06 14:30:07, sohanjg wrote: > On 2014/08/06 14:17:28, danakj wrote: > > On 2014/08/06 14:09:26, sohanjg wrote: > > > On 2014/08/06 13:53:34, danakj wrote: > > > > If the clip was a rect, what about this begin/end layer would have made a > > > > non-empty opaque rect? > > > > > > yes, for rect clip, we would have got a non-empty opaque rect, but we would > > need > > > a new context for that. > > > shud we add 1 more test for that ? > > > > I don't understand, the opaque rect is non-empty? But I see no fillRect or > > anything of the sort here to drwa something opaque? Why do you need another > > context? > > hmm..once we set context.clipPath, it seems we cannot have any opaque region > even if do fillRect here. > To get a opaque region we need a new context, with a rect device clip here. You can clipRect with kReplace_Op for instance?
sorry for the tests not being fully convincing, have updated them, please take a look. thanks. https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1074: On 2014/08/06 14:32:10, danakj wrote: > On 2014/08/06 14:30:07, sohanjg wrote: > > On 2014/08/06 14:17:28, danakj wrote: > > > On 2014/08/06 14:09:26, sohanjg wrote: > > > > On 2014/08/06 13:53:34, danakj wrote: > > > > > If the clip was a rect, what about this begin/end layer would have made > a > > > > > non-empty opaque rect? > > > > > > > > yes, for rect clip, we would have got a non-empty opaque rect, but we > would > > > need > > > > a new context for that. > > > > shud we add 1 more test for that ? > > > > > > I don't understand, the opaque rect is non-empty? But I see no fillRect or > > > anything of the sort here to drwa something opaque? Why do you need another > > > context? > > > > hmm..once we set context.clipPath, it seems we cannot have any opaque region > > even if do fillRect here. > > To get a opaque region we need a new context, with a rect device clip here. > > You can clipRect with kReplace_Op for instance? I have updated the test and used rect device clip in the beginning of the test, and later changed it by setting clippath, this addresses our test well. hope its ok. https://codereview.chromium.org/417153002/diff/100001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1107: // we will intersect device clip rect with src opaque rect. On 2014/08/06 13:53:33, danakj wrote: > Can you change the inputs so that we see the intersection happen by some numbers > changing? I don't see any clip here at all? Done.
Thanks, they look better but I still don't see an intersection happening? https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1060: nit: rm extra whitespace here https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1074: nit: rm extra whitespace here https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1107: nit: rm extra whitespace here https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1115: // we will intersect device clip rect with src opaque rect. You didn't set any device clip rect in this test, can you set one so we see this intersection happening? https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1118: nit: rm extra whitespace here
please take a look, thanks. https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... File Source/platform/graphics/GraphicsContextTest.cpp (right): https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1060: On 2014/08/07 12:49:55, danakj wrote: > nit: rm extra whitespace here Done. https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1074: On 2014/08/07 12:49:55, danakj wrote: > nit: rm extra whitespace here Done. https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1107: On 2014/08/07 12:49:55, danakj wrote: > nit: rm extra whitespace here Done. https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1115: // we will intersect device clip rect with src opaque rect. On 2014/08/07 12:49:56, danakj wrote: > You didn't set any device clip rect in this test, can you set one so we see this > intersection happening? Done. https://codereview.chromium.org/417153002/diff/120001/Source/platform/graphic... Source/platform/graphics/GraphicsContextTest.cpp:1118: On 2014/08/07 12:49:55, danakj wrote: > nit: rm extra whitespace here Done.
Thanks, LGTM
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/417153002/140001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...) mac_blink_compile_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: blink_presubmit on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
On 2014/08/07 16:12:02, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > blink_presubmit on tryserver.blink > (http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...) senorblanco@, this i think needs your formal lgtm, to land.
LGTM
The CQ bit was checked by sohan.jyoti@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/417153002/140001
Message was sent while issue was closed.
Change committed as 179741
Message was sent while issue was closed.
Description was changed from ========== Avoid passing uninitialized value to markRectAsNonOpaque. While applying opaque region for layer if device clip is not a rect we don't alter the opaque rect, and if we have a non-opaque preserving transfer mode along with it, we mark the opaque rect as empty. BUG=390999 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179741 ========== to ========== Avoid passing uninitialized value to markRectAsNonOpaque. While applying opaque region for layer if device clip is not a rect we don't alter the opaque rect, and if we have a non-opaque preserving transfer mode along with it, we mark the opaque rect as empty. BUG=390999 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=179741 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
