948377 - FrameMetrics::CalculateCompositedRectInCssPixels should not be rounding anything

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

See bug 929432 comment 51 and on. That roudning always looked suspicious to me, and I don't think we should ever really be rounding an intermediate value like that. Callers of the function can round it if they so desire; we should just take out the rounding.

Comment 1

[Botond Ballo [:botond]]

My patch for bug 935219 changes CalculateCompositedRectInCssPixels() to return CSSIntRect instead of CSSRect, since it rounds the value anyways and some consumers want a CSSIntRect.

Maybe I should update the patch to remove the rounding and keep the return value as CSSRect? That would probably make the patch simpler, in fact.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Yeah I would prefer that.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I take back comment 2. We can do the rounding cleanup afterwards to reduce the risk of unexpected regressions from bug 935219 - that patch is already fairly large and I don't want to sneak more changes into it.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Most of the call sites only care about the width and height properties so I also added a function to just get the size.

Try run pending at https://tbpl.mozilla.org/?tree=Try&rev=e9c1211f6c86

Comment 5

[Botond Ballo [:botond]]

Comment on attachment 8397908 [details] [diff] [review]
Patch

Review of attachment 8397908 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/Axis.cpp
@@ +273,5 @@
>  }
>  
>  float Axis::GetCompositionLength() {
>    const FrameMetrics& metrics = mAsyncPanZoomController->GetFrameMetrics();
> +  return GetRectLength(metrics.CalculateCompositedRectInCssPixels());

Why not just get the size here?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #5)
> >  float Axis::GetCompositionLength() {
> >    const FrameMetrics& metrics = mAsyncPanZoomController->GetFrameMetrics();
> > +  return GetRectLength(metrics.CalculateCompositedRectInCssPixels());
> 
> Why not just get the size here?

I didn't want to add a GetSizeDimension function or equivalent to Axis. I guess I could but if this is the only thing using it it didn't really seem worthwhile.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8947d2b88791

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8947d2b88791

Comment 9

[Sotaro Ikeda [:sotaro]]

Nominate to "b2g-v1.4". This bug blocks 1.4+ bug.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(In reply to Sotaro Ikeda [:sotaro] from comment #9)
> Nominate to "b2g-v1.4". This bug blocks 1.4+ bug.

I duped that bug to this one, so the 1.4+ flag should just carry over.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Branch patch for 1.4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4e16aa9ba844