Closed Bug 1034258 Opened 10 years ago Closed 10 years ago

Rounding of composition bounds causes incorrect displayport margin computation


(Core :: Panning and Zooming, defect)

Gonk (Firefox OS)
Not set



blocking-b2g 2.0+
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v1.3 --- wontfix
b2g-v1.4 --- wontfix
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed


(Reporter: kats, Assigned: kats)




(2 files)

Attached file Log snippet
Use the STR at [1], and then examine the displayport that is being used by layout for the contenteditable scrollable frame (using logging or gdb or whatever). I added some local logging, it is attached.

In the last line of the log, you can see that the bottom margin is negative 0.020407. This ends up making the critical displayport shorter than it should be, resulting in checkerboarding. However if you look at the first line of the log, the APZC computes a displayport (in CSS pixels) that stretches from y=-44 with a height of 144. The composition bounds is 49 layer pixels tall at a zoom of 0.490, which makes it 100 CSS pixels tall. The scroll position is at y=44 CSS pixels, and the scrollable rect is 144 CSS pixels tall. Basically, the first line indicates the displayport should cover the entire scrollable rect and composition bounds, but the last line sets up a value so that it doesn't.

The reason this happens is because the zoom isn't exactly 0.49, it's (480/980) == 0.48979591836735. Which means the composition bounds should be 100*0.48979591836735 = 48.979591836735 parent layer pixels, rather than the rounded 49 parent layer pixels. Therefore, when CalculatePendingDisplayPort calculates the compositionSize variable, it converts the 49 layer pixels back into 100.04166666666604 CSS pixels rather than 100 CSS pixels, introducing a rounding error 0.0416 CSS pixels. This can be seen in line 2 of the log output.

The proper fix here is probably to change mCompositionBounds to be a ParentLayerRect rather than a ParentLayerIntRect, so that it is more precise. I'll write up a patch that does that, but it might be too risky to uplift to 2.0. I might have to find some other hacky way to fix this for 2.0.

This fixes the problem in comment 0. Not sure yet if it breaks other things.
Comment on attachment 8450491 [details] [diff] [review]
Convert mCompositionBounds to ParentLayerRect

Review of attachment 8450491 [details] [diff] [review]:

Pre-emptive r+, looked over it and I don't see anything wrong at least.
Attachment #8450491 - Flags: review+
Local B2G and Linux builds work, gtests pass. Try push:
Blocks a blocker, so it should be 2.0+. However, I'm not sure if we want a smaller fix to uplift to 2.0, so flagging as NO_UPLIFT for now.
blocking-b2g: --- → 2.0?
Whiteboard: [NO_UPLIFT]
blocking-b2g: 2.0? → 2.0+
I rebased this patch for aurora (only one hunk needed manual fixup), did a build-only try push [1], and did some local testing on B2G. I think it's actually not that bad to uplift. Leaving NO_UPLIFT for the moment because I'd like it to bake on m-c for a bit but I won't spend time coming up with a branch-specific smaller fix unless this patch breaks things.

Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.