Dirty rects always contain full tiles with delegated rendering.

Dirty rects always contain full tiles with delegated rendering.

Fix: Compute update_rect separately from original dirty rects and pass
them to update_rect_ as well as updating the contents.

BUG=316469
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238911

[prashant.n, 2013-11-25 06:01:26]

update_rect created from original dirty rects is passed to
1. update_rect_ and
2. PrepareToUpdate so that only original dirty area will be updated(rastered)
from engine.

Please check if we need point 2.

[enne (OOO), 2013-11-25 20:59:48]

Unit test?

Please also format your description to 72 characters max width.

https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc
File cc/layers/tiled_layer.cc (right):

https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:490: if (!update_rect.IsEmpty()) {
I'm not sure this can ever happen.  I'd prefer a DCHECK to a conditional.

https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:497: Updater()->PrepareToUpdate(update_rect,
Re: point #2, I think this is wrong.  You need the entire paint rect.  Tiles
that can't be partially updated need to be entirely rastered and uploaded.

[prashant.n, 2013-11-26 04:56:06]

Please reply to my queries

https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc
File cc/layers/tiled_layer.cc (right):

https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:490: if (!update_rect.IsEmpty()) {
Yes, true, this will never happen. I'll remove this.

https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:497: Updater()->PrepareToUpdate(update_rect,
In TiledLayer::TileOnlyNeedsPartialUpdate(), we check two things -
1. whether dirty rect is partial
2. tile has the backing texture.

Can there be the case in delegate rendering, where tile has the backing texture
and there we could have updated only original dirty rect?

[danakj, 2013-11-26 16:08:10]

On 2013/11/26 04:56:06, prashant.n wrote:
> Please reply to my queries
> 
> https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc
> File cc/layers/tiled_layer.cc (right):
> 
>
https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc#...
> cc/layers/tiled_layer.cc:490: if (!update_rect.IsEmpty()) {
> Yes, true, this will never happen. I'll remove this.
> 
>
https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc#...
> cc/layers/tiled_layer.cc:497: Updater()->PrepareToUpdate(update_rect,
> In TiledLayer::TileOnlyNeedsPartialUpdate(), we check two things -
> 1. whether dirty rect is partial
> 2. tile has the backing texture.

3. if we can do a partial update (check they are allowed and we only allow so
many per frame).

> Can there be the case in delegate rendering, where tile has the backing
texture
> and there we could have updated only original dirty rect?

3 will always be false under ubercomp.

[danakj, 2013-11-26 16:08:39]

https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc
File cc/layers/tiled_layer.cc (right):

https://codereview.chromium.org/85143002/diff/30001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:497: Updater()->PrepareToUpdate(update_rect,
On 2013/11/25 20:59:48, enne wrote:
> Re: point #2, I think this is wrong.  You need the entire paint rect.  Tiles
> that can't be partially updated need to be entirely rastered and uploaded.

I agree, it should only be the update_rect_ that changes in this CL. The
painting should not change.

[prashant.n, 2013-11-27 04:26:20]

Yes, you are correct, checked the path.

[danakj, 2013-11-27 18:09:09]

This looks good, can you add a unit test that invalidates some partial tiles,
verifies we paint the entire tiles, but only consider the partial tiles as
damaged?

https://codereview.chromium.org/85143002/diff/60001/cc/layers/tiled_layer.cc
File cc/layers/tiled_layer.cc (right):

https://codereview.chromium.org/85143002/diff/60001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:444: // Prepare update rect from original dirty rects
period at end of sentence.

https://codereview.chromium.org/85143002/diff/60001/cc/layers/tiled_layer.cc#...
cc/layers/tiled_layer.cc:492: // the SkCanvas until the paint finishes, so we
grab a local reference here
this change looks unneeded?

[prashant.n, 2013-12-02 11:09:56]

Addressed review comments and added unit test.

[prashant.n, 2013-12-02 11:22:52]

Looks some other tests are failing due to this patch. I'm checking those.

[prashant.n, 2013-12-02 14:30:48]

Fixed broken tests.
If we have unit test in layer_tree_host_unittest_damage.cc, do we need a
separate unit test in tiled_layer_unittest.cc?

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_damage.cc:385: paint_rect.ToString());
Do we need this as we can verify with actual values?

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_damage.cc:389:
EXPECT_FALSE(root_damage.Contains(paint_rect));
Should we verify this, as we have verified with actual values?
(100+10, 100+11, 12, 13) is inside (100, 100, 30, 30).

[danakj, 2013-12-02 15:06:46]

On 2013/12/02 14:30:48, prashant.n wrote:
> Fixed broken tests.
> If we have unit test in layer_tree_host_unittest_damage.cc, do we need a
> separate unit test in tiled_layer_unittest.cc?

I think the l_t_h_u_damage test is enough.

[danakj, 2013-12-02 15:06:52]

https://codereview.chromium.org/85143002/diff/120001/cc/layers/content_layer.h
File cc/layers/content_layer.h (right):

https://codereview.chromium.org/85143002/diff/120001/cc/layers/content_layer....
cc/layers/content_layer.h:64: virtual LayerUpdater* Updater() const OVERRIDE;
Keep this method private

https://codereview.chromium.org/85143002/diff/120001/cc/layers/tiled_layer_un...
File cc/layers/tiled_layer_unittest.cc (right):

https://codereview.chromium.org/85143002/diff/120001/cc/layers/tiled_layer_un...
cc/layers/tiled_layer_unittest.cc:1137: TEST_F(TiledLayerPartialUpdateTest, \
no \ needed

https://codereview.chromium.org/85143002/diff/120001/cc/test/fake_content_lay...
File cc/test/fake_content_layer.cc (right):

https://codereview.chromium.org/85143002/diff/120001/cc/test/fake_content_lay...
cc/test/fake_content_layer.cc:35: return
(reinterpret_cast<FakeContentLayerUpdater*>(ContentLayer::Updater()));
this should be a static_cast, no?

https://codereview.chromium.org/85143002/diff/120001/cc/test/fake_content_lay...
File cc/test/fake_content_layer.h (right):

https://codereview.chromium.org/85143002/diff/120001/cc/test/fake_content_lay...
cc/test/fake_content_layer.h:16: gfx::Rect content_rect() const OVERRIDE;
you can just do "using ContentLayerUpdater::content_rect;" here?

https://codereview.chromium.org/85143002/diff/120001/cc/test/fake_content_lay...
cc/test/fake_content_layer.h:34: virtual FakeContentLayerUpdater* Updater()
const;
How about instead, make this LastPaintRect(), and have it call the
content_rect() on the updater? Then the new FakeContentLayerUpdater class
doesn't need to be exposed and be entirely in the .cc file.

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_damage.cc:382: gfx::RectF paint_rect =
static_cast<gfx::RectF>
You don't need to cast from Rect to RectF

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_damage.cc:384:
EXPECT_EQ((gfx::RectF(child_->bounds()).ToString()),
you don't need () around the RectF.ToString() call

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_damage.cc:389:
EXPECT_FALSE(root_damage.Contains(paint_rect));
On 2013/12/02 14:30:48, prashant.n wrote:
> Should we verify this, as we have verified with actual values?
> (100+10, 100+11, 12, 13) is inside (100, 100, 30, 30).

I think the above check against layer->bounds() is enough.

[danakj, 2013-12-02 15:09:48]

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_damage.cc (right):

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_damage.cc:378: // Verify damage area.
Instead of saying "Verify damage area" which is clear that's what you're doing,
you can say why you expect the damage area to be, or why. ie. "The damaged area
should match the invalidation exactly.

It looks like you could move the root_damage check out of this if/else statement
entirely, since it'll always be the same now. And just test the paint_rect in
the if/else (also add an if case to demonstrate the difference?)

https://codereview.chromium.org/85143002/diff/120001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_damage.cc:381: // Verify paint area.
Same here, say what you're expecting or why rather than just stating that you're
verifying something.

[prashant.n, 2013-12-03 08:30:55]

Addressed the review comments. Please take a look at.

https://codereview.chromium.org/85143002/diff/140001/cc/layers/content_layer.h
File cc/layers/content_layer.h (right):

https://codereview.chromium.org/85143002/diff/140001/cc/layers/content_layer....
cc/layers/content_layer.h:65: 
We need to make this member protected to get accessed from derived class.

[danakj, 2013-12-03 18:07:49]

Thanks, LGTM

[commit-bot: I haz the power, 2013-12-04 04:10:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prashant.n@samsung.com/85143002/140001

[commit-bot: I haz the power, 2013-12-04 04:34:19]

Retried try job too often on mac_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[prashant.n, 2013-12-04 05:02:47]

The new class added needs explicit destructor

[danakj, 2013-12-04 16:32:16]

https://codereview.chromium.org/85143002/diff/160001/cc/test/fake_content_lay...
File cc/test/fake_content_layer.cc (right):

https://codereview.chromium.org/85143002/diff/160001/cc/test/fake_content_lay...
cc/test/fake_content_layer.cc:15: ~FakeContentLayerUpdater() {}
It should be virtual, then LGTM

[prashant.n, 2013-12-05 04:15:36]

Please taka a look at.

[commit-bot: I haz the power, 2013-12-05 04:16:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prashant.n@samsung.com/85143002/180001

[commit-bot: I haz the power, 2013-12-05 04:32:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/prashant.n@samsung.com/85143002/200001

[commit-bot: I haz the power, 2013-12-05 07:22:53]

Change committed as 238911