cc: The RenderSurface contentRect can have negative position.

cc: The RenderSurface contentRect can have negative position.

The position of the RenderSurface contentRect is relative to the position
of its owning layer. The size of the contentRect should fit inside the unclipped
surface rect.

R=piman,shawnsingh
BUG=171734
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180858

[shawnsingh, 2013-02-04 21:51:08]

LGTM, one idea below, either way is OK

also nit:  Might be helpful to clarify which "rect" you're referring to, on the
bug description.

https://codereview.chromium.org/12179022/diff/1/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/12179022/diff/1/cc/render_surface_impl.cc#new...
cc/render_surface_impl.cc:213: gfx::Rect(contentRect().size())));
Thanks for catching this.  I think it might be a stricter (better) check to
include positions in this DCHECK too.  Is there some reason we shouldnt?

i.e.   DCHECK(gfx::RectF(unclippedSurfacePos,
unclippedSurfaceSize).Contains(contentRect())));

[danakj, 2013-02-04 21:52:15]

https://codereview.chromium.org/12179022/diff/1/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/12179022/diff/1/cc/render_surface_impl.cc#new...
cc/render_surface_impl.cc:213: gfx::Rect(contentRect().size())));
On 2013/02/04 21:51:08, shawnsingh wrote:
> Thanks for catching this.  I think it might be a stricter (better) check to
> include positions in this DCHECK too.  Is there some reason we shouldnt?
> 
> i.e.   DCHECK(gfx::RectF(unclippedSurfacePos,
> unclippedSurfaceSize).Contains(contentRect())));

I just didn't think we have the position of the unclipped surface here. Do we?

[shawnsingh, 2013-02-04 21:59:24]

https://codereview.chromium.org/12179022/diff/1/cc/render_surface_impl.cc
File cc/render_surface_impl.cc (right):

https://codereview.chromium.org/12179022/diff/1/cc/render_surface_impl.cc#new...
cc/render_surface_impl.cc:213: gfx::Rect(contentRect().size())));
On 2013/02/04 21:52:15, danakj wrote:
> I just didn't think we have the position of the unclipped surface here. Do we?

Oh, I see, you're right.

Also, thinking a second time - your change looks fine, but isn't it true that we
pin the origin of the surface to the owning layer, ** in the case where the
layer does have a mask / masksToBounds ?

So what is the case where a contentRect on a surface with mask would have
actually triggered the original DCHECK incorrectly?

[danakj, 2013-02-04 22:02:31]

On 2013/02/04 21:59:24, shawnsingh wrote:
> https://codereview.chromium.org/12179022/diff/1/cc/render_surface_impl.cc
> File cc/render_surface_impl.cc (right):
> 
>
https://codereview.chromium.org/12179022/diff/1/cc/render_surface_impl.cc#new...
> cc/render_surface_impl.cc:213: gfx::Rect(contentRect().size())));
> On 2013/02/04 21:52:15, danakj wrote:
> > I just didn't think we have the position of the unclipped surface here. Do
we?
> 
> Oh, I see, you're right.
> 
> Also, thinking a second time - your change looks fine, but isn't it true that
we
> pin the origin of the surface to the owning layer, ** in the case where the
> layer does have a mask / masksToBounds ?
> 
> So what is the case where a contentRect on a surface with mask would have
> actually triggered the original DCHECK incorrectly?

On the site from the bug
https://code.google.com/p/chromium/issues/detail?id=171734

I saw this:
unclippedSurfaceSize 566.000000x500.000000
Surface contentRect 92,-1 365x496

The owning layer had position 72,74 or something.. the surface contentRect
position was very unrelated to the position of the owning layer.

[commit-bot: I haz the power, 2013-02-05 00:15:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12179022/1

[commit-bot: I haz the power, 2013-02-06 00:05:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/12179022/1

[commit-bot: I haz the power, 2013-02-06 02:00:13]

Change committed as 180858