Closed Bug 948377 Opened 6 years ago Closed 6 years ago

FrameMetrics::CalculateCompositedRectInCssPixels should not be rounding anything

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.4+
Tracking Status
firefox30 --- wontfix
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [caf priority: p2][CR 674614])

Attachments

(2 files)

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.
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.
Yeah I would prefer that.
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.
Assignee: nobody → bugmail.mozilla
Attached patch PatchSplinter Review
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 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?
Attachment #8397908 - Flags: review?(botond) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/8947d2b88791
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Blocks: 1019300
Nominate to "b2g-v1.4". This bug blocks 1.4+ bug.
blocking-b2g: --- → 1.4?
Flags: needinfo?
Flags: needinfo?
(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.
No longer blocks: 1019300
Blocks: 984663
blocking-b2g: 1.4? → 1.4+
Whiteboard: [CR 674614]
Whiteboard: [CR 674614] → [caf priority: p2][CR 674614]
You need to log in before you can comment on or make changes to this bug.