Converted LayerImpl::bounds() to return SizeF.

Converted LayerImpl::bounds() to return SizeF.

Since bounds includes bounds_delta, which is SizeF, it makes sense to return
the floating point sum of these two values. In call sites broken by the
change, the returned value is converted to Size using ToCeiledSize to
preserve the current snapping behavior, though in most (all?) these cases
the layer bounds are expected to be integral anyway.

BUG=

Committed: https://crrev.com/c7935852d9590e2456e92c4ad2d0b48c816ec5ed
Cr-Commit-Position: refs/heads/master@{#298439}

[bokan, 2014-10-06 23:11:41]

bokan@chromium.org changed reviewers:
+ aelias@chromium.org

[bokan, 2014-10-06 23:11:42]

ptal.

[aelias_OOO_until_Jul13, 2014-10-06 23:34:26]

lgtm

[bokan, 2014-10-07 01:16:15]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-10-07 01:17:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634683003/1

[commit-bot: I haz the power, 2014-10-07 03:22:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-07 03:22:41]

Exceeded time limit waiting for builds to trigger.

[bokan, 2014-10-07 10:31:40]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2014-10-07 10:32:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/634683003/1

[commit-bot: I haz the power, 2014-10-07 10:33:13]

Committed patchset #1 (id:1) as 890a31ac8a1477389b7757131738aed5914099f8

[commit-bot: I haz the power, 2014-10-07 10:34:06]

Patchset 1 (id:??) landed as
https://crrev.com/c7935852d9590e2456e92c4ad2d0b48c816ec5ed
Cr-Commit-Position: refs/heads/master@{#298439}

[enne (OOO), 2014-10-07 17:29:12]

I'm not very keen on this patch.

Also, there's no bug listed here.  Is this fixing something?

[bokan, 2014-10-07 18:06:17]

On 2014/10/07 17:29:12, enne wrote:
> I'm not very keen on this patch.
> 
> Also, there's no bug listed here.  Is this fixing something?

It's a Todo I picked up so no bug, Alexandre mentioned he's seen some bugs
caused by this but I don't know what they were. I was eager for this as a
prerequisite for another patch. I'm trying to match the scroll extents behavior
between Blink and CCs LayerImpl::MaxScrollOffset. The Blink side ended up having
to add the delta, ceil, scale the bounds, floor, subtract the clip, then floor
again implicitly which seems kind of pathological. IMO it makes sense since the
top controls can change the (viewport) bounds by a fractional amount so the rest
of the compositor should to know about it.

[danakj, 2014-10-07 18:41:58]

On Tue, Oct 7, 2014 at 2:06 PM, <bokan@chromium.org> wrote:

> On 2014/10/07 17:29:12, enne wrote:
>
>> I'm not very keen on this patch.
>>
>
>  Also, there's no bug listed here.  Is this fixing something?
>>
>
> It's a Todo I picked up so no bug, Alexandre mentioned he's seen some bugs
> caused by this but I don't know what they were. I was eager for this as a

prerequisite for another patch. I'm trying to match the scroll extents
> behavior
> between Blink and CCs LayerImpl::MaxScrollOffset. The Blink side ended up
> having
>

FWIW ScrollOffsets are doubles because a float is not able to represent
absolute values in spaces the size of web content.


> to add the delta, ceil, scale the bounds, floor, subtract the clip, then
> floor
> again implicitly which seems kind of pathological. IMO it makes sense
> since the
>

I think what you're saying is that blink has to guess at cc content bounds
to try work there, since that's an internal thing to cc right now? Why does
a layer size need to be fractionally sized because of the viewport changing
size?


> top controls can change the (viewport) bounds by a fractional amount so
> the rest
> of the compositor should to know about it.
>

The viewport is already specified in device pixels, so that should be
expressible with integers?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bokan, 2014-10-07 19:02:40]

On 2014/10/07 18:41:58, danakj wrote:
> On Tue, Oct 7, 2014 at 2:06 PM, <mailto:bokan@chromium.org> wrote:
> 
> > On 2014/10/07 17:29:12, enne wrote:
> >
> >> I'm not very keen on this patch.
> >>
> >
> >  Also, there's no bug listed here.  Is this fixing something?
> >>
> >
> > It's a Todo I picked up so no bug, Alexandre mentioned he's seen some bugs
> > caused by this but I don't know what they were. I was eager for this as a
> 
> prerequisite for another patch. I'm trying to match the scroll extents
> > behavior
> > between Blink and CCs LayerImpl::MaxScrollOffset. The Blink side ended up
> > having
> >
> 
> FWIW ScrollOffsets are doubles because a float is not able to represent
> absolute values in spaces the size of web content.
> 
I would expect content layers to still be integral values. The two viewport
layers are of sizes that should be expressible in floats.

> 
> > to add the delta, ceil, scale the bounds, floor, subtract the clip, then
> > floor
> > again implicitly which seems kind of pathological. IMO it makes sense
> > since the
> >
> 
> I think what you're saying is that blink has to guess at cc content bounds
> to try work there, since that's an internal thing to cc right now? Why does
> a layer size need to be fractionally sized because of the viewport changing
> size?
> 
It's not guessing, I'm trying to reproduce the steps CC takes to calculate
the max scroll offset. In MaxScrollOffset, the scaled_scroll_bounds are
calculated by scaling bounds(), which right now would snap before scaling
(and again after). I suppose we can just calculate the exact bounds by adding
the private variables manually here; I just saw the TODO and figured this was
something we wanted to do anyway.
 
> 
> > top controls can change the (viewport) bounds by a fractional amount so
> > the rest
> > of the compositor should to know about it.
> >
> 
> The viewport is already specified in device pixels, so that should be
> expressible with integers?
> 
IIRC that's only true for the root layer? Now that we have a second "outer"
viewport it needs to be adjusted by the top controls as well, albeit not
directly (it's adjusted to match the aspect ratio) so perhaps the outer
viewport need not be pixel-perfect. In any case, as mentioned above, I can
get at the actual bounds through the privates so it's not strictly
necessary for my needs.

[aelias_OOO_until_Jul13, 2014-10-07 19:05:20]

From the point of view of scrolling, the viewport is not device_viewport_size,
but rather the inner viewport clip layer provided by Blink, which is in DIP
pixels.  Since wjmaclean's first big patch, the scrolling-related logic makes
comparisons between the clip viewport and the scroll layer viewport.  The top
controls moving by sub-physical-pixel distances then forces us to resize the
clip layer by non-integral values as well.  The early integer snapping was
causing issues like bottom-fixed-position elements jittering.

[aelias_OOO_until_Jul13, 2014-10-07 19:57:17]

Also, I apologize for not cc'ing you folks on initial review of this patch.

[danakj, 2014-10-07 20:47:45]

Thanks for the context. I think this seems like a very large hammer to fix
the problem of a single clipping layer. Maybe this is just what we need to
do, but I'm not a fan of the side effects this has to make
clipping/positioning in the viewport correct, which could have a completely
orthogonal resolution.

On Tue, Oct 7, 2014 at 3:05 PM, <aelias@chromium.org> wrote:

> From the point of view of scrolling, the viewport is not
> device_viewport_size,
> but rather the inner viewport clip layer provided by Blink, which is in DIP
> pixels.  Since wjmaclean's first big patch, the scrolling-related logic
> makes
> comparisons between the clip viewport and the scroll layer viewport.  The
> top
> controls moving by sub-physical-pixel distances then forces us to resize
> the
> clip layer by non-integral values as well.  The early integer snapping was
> causing issues like bottom-fixed-position elements jittering.
>
> https://codereview.chromium.org/634683003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aelias_OOO_until_Jul13, 2014-10-07 21:14:59]

I'm not sure I agree the hammer is that large.  Although this technically
affects all layers, the only ones that actually cares about the int/float
distinction are the root scrolling ones (can have very large documents, and
needs subpixel accuracy -- every other layer just sticks to small ints).

I think we're trapped in an uncertain state right now where we're afraid to fix
this chronic problem of subpixel accuracy out of fear of regressing these large
document cases, but on the other hand we're not realistically going to invest a
lot of effort into e.g. introducing double-based geometry types and using them
everywhere in CC.  Given the absence of the will to make a wholesale change like
that, I feel changing more things to floats is a better tradeoff in the medium
term, as we do concretely care about subpixel accuracy all the time now, whereas
the 8-million-pixel-high documents are rare to the point of being a mostly
theoretical concern (imo).

Secondly, I'm looking ahead to the time when we want to move the Android browser
compositor to DIP coordinates, one of the main blockers for that is changing all
the layer bounds to DIPs.  When that day comes, this debate will arise again,
and it would be good to know if we need to block that change on switching
everything to doubles/fixed-point, or whether SizeF layer-bounds are something
we can live with.  Another way of looking at this change is as a smaller-scoped
and easily-revertible trial balloon we can ship some releases with and get a
more concrete idea of how often the large-document scrolling bugs actually come
up, which will better inform the debate at that time.  I think we have a
difference of opinion right now about how common/relevant the large-document
cases are, and getting bug reports (or not) from the wild would help prove who's
right.

[enne (OOO), 2014-10-07 21:20:05]

I very much agree with Dana.  I think this looks small, but it's something that
has a lot of implications that haven't been considered.

In other words, this patch is really about resizing one layer to get proper
scroll offsets, but floating point bounds() implemented properly would require a
lot more thinking about to do properly.

It's especially weird to have floating point bounds but integer content bounds. 
For example, if you had like a solid color layer with floating point bounds(),
then how do you issue quads with the right size? Would you need antialiasing for
that edge?  Do you just round its content_bounds out to an integer?  Do we need
to make more checks in LayerTreeHostCommon about when clips are axis aligned if
their bounds() are not integers?

[aelias_OOO_until_Jul13, 2014-10-07 21:34:34]

On 2014/10/07 at 21:20:05, enne wrote:
> I very much agree with Dana.  I think this looks small, but it's something
that has a lot of implications that haven't been considered.
> 
> In other words, this patch is really about resizing one layer to get proper
scroll offsets, but floating point bounds() implemented properly would require a
lot more thinking about to do properly.
> 
> It's especially weird to have floating point bounds but integer content
bounds.  For example, if you had like a solid color layer with floating point
bounds(), then how do you issue quads with the right size? Would you need
antialiasing for that edge?  Do you just round its content_bounds out to an
integer?  Do we need to make more checks in LayerTreeHostCommon about when clips
are axis aligned if their bounds() are not integers?

Hmm, the reason to make this change has nothing to do with those drawing-related
issues.  This makes me think that an underlying problem is that bounds() is
overloaded for both scrolling and drawing use cases.  One way to solve this
would be to have instead of bounds_delta:

gfx::SizeF BoundsForScrolling() const { return bounds_for_scrolling_.IsEmpty() ?
bounds_ : bounds_for_scrolling_; }

then use that everywhere scrolling-related.  That would address those concerns,
although not Dana's large-document concern (for that one, see my argument in
#19).

[danakj, 2014-10-08 20:59:44]

On 2014/10/07 21:34:34, aelias wrote:
> On 2014/10/07 at 21:20:05, enne wrote:
> > I very much agree with Dana.  I think this looks small, but it's something
> that has a lot of implications that haven't been considered.
> > 
> > In other words, this patch is really about resizing one layer to get proper
> scroll offsets, but floating point bounds() implemented properly would require
a
> lot more thinking about to do properly.
> > 
> > It's especially weird to have floating point bounds but integer content
> bounds.  For example, if you had like a solid color layer with floating point
> bounds(), then how do you issue quads with the right size? Would you need
> antialiasing for that edge?  Do you just round its content_bounds out to an
> integer?  Do we need to make more checks in LayerTreeHostCommon about when
clips
> are axis aligned if their bounds() are not integers?
> 
> Hmm, the reason to make this change has nothing to do with those
drawing-related
> issues.  This makes me think that an underlying problem is that bounds() is
> overloaded for both scrolling and drawing use cases.

+1

> One way to solve this would be to have instead of bounds_delta:
> 
> gfx::SizeF BoundsForScrolling() const { return bounds_for_scrolling_.IsEmpty()
?
> bounds_ : bounds_for_scrolling_; }
> 
> then use that everywhere scrolling-related.  That would address those
concerns,
> although not Dana's large-document concern (for that one, see my argument in
> #19).

How does that really differ from having LayerImpl::FixedContainerSizeDelta? I
see that used in the fixed pos math right now, do we still have jittering with
bottom fixed pos? Or does that method fix it at the moment?

[aelias_OOO_until_Jul13, 2014-10-08 21:16:25]

> How does that really differ from having LayerImpl::FixedContainerSizeDelta? I
see that used in the fixed pos math right now, do we still have jittering with
bottom fixed pos? Or does that method fix it at the moment?

One big difference is bokan@'s motivation for making this change, the effect on
MaxScrollOffset and in particular, the need to compute that the same way between
main and impl threads.

We at some point suffered a regression currently causing some jittering in
bottom-fixed pos although I haven't investigated the exact cause.  In general,
every place that snaps to int in the pipeline raises the risk of this kind of
bug appearing.  Such bugs can generally be avoided by careful reasoning (e.g.
snapping another thing in the same way to avoid a mismatch), but that's brittle
and makes issues come back on seemingly minor refactorings.  That's why I prefer
to preserve fractional information as much as possible.

BTW, this patch has now been reverted
(https://codereview.chromium.org/637913003/ ).

[danakj, 2014-10-08 21:28:50]

On Wed, Oct 8, 2014 at 5:16 PM, <aelias@chromium.org> wrote:

> How does that really differ from having LayerImpl::FixedContainerSizeDelta?
>> I
>>
> see that used in the fixed pos math right now, do we still have jittering
> with
> bottom fixed pos? Or does that method fix it at the moment?
>
> One big difference is bokan@'s motivation for making this change, the
> effect on
> MaxScrollOffset and in particular, the need to compute that the same way
> between
> main and impl threads.
>
> We at some point suffered a regression currently causing some jittering in
> bottom-fixed pos although I haven't investigated the exact cause.  In
> general,
> every place that snaps to int in the pipeline raises the risk of this kind
> of
> bug appearing.  Such bugs can generally be avoided by careful reasoning
> (e.g.
> snapping another thing in the same way to avoid a mismatch), but that's
> brittle
> and makes issues come back on seemingly minor refactorings.  That's why I
> prefer
> to preserve fractional information as much as possible.
>

Looks like MaxScrollOffset already throws the bounds into floats when it
does its math:

https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...

So adding a BoundsForScrolling would probably not change anything with that
at the moment.

That said, mirroring the bounds() property sounds a little awkward, so I'd
prefer to avoid that if possible. Also the if (present) useonething; else
useanother; inside of it makes understanding behaviour at every call site
more complicated. I totally agree with you about a fragile system though,
and I don't wanna create that either.

Does scrolling code outside of MaxScrollOffset and
FixedContainerSizeDelta actually use layer bounds? I tried looking around
LTHI but I don't see anything that is using it for scrolling. The only use
was "which scrollbar is the mouse close to".

MaxScrollOffset returns a gfx::ScrollOffset which is already fractional.
And the FixedContainerSizeDelta returns a Vector2dF which is fractional.
Are there other places that end up using an integer bounds for scrolling? I
would have expected MaxScrollOffset to be that fractional proxy for them
all, but I'm interested to know about others.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[aelias_OOO_until_Jul13, 2014-10-08 21:52:44]

On 2014/10/08 at 21:28:50, danakj wrote:
> On Wed, Oct 8, 2014 at 5:16 PM, <aelias@chromium.org> wrote:
> 
> > How does that really differ from having LayerImpl::FixedContainerSizeDelta?
> >> I
> >>
> > see that used in the fixed pos math right now, do we still have jittering
> > with
> > bottom fixed pos? Or does that method fix it at the moment?
> >
> > One big difference is bokan@'s motivation for making this change, the
> > effect on
> > MaxScrollOffset and in particular, the need to compute that the same way
> > between
> > main and impl threads.
> >
> > We at some point suffered a regression currently causing some jittering in
> > bottom-fixed pos although I haven't investigated the exact cause.  In
> > general,
> > every place that snaps to int in the pipeline raises the risk of this kind
> > of
> > bug appearing.  Such bugs can generally be avoided by careful reasoning
> > (e.g.
> > snapping another thing in the same way to avoid a mismatch), but that's
> > brittle
> > and makes issues come back on seemingly minor refactorings.  That's why I
> > prefer
> > to preserve fractional information as much as possible.
> >
> 
> Looks like MaxScrollOffset already throws the bounds into floats when it
> does its math:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer_im...
> 
> So adding a BoundsForScrolling would probably not change anything with that
> at the moment.

There's still an integer snap there hidden inside the bounds() call on the line
"scroll_clip_layer_->bounds().height(),", in the case where bounds_delta_ is
nonzero due to top controls.

Cataloguing the scroll-related uses of clip layer bounds(), I see
LayerImpl::MaxScrollOffset, LayerImpl::SetScrollbarPosition,
LayerImpl::FixedContainerSizeDelta, and LayerTreeImpl::ScrollableViewportSize.

[aelias_OOO_until_Jul13, 2014-10-08 21:59:36]

> Also the if (present) useonething; else
> useanother; inside of it makes understanding behaviour at every call site
> more complicated.

Yeah, you're right about that.  On second thought, we could keep bounds_delta_
and just have bounds() and BoundsForScrolling() both sum in the same way we do
today, except that bounds() would additionally snap to int.