Closed Bug 615794 Opened 9 years ago Closed 9 years ago

Fix incorrect usage of ToNearestPixels in FrameLayerBuilder and elsewhere

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- final+

People

(Reporter: roc, Assigned: roc)

References

Details

(Keywords: regression)

Attachments

(6 files, 2 obsolete files)

7.83 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.20 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.36 KB, patch
Details | Diff | Splinter Review
2.16 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
18.22 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
3.02 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
In FrameLayerBuilder in a couple of places we do
  nsIntRect itemVisibleRect =
    aItem->GetVisibleRect().ToNearestPixels(appUnitsPerDevPixel);

This code is assuming that when the item draws, whatever it draws will be
snapped to pixel boundaries. But this assumption is overoptimistic for a couple
of reasons:
-- Some content, such as an element transformed by rotation, simply doesn't
snap its drawing.
-- If the context has a tricky transform on it when we come to draw the
content, we might not snap.

Using ToOutsidePixels here would be the conservative approach and it would give
correct results. However it would mean that for content that *does* snap, but
is not already aligned at pixel boundaries, the visible rect is bigger than the
area that actually gets drawn, so a layer containing only that content could no
longer be treated as opaque. That could be bad.

In BasicLayers we have a couple of places where we do
gfxUtils::ClipToRegionSnapped that have related issues.
blocking2.0: --- → ?
Attachment #494633 - Flags: review?(jmuizelaar) → review+
The previous two patches are conservative and correct. But they can lead to regressions because we've lost information about what's snapped. When a display item draws a snapped rectangle, we can give it a smaller rectangle for the visible nsIntRegion, and a bigger rectangle for the opaque nsIntRegion. This patch adjusts GetBounds and GetOpaqueRegion to return the snapped region if we know the display item is not transformed.

The patch also modifies PresShell::RenderDocument so we tell nsDisplayListBuilder we're in a transform if we're being asked to draw into a context which has a transform (other than a straight transform by pixels).
Attachment #494909 - Flags: review?(tnikkel)
Attached patch Part 1 v2Splinter Review
Fixes a regression in this patch. When converting a ThebesLayer to a ColorLayer we need to ensure that not only is the display item a uniform color inside its bounds, but that those bounds contain the entire visible rect of pixels for the item. If the bounds aren't pixel-aligned there might be a row or column of pixels that aren't fully covered.
Attachment #495462 - Flags: review?(tnikkel)
Attachment #494632 - Attachment is obsolete: true
Attachment #494632 - Flags: review?(tnikkel)
Something in my patch queue --- I think the fixes in this bug --- makes the abspos-*.html transform tests pass with D2D.
Part 3 means the display item GetBounds is not always the frame border-box, so occasionally we can get an abort in RoundedRectIntersectsRect. We should just return false there instead.
Attachment #496318 - Flags: review?(dbaron)
Comment on attachment 496318 [details] [diff] [review]
Part 5: Don't abort in RoundedRectIntersectsRect

r=dbaron
Attachment #496318 - Flags: review?(dbaron) → review+
Blocks a blocker, and fixes regressions too.
blocking2.0: ? → final+
Attached patch Part 3 v2Splinter Review
Snap nsDisplayClip too
Attachment #497754 - Flags: review?(tnikkel)
It's not correct to completely fill the destination context with the
mForcedBackgroundColor. FindOpaqueBackgroundColorFor only checks that
the bounds of the layer's visible region can be filled with the color.
Drawing outside the bounds of the visible region may not necessarily
be ignored (see comments on Layer::SetVisibleRegion).

(This is an existing bug revealed by part 2.)
Attachment #498662 - Flags: review?(tnikkel)
Attachment #498662 - Flags: review?(tnikkel) → review+
Comment on attachment 494909 [details] [diff] [review]
Part 3: Snap bounds of border and background display items to pixels if we're sure they will be snapped

I assume this is obsolete now?
Comment on attachment 494909 [details] [diff] [review]
Part 3: Snap bounds of border and background display items to pixels if we're sure they will be snapped

Yes
Attachment #494909 - Attachment is obsolete: true
Attachment #494909 - Flags: review?(tnikkel)
Could you push your patch queue to your users repo? I will probably want to look at FrameLayerBuilder in the context of your other pending changes.
Attachment #495462 - Flags: review?(tnikkel) → review+
Attachment #497754 - Flags: review?(tnikkel) → review+
Whiteboard: [needs landing]
Blocks 579808, which is a regression.
Keywords: regression
A tryserver build with these patches was green (modulo known random orange) on a tryserver build with "-b d -p all -t none -u all":
http://hg.mozilla.org/try/pushloghtml?changeset=b3f754733e39
I think this is safe to land.
Keywords: checkin-needed
Depends on: 623606
Blocks: 598575
Something in Parts 1.5 - 5 has caused <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c145>, while Part 1 alone fixes Bug 598575. Would you like to treat this in a new bug or would you like to investigate the problem here?
(In reply to comment #18)
> Something in Parts 1.5 - 5 has caused
> <https://bugzilla.mozilla.org/show_bug.cgi?id=363861#c145>

While backout of these checkins helped when printing to PDF my own reduced testcase, this didn't restore printing of semi-transparent areas of a real-life case <http://weblogs.mozillazine.org/asa/archives/2010/12/visualizing_plugin_m.html>. Even if these patches contributed to the new problem, they are not alone responsible.

My apologies.
Depends on: 714346
You need to log in before you can comment on or make changes to this bug.