Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
974603002:
Remove unittests based on wrong assumption of compositor (Closed)
|
Created:
5 years, 9 months ago by weiliangc Modified:
5 years, 9 months ago CC:
cc-bugs_chromium.org, chromium-reviews, danakj+watch_chromium.org, jbauman+watch_chromium.org, kalyank, oshima, piman+watch_chromium.org, sievers+watch_chromium.org, Ian Vollick Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unittests based on wrong assumption of compositor
Once impl-side-painting is on, assumptions on |ui::LayerDelegate|
regarding canvas size and scale reflecting exact damage rect and scale
would no longer hold true. This removes unittests that are based on
these assumptions.
R=enne
BUG=314185
Committed: https://crrev.com/d048acf227321848bc9652c393b94ea80833fe14
Cr-Commit-Position: refs/heads/master@{#319933}
Patch Set 1 #
Total comments: 11
Patch Set 2 : fix delegate layer size #
Total comments: 3
Patch Set 3 : clean up #
Total comments: 4
Patch Set 4 : address review comments #
Total comments: 4
Patch Set 5 : rebase #
Total comments: 1
Patch Set 6 : add comments #Messages
Total messages: 31 (7 generated)
lgtm in terms of assumptions about cc, but I'm not an owner.
weiliangc@chromium.org changed reviewers: + danakj@chromium.org, piman@chromium.org
danakj@ or piman@?
https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest.cc File ui/compositor/layer_unittest.cc (left): https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:532: EXPECT_EQ(delegate.paint_size(), gfx::Size(200, 200)); can you make the delegate bigger so that you can verify the rect isn't the full layer? i assume that is the point of the test that we're passing along what we paint not just the layer bounds always? https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:1330: EXPECT_EQ("2.0 2.0", root_delegate.ToScaleString()); Does the DSF no longer go to the delegate? This test is no longer testing anything either. I have similar comment to the other test. https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:1389: EXPECT_EQ("140x180", l1_delegate.paint_size().ToString()); This test isn't testing anything now. We should look at blame and see what bug this was testing and how we can change the test to guard regressions of that bug.
https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest.cc File ui/compositor/layer_unittest.cc (left): https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:532: EXPECT_EQ(delegate.paint_size(), gfx::Size(200, 200)); On 2015/03/02 23:03:09, danakj wrote: > can you make the delegate bigger so that you can verify the rect isn't the full > layer? i assume that is the point of the test that we're passing along what we > paint not just the layer bounds always? Done. https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:1330: EXPECT_EQ("2.0 2.0", root_delegate.ToScaleString()); On 2015/03/02 23:03:09, danakj wrote: > Does the DSF no longer go to the delegate? This test is no longer testing > anything either. I have similar comment to the other test. The ScaleString is from the canvas passed to LayerDelegate. I don't think this directly reflects DSF. https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:1389: EXPECT_EQ("140x180", l1_delegate.paint_size().ToString()); On 2015/03/02 23:03:09, danakj wrote: > This test isn't testing anything now. We should look at blame and see what bug > this was testing and how we can change the test to guard regressions of that > bug. The test was added when moving DIP translation to ui/compositor (https://codereview.chromium.org/10221028) Maybe this can still use to test DSF is passed along?
https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest.cc File ui/compositor/layer_unittest.cc (left): https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:1327: // Canvas size must have been scaled down up. You can remove this WaitForDraw() block entirely if it's not doing anything (I don't think it is?) https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:1348: WaitForDraw(); same here https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:1363: EXPECT_EQ("0x0", root_delegate.paint_size().ToString()); can you test "paint_count()" or something instead, and remove paint_size() and ToScaleString() entirely? https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:1389: EXPECT_EQ("140x180", l1_delegate.paint_size().ToString()); On 2015/03/03 16:35:44, weiliangc wrote: > On 2015/03/02 23:03:09, danakj wrote: > > This test isn't testing anything now. We should look at blame and see what bug > > this was testing and how we can change the test to guard regressions of that > > bug. > > The test was added when moving DIP translation to ui/compositor > (https://codereview.chromium.org/10221028) > > Maybe this can still use to test DSF is passed along? Thanks for digging. I think this test made sense to make sure painting happens at the right DSF (ie it uses images of the right scale factory in the recording). But now we just want to make sure the delegate's device_scale_factor() is right, so that it paints with images at the right scale. I Don't think the WaitForDraws() do anything for this test either, can you remove them, and comment near the EXPECTs on the device scale factor to show that these are what we care about and why? https://codereview.chromium.org/974603002/diff/20001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/974603002/diff/20001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(), gfx::Size(1030, 1030)); This bakes in cc internal knowledge into these numbers, I think it's enough to say that this on the paint Contains the layer, and the other ones, the paints do not Contain the layer. Also btw this was wrong here, but EXPECT(expected, actual), this is backward.
https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest.cc File ui/compositor/layer_unittest.cc (left): https://codereview.chromium.org/974603002/diff/1/ui/compositor/layer_unittest... ui/compositor/layer_unittest.cc:1363: EXPECT_EQ("0x0", root_delegate.paint_size().ToString()); On 2015/03/03 17:57:05, danakj wrote: > can you test "paint_count()" or something instead, and remove paint_size() and > ToScaleString() entirely? paint_size() can still be used for the later tests's "contains layer or not" I think. https://codereview.chromium.org/974603002/diff/20001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/974603002/diff/20001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(), gfx::Size(1030, 1030)); On 2015/03/03 17:57:05, danakj wrote: > This bakes in cc internal knowledge into these numbers, I think it's enough to > say that this on the paint Contains the layer, and the other ones, the paints do > not Contain the layer. > > Also btw this was wrong here, but EXPECT(expected, actual), this is backward. paint_size() still used here. (also time to test my regex power)
https://codereview.chromium.org/974603002/diff/20001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/974603002/diff/20001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(), gfx::Size(1030, 1030)); On 2015/03/03 at 17:57:05, danakj wrote: > This bakes in cc internal knowledge into these numbers, I think it's enough to say that this on the paint Contains the layer, and the other ones, the paints do not Contain the layer. > > Also btw this was wrong here, but EXPECT(expected, actual), this is backward. paint_size still needed here, since canvas rect won't have information for (top,left) position, the paint rect won't contain layer bounds w/o impl-side.
On 2015/03/06 20:42:46, weiliangc wrote: > https://codereview.chromium.org/974603002/diff/20001/ui/compositor/layer_unit... > File ui/compositor/layer_unittest.cc (right): > > https://codereview.chromium.org/974603002/diff/20001/ui/compositor/layer_unit... > ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(), > gfx::Size(1030, 1030)); > On 2015/03/03 at 17:57:05, danakj wrote: > > This bakes in cc internal knowledge into these numbers, I think it's enough to > say that this on the paint Contains the layer, and the other ones, the paints do > not Contain the layer. > > > > Also btw this was wrong here, but EXPECT(expected, actual), this is backward. > > paint_size still needed here, since canvas rect won't have information for > (top,left) position, the paint rect won't contain layer bounds w/o impl-side. Ya, ok fair shout. let's remove the checks against paint_size() entirely here? the color checks verify the delegate is being used.
Updated, no paint_size. PTAL.
LGTM https://codereview.chromium.org/974603002/diff/40001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/974603002/diff/40001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:498: CreateColorLayer(SK_ColorBLACK, gfx::Rect(20, 20, 1000, 1000))); no need to change this size now https://codereview.chromium.org/974603002/diff/40001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:508: l1->SchedulePaint(gfx::Rect(0, 0, 1000, 1000)); can just SchedulePaint the same rect for each of these cases now https://codereview.chromium.org/974603002/diff/40001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:1305: // right sacle. scale https://codereview.chromium.org/974603002/diff/40001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:1319: // right sacle. scale
sky@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/974603002/diff/60001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (left): https://codereview.chromium.org/974603002/diff/60001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(), l1->bounds().size()); It's still good to check the paint regions. Can you use a contains check if the sizes are bigger now?
https://codereview.chromium.org/974603002/diff/60001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (left): https://codereview.chromium.org/974603002/diff/60001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(), l1->bounds().size()); On 2015/03/06 23:32:08, sky wrote: > It's still good to check the paint regions. Can you use a contains check if the > sizes are bigger now? I think a test that verifies invalidating X gets back a paint rect of Y belongs in CC unit tests not layer_unittests (and we have tests like this). Or is this adding something beyond that?
https://codereview.chromium.org/974603002/diff/60001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (left): https://codereview.chromium.org/974603002/diff/60001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(), l1->bounds().size()); On 2015/03/07 00:02:36, danakj wrote: > On 2015/03/06 23:32:08, sky wrote: > > It's still good to check the paint regions. Can you use a contains check if > the > > sizes are bigger now? > > I think a test that verifies invalidating X gets back a paint rect of Y belongs > in CC unit tests not layer_unittests (and we have tests like this). Or is this > adding something beyond that? Isn't this verified the paint makes it through to the delegate? That seems like a worth while assertion.
On Mon, Mar 9, 2015 at 8:38 AM, <sky@chromium.org> wrote: > > https://codereview.chromium.org/974603002/diff/60001/ui/ > compositor/layer_unittest.cc > File ui/compositor/layer_unittest.cc (left): > > https://codereview.chromium.org/974603002/diff/60001/ui/ > compositor/layer_unittest.cc#oldcode527 > ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(), > l1->bounds().size()); > On 2015/03/07 00:02:36, danakj wrote: > >> On 2015/03/06 23:32:08, sky wrote: >> > It's still good to check the paint regions. Can you use a contains >> > check if > >> the >> > sizes are bigger now? >> > > I think a test that verifies invalidating X gets back a paint rect of >> > Y belongs > >> in CC unit tests not layer_unittests (and we have tests like this). Or >> > is this > >> adding something beyond that? >> > > Isn't this verified the paint makes it through to the delegate? That > seems like a worth while assertion. > We can verify the paint made it through without checking the size. I agree that'd be good to do. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/974603002/diff/60001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (left): https://codereview.chromium.org/974603002/diff/60001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:527: EXPECT_EQ(delegate.paint_size(), l1->bounds().size()); On 2015/03/09 at 15:38:42, sky wrote: > On 2015/03/07 00:02:36, danakj wrote: > > On 2015/03/06 23:32:08, sky wrote: > > > It's still good to check the paint regions. Can you use a contains check if > > the > > > sizes are bigger now? > > > > I think a test that verifies invalidating X gets back a paint rect of Y belongs > > in CC unit tests not layer_unittests (and we have tests like this). Or is this > > adding something beyond that? > > Isn't this verified the paint makes it through to the delegate? That seems like a worth while assertion. |color_index| is updated when paint makes it through to delegate. |color_index| is updated at the same time when |paint_size| is updated. Checking |color_index| would serve this purpose.
sky: ping? This is the only thing blocking us enabling UI implside painting. https://codereview.chromium.org/974603002/diff/80001/ui/compositor/layer_unit... File ui/compositor/layer_unittest.cc (right): https://codereview.chromium.org/974603002/diff/80001/ui/compositor/layer_unit... ui/compositor/layer_unittest.cc:511: EXPECT_EQ(1, delegate.color_index()); Can you add a comment here saying that checking the color_index is verifying that the paint made it through to the delegate?
On 2015/03/10 16:59:37, danakj wrote: > sky: ping? This is the only thing blocking us enabling UI implside painting. > > https://codereview.chromium.org/974603002/diff/80001/ui/compositor/layer_unit... > File ui/compositor/layer_unittest.cc (right): > > https://codereview.chromium.org/974603002/diff/80001/ui/compositor/layer_unit... > ui/compositor/layer_unittest.cc:511: EXPECT_EQ(1, delegate.color_index()); > Can you add a comment here saying that checking the color_index is verifying > that the paint made it through to the delegate? Sorry, I thought the patch was going to be updated to make sure the size at least contains the requested region. Again, that seems like a worthwhile assertion. Am I wrong?
On Tue, Mar 10, 2015 at 11:10 AM, <sky@chromium.org> wrote: > On 2015/03/10 16:59:37, danakj wrote: > >> sky: ping? This is the only thing blocking us enabling UI implside >> painting. >> > > > https://codereview.chromium.org/974603002/diff/80001/ui/ > compositor/layer_unittest.cc > >> File ui/compositor/layer_unittest.cc (right): >> > > > https://codereview.chromium.org/974603002/diff/80001/ui/ > compositor/layer_unittest.cc#newcode511 > >> ui/compositor/layer_unittest.cc:511: EXPECT_EQ(1, >> delegate.color_index()); >> Can you add a comment here saying that checking the color_index is >> verifying >> that the paint made it through to the delegate? >> > > Sorry, I thought the patch was going to be updated to make sure the size at > least contains the requested region. Again, that seems like a worthwhile > assertion. Am I wrong? > I think making sure a paint happens is cool. But expectations about the invalidation => paint rect seems like the wrong layer for this. PictureLayer unit tests do this sort of validation. Arguably, making a test that verifies ui::Layer passes through the right invalidation to a cc::Layer would be okay. And a test verifying that whatever rect a cc::Layer asks to paint makes it back through the ui::Layer is okay. But putting test expectations here about the connection in between just means we have to rebase/change tests in ui/compositor when the implementation in cc/ changes which seems wrong. > > https://codereview.chromium.org/974603002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Ok, LGTM
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/974603002/#ps80001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/974603002/80001
The CQ bit was unchecked by weiliangc@chromium.org
The CQ bit was checked by weiliangc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, enne@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/974603002/#ps100001 (title: "add comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/974603002/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d048acf227321848bc9652c393b94ea80833fe14 Cr-Commit-Position: refs/heads/master@{#319933} |
