cc: Cleanup Layer::SetNeedsDisplayInRect

cc: Cleanup Layer::SetNeedsDisplayInRect

This comment is no longer true.  Also, simplified the early out
condition and added a test.

R=danakj@chromium.org
BUG=none

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

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

[danakj, 2013-07-11 21:28:51]

LGTM

[commit-bot: I haz the power, 2013-07-11 21:33:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/5831706594508800/562...

[slavi, 2013-07-11 21:58:09]

https://codereview.chromium.org/5831706594508800/diff/5629499534213120/cc/lay...
File cc/layers/layer.cc (right):

https://codereview.chromium.org/5831706594508800/diff/5629499534213120/cc/lay...
cc/layers/layer.cc:623: if (dirty_rect.IsEmpty())
Don't do this. When I wrote that function, the intended semantics was that
dirty_rect will be appended to the region that need update. Passing empty rect
still should issue a commit.

See 
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...
for example of such usage.

[danakj, 2013-07-11 21:59:12]

https://codereview.chromium.org/5831706594508800/diff/5629499534213120/cc/lay...
File cc/layers/layer.cc (right):

https://codereview.chromium.org/5831706594508800/diff/5629499534213120/cc/lay...
cc/layers/layer.cc:623: if (dirty_rect.IsEmpty())
On 2013/07/11 21:58:09, slavi wrote:
> Don't do this. When I wrote that function, the intended semantics was that
> dirty_rect will be appended to the region that need update. Passing empty rect
> still should issue a commit.
> 
> See 
>
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/widget/de...
> for example of such usage.

Why not just call SetNeedsCommit on the layer's host?

[enne (OOO), 2013-07-11 22:06:09]

Without passing a valid dirty rect, how is that code correct during partial
swap?

What's the use case where you need a commit without damage?

[skaslev, 2013-07-12 00:32:04]

I don't remember the details anymore. I can refer you to the example in the
cs link I sent or just trace callers with empty rects in the browser
compositor. The point was that if you want to change the semantics of
calling this function with empty rects, you might also have to track down
the callers that pass empty rects as a request for redraw.


On Thu, Jul 11, 2013 at 3:06 PM, <enne@chromium.org> wrote:

> Without passing a valid dirty rect, how is that code correct during partial
> swap?
>
> What's the use case where you need a commit without damage?
>
>
https://codereview.chromium.**org/5831706594508800/<https://codereview.chromi...
>



-- 
Slavomir Kaslev | Software Engineer | skaslev@google.com | 562 217 8497

[enne (OOO), 2013-07-24 00:52:01]

skaslev: How does that code even get called with a compositor? It looks like
every caller calls HandlePaintAccelerated (which passes a dirty rect) first
before calling HandlePaint (which just passes a canvas), e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/win/hwnd_....

I know very little about this code, but I'm having a hard time seeing what
you're worried about.  Do you have more examples?

[tomhudson, 2013-07-24 09:02:42]

https://codereview.chromium.org/5831706594508800/diff/5629499534213120/cc/lay...
File cc/layers/layer.cc (right):

https://codereview.chromium.org/5831706594508800/diff/5629499534213120/cc/lay...
cc/layers/layer.cc:623: if (dirty_rect.IsEmpty())
On 2013/07/11 21:58:09, slavi wrote:
> Don't do this. When I wrote that function, the intended semantics was that
> dirty_rect will be appended to the region that need update. Passing empty rect
> still should issue a commit.

If people can't determine the intended semantics by reading the code, do we need
to add comments? Or more descriptive naming?

[enne (OOO), 2013-07-24 19:07:14]

I do not think that the code that skaslev points out can make it to the
compositor.  Even if it did, we would commit and not draw because of the no
damage early out.  So, I do not believe that anything can actually be relying on
this logic.

I'm going to commit this and will deal with any fallout, if any.

[commit-bot: I haz the power, 2013-07-24 19:11:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/5831706594508800/562...

[commit-bot: I haz the power, 2013-07-25 15:22:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/5831706594508800/562...

[commit-bot: I haz the power, 2013-07-26 03:11:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/5831706594508800/562...

[commit-bot: I haz the power, 2013-07-28 08:34:47]

Change committed as 214103

[ojan, 2013-07-28 18:23:19]

Looks like this patch caused regressions:
https://code.google.com/p/chromium/issues/detail?id=265258

[Stephen White, 2013-07-29 06:37:26]

On 2013/07/28 18:23:19, ojan wrote:
> Looks like this patch caused regressions:
> https://code.google.com/p/chromium/issues/detail?id=265258

Yep, I verified this change locally as the culprit, and rolled out in r214163.

[enne (OOO), 2013-07-30 20:01:27]

danakj: Take two.  PTAL.

https://codereview.chromium.org/5831706594508800/diff/54001/cc/layers/image_l...
File cc/layers/image_layer.cc (right):

https://codereview.chromium.org/5831706594508800/diff/54001/cc/layers/image_l...
cc/layers/image_layer.cc:46: if (!updater_->UsingBitmap(bitmap_)) {
The reason this code was broken before was that ImageLayer::SetBitmap above was
calling SetNeedsDisplay() when the bounds of the layer was empty.  This
prevented needs_display_ from getting set to true, causing the tile size to not
be updated in this conditional.  The tile size being zero means that we were
requesting shared memory of size zero which blew up.

This code is really trying to check that the bitmap has changed, and so check
this explicitly.  It's also a little sketchy that it was unsetting
needs_display_ here.

https://codereview.chromium.org/5831706594508800/diff/54001/cc/layers/nine_pa...
File cc/layers/nine_patch_layer.cc (right):

https://codereview.chromium.org/5831706594508800/diff/54001/cc/layers/nine_pa...
cc/layers/nine_patch_layer.cc:36: } else if (bitmap_dirty_ && DrawsContent()) {
NinePatchLayer also uses image layer updaters and was the only other layer type
checking needs_display_ for similar reasons when setting the bitmap. 
bitmap_dirty_ is redundant with needs_display_ here and so can be removed.

[danakj, 2013-07-31 01:19:07]

LGTM

Seems like NinePatchLayer could remove bitmap_dirty_ and use your new
IsUsingBitmap method as well.

[enne (OOO), 2013-07-31 02:30:56]

On 2013/07/31 01:19:07, danakj wrote:

> Seems like NinePatchLayer could remove bitmap_dirty_ and use your new
> IsUsingBitmap method as well.

I thought about that, but NinePatchLayer uses bitmap_dirty_ in both
SetTexturePriorities (to create the resource) and in Update (to queue its
upload), so it wasn't as simple to remove.  ImageLayer doesn't need a separate
variable because TiledLayer handles the upload implicitly because of the
invalidations.

[commit-bot: I haz the power, 2013-07-31 02:52:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/5831706594508800/54001

[commit-bot: I haz the power, 2013-07-31 02:52:53]

Failed to apply patch for cc/layers/layer.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file cc/layers/layer.cc
  Hunk #1 FAILED at 663.
  1 out of 1 hunk FAILED -- saving rejects to file cc/layers/layer.cc.rej

Patch:       cc/layers/layer.cc
Index: cc/layers/layer.cc
diff --git a/cc/layers/layer.cc b/cc/layers/layer.cc
index
5e7bd77ecc9ee5fa31359d0d78e6980f529f7fc3..fc9f2daba445f1b78b01dc5a3c2624a620663b95
100644
--- a/cc/layers/layer.cc
+++ b/cc/layers/layer.cc
@@ -663,13 +663,13 @@ void Layer::SetHideLayerAndSubtree(bool hide) {
 }
 
 void Layer::SetNeedsDisplayRect(const gfx::RectF& dirty_rect) {
+  if (dirty_rect.IsEmpty())
+    return;
+
   update_rect_.Union(dirty_rect);
   needs_display_ = true;
 
-  // Simply mark the contents as dirty. For non-root layers, the call to
-  // SetNeedsCommit will schedule a fresh compositing pass.
-  // For the root layer, SetNeedsCommit has no effect.
-  if (DrawsContent() && !update_rect_.IsEmpty())
+  if (DrawsContent())
     SetNeedsCommit();
 }

[commit-bot: I haz the power, 2013-07-31 17:31:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enne@chromium.org/5831706594508800/67001

[commit-bot: I haz the power, 2013-07-31 20:20:26]

Change committed as 214819