Last Comment Bug 637852 - Move resolution handling out of layer system into FrameLayerBuilder
: Move resolution handling out of layer system into FrameLayerBuilder
Status: RESOLVED FIXED
QA?
: mobile, perf
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla7
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Andrew Overholt [:overholt]
Mentors:
: 633344 642282 646607 (view as bug list)
Depends on: 630743 668698 669522 681033 692713 766564 778698
Blocks: 637597 656797 662009 633344 639978 646276 651341 662521 663776
  Show dependency treegraph
 
Reported: 2011-03-01 13:29 PST by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-07-30 04:40 PDT (History)
23 users (show)
ehsan: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
7+


Attachments
Test (43.74 KB, text/html)
2011-03-02 15:56 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details
WIP: snap scroll offsets taking resolution into account (8.77 KB, patch)
2011-03-10 15:10 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
WIP: convert FrameLayerBuilder computations to app units (22.81 KB, patch)
2011-03-10 15:10 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
WIP: move resolution handling into FrameLayerBuilder (17.25 KB, patch)
2011-03-10 15:12 PST, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Part 1: Don't snap BasicThebesLayer effective transforms when we're not retaining layers (1.26 KB, patch)
2011-05-12 22:35 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 2: Add BaseRect::ScaleInverseRoundOut API (1.26 KB, patch)
2011-05-18 05:03 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
Details | Diff | Splinter Review
Part 3: Add nsPoint::ScaleToNearestPixels, nsRect::ScaleToNearestPixels, nsRect::ScaleToInsidePixels and nsRect::ScaleToOutsidePixels APIs (7.69 KB, patch)
2011-05-18 05:03 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
Details | Diff | Splinter Review
Part 4: Add nsRegion::ScaleInverseRoundOut and nsRegion::ScaleToOutsidePixels APIs (3.44 KB, patch)
2011-05-18 05:04 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
Details | Diff | Splinter Review
Part 5: Create ContainerParameters structure to carry resolution scale factors through layer creation (23.70 KB, patch)
2011-05-18 05:06 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 6: Implement resolution scaling in FrameLayerBuilder (36.32 KB, patch)
2011-05-18 05:07 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 7: Remove resolution support from D3D10 layers (10.91 KB, patch)
2011-05-18 05:08 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Splinter Review
Part 8: Remove resolution support from D3D9 layers (16.44 KB, patch)
2011-05-18 05:09 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Splinter Review
Part 9: Remove resolution support from BasicLayers, ShadowLayers and GL layers (70.04 KB, patch)
2011-05-18 05:09 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Splinter Review
Part 10: Remove mX/YResolution from ThebesLayer (3.00 KB, patch)
2011-05-18 05:11 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
Details | Diff | Splinter Review
Part 11: Remove ExtendForScaling from nsRect and nsRegion (4.96 KB, patch)
2011-05-18 05:12 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
Details | Diff | Splinter Review
Part 12: Dump layer trees and display lists to stdout instead of stderr so that they stay in order with window.dump() and other output (3.42 KB, patch)
2011-05-18 05:13 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 13: Allow snapping of text baselines to occur when there's a scale in the current transform (3.04 KB, patch)
2011-05-18 05:24 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 14: Try to use snappable rects to draw solid borders instead of using stroke, when a scaling transform is present (7.07 KB, patch)
2011-05-18 05:27 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
Details | Diff | Splinter Review
Part 15: Don't round mOuterRect/mInnerRect if there's a scale factor in the current transform (2.41 KB, patch)
2011-05-18 05:29 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
joe: review+
Details | Diff | Splinter Review
Part 16: Mark newly-passing reftests (5.58 KB, patch)
2011-05-18 05:31 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 17: Tighten up AreLayersMarkedActive so we track exactly what properties are changing. Also, clamp resolution to power-of-2 only if the frame's transform has a scale (11.57 KB, patch)
2011-05-18 05:35 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 18: Support computing the "residual transform" for a ThebesLayer --- the difference between its snapped transform and the ideal transform --- and use it to align ThebesLayer drawing for transform (15.68 KB, patch)
2011-05-18 06:14 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 19: Test (from bug 633344) (2.92 KB, patch)
2011-05-18 06:17 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 20: Allow fast scrolling within transformed content (983 bytes, patch)
2011-05-18 06:18 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 21: Skip invalidation if aRegionToInvalidateIsEmpty (note that ScaleRoundOut on an empty bounds rect can return a non-empty rect) (5.78 KB, patch)
2011-05-18 06:21 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 22: Detect when the contents of a ThebesLayer have shifted by a subpixel amount and repaint the entire layer when that happens (7.10 KB, patch)
2011-05-18 06:22 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 6 v2 (36.46 KB, patch)
2011-05-31 22:10 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 23: fix incorrect use of 'abs' (3.26 KB, patch)
2011-06-20 05:15 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 24: fix scale/translate order (3.79 KB, patch)
2011-06-20 05:59 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 25: Add nsSize ScaleToNearestPixels (1.76 KB, patch)
2011-06-20 18:39 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Part 26: Ensure that FrameMetrics coordinates are correctly stored as layer coordinates (5.65 KB, patch)
2011-06-20 18:40 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
27: Fix RenderFrameParent rendering to handle transforms on the root layer, and fix bugs with parameters being modified (6.45 KB, patch)
2011-06-20 18:46 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
ben: review+
Details | Diff | Splinter Review
manual testcase (1.63 KB, text/html)
2011-06-20 18:52 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
modified testcase (1.67 KB, text/html)
2011-06-20 19:32 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details
Part 28: Fix nsDisplayScrollInfoLayer abort by ensuring nsDisplayScrollInfoLayer is before all other nsDisplayScrollInfos in z-order (2.12 KB, patch)
2011-06-20 19:34 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-01 13:29:00 PST
To avoid seaming in ThebesLayerBuffer when we use scrolling optimizations, the displayport and scroll offsets need to fall on whole device-pixel boundaries, taking resolution into account.  With a non-identify resolution, we need to specify scroll offsets with fractional components to get device-pixel snapping.  Currently nsGlobalWindow::ScrollTo() only accepts integer scroll offsets.  We probably can't change that, so might need to add an nsIDOMWindowUtils backdoor.

One issue that remains was raised in bug 635035 comment 34: we can end up using the ThebesLayerBuffer self-copy code for non-identity resolution with transformed ThebesLayers.  AFAIK nothing ensures, or can ensure, that those scrolls are snapped to device pixels.  If we get the rounding wrong, then we can get seaming there.  Worth pondering, but that's already a problem on m-c tip so could be fixed in another bug.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-01 13:29:54 PST
This work doesn't really depend on bug 630743 code-wise, but there's no motivation to land this until after bug 630743.
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-01 13:46:42 PST
It strikes me that potential fallout from this is interfaces that return the scroll offset in int CSS pixels.  Not sure how big of a deal that will be.
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-02 15:56:27 PST
Created attachment 516439 [details]
Test

(To reiterate, this test is only relevant after bug 630743 is fixed.)

STR
 (1) Load test
 (2) Press "Dance".  (URL can be out of view or not, doesn't matter after 630743.)

The test scrolls right/down alternately by 30 CSS pixels = 22.5 device pixels with the scale applied.  If the offsets aren't snapped, the text in the page should dance.  This isn't testing *fennec's* setting of the scroll offset during user-initiated panning, but rather content-initiated scrolling.  If both go through the same code path, this test should be OK.  If not, a good test will be hard.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-03 14:27:08 PST
With a build including bug 630743, in the updated test at http://people.mozilla.com/~cjones/test-637852.html I see jumping text, on the galaxy tab in portrait.  (Which makes sense.)
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-05 14:54:20 PST
Forgot to mark this yesterday.  I've got a patch and some ideas.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-05 14:56:17 PST
This is the second part of bug 630743, and should block for the same reason.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-08 02:05:01 PST
Update
 - good news: fixed text-positioning problems
 - bad news: patches are invasive, break some assumptions, need more work

I'm out of steam for the night, but haven't given up.  Will resume tomorrow.  This bug is looking riskier.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-08 13:23:21 PST
This has become too much of a tar baby for me to target it for 4.0, with a clear conscience.  HOWEVER
 - I think we can use this work to improve the platform APIs fennec is using
 - I think this and blockees should land ASAP for a 4.x release
 - I propose we fork this work off into a prototype hybrid m-c/m-b project branch and continue on

We've got enough goodies for a killer 4.0; let's focus on less risky stuff.

(To relevant parties:) Let's chat about this future work in-person soon, perhaps after the fennec meeting tomorrow.
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-08 14:36:22 PST
The fundamental problem here was threefold
 (1) displayport not on device-pixel boundaries accounting for resolution.  Seemed fixed in my testing by bug 630743, wasn't extensive though.
 (2) scroll offsets not on device-pixel boundaries.  This would lead to seaming if all other issues were fixed with bug 635035.
 (3) cairo's fallback text drawing code snaps glyph positions to integral offsets from surfaces.  FrameLayerBuilder rounds visible regions out to pixel boundaries *before* resolution is applied, which stomps on fixes from (1) and (2).  This still leads to dancing text with both those fixed.

I tried a hacky fix for (3) that translates the gfxContext by the fractional device pixel the bad snapping stomps on, but didn't get it to work.  I gave up pretty quickly in favor of a "real fix."  I'm not sure this could work, but maybe; it also has issues in possibly leaving the bottom/right part of the surface unpainted.  Would need care.

I've got a pretty good start on fixing all three issues "for real".  The patches I have now do
 - nudge scroll offsets to device-pixel boundaries taking resolution into account (i.e., scroll offset might fall on fractional CSS pixel)
 - switch FrameLayerBuilder computations to be in app units, only rounding out to device pixels when needed
 - make FrameLayerBuilder aware of resolution, and snap to pixels while taking resolution into account (relying on (1) and (2) above).  This makes resolution-scaled thebeslayer coordinates be in actual device pixels, which is what we should have done all along.
 - have FrameLayerBuilder::DrawThebesLayer responsible for mapping real-device-pixel ThebesLayer coordinates for painting/invalidating back to possibly-fractional-CSS-pixel app unit values, taking resolution into account.

It's the change in semantics of resolution-scaled thebeslayers that introduces all the risk.  This requires
 - update ThebesLayerBuffer and ThebesLayer.  Mostly done, but breaks resolution-scaling for CSS transforms.  That needs more thought.
 - updates as necessary for ContainerLayers and other layers.  Not sure what's needed here yet.
 - fix viewconfig APIs for the frontend.  The scaleX/Y stuff is still OK, but the scroll offsets reported to the frontend in layer FrameMetrics are just bogus now; they're the original snapped-out values that caused the problems.  These offsets aren't too terribly far off, so it might be possible to hack in some fixes.
 - fix sub-frame scrolling.  This was relying on inheriting resolution from the root scroll frame, AFAICT.  Not too hard to hack in.
 - guarantee device-pixel displayport/scroll offset for sub-frames.  Otherwise these will have the same dancing+seaming we're fixing for root frames.

I may have forgotten something, writing quickly.  All this adds risk to desktop FF too, not just mobile.

Moving forward, I think we have these options
 (A) Find a hacky fix for dancing text for 4.0.  Might be possible, not sure.  This wouldn't necessarily unblock 634397 and 635035.
 (B) Hacky guarantee that ThebesLayerBuffer rotations always fall on device pixels.  This would unblock 634397 and 635035.
 (C) We could get A and B by continuing to hack away at these patches.
 (D) Finish these patches for 4.x, and change the APIs used by the frontend to always deal in real device pixels.  This will make everyone's life easier.
 (E) Ship 4.0 with current resolution API, deprecate it in favor of something else post-4.0.
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-08 14:57:01 PST
(This work also fixes the jitter in buffer sizes noted in bug 635035.)
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-10 15:10:24 PST
Created attachment 518539 [details] [diff] [review]
WIP: snap scroll offsets taking resolution into account
Comment 12 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-10 15:10:58 PST
Created attachment 518540 [details] [diff] [review]
WIP: convert FrameLayerBuilder computations to app units

Partially broken.
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-10 15:12:08 PST
Created attachment 518541 [details] [diff] [review]
WIP: move resolution handling into FrameLayerBuilder

Keeps ThebesLayer coordinates in actual device pixels, moves map->CSS pixels into FrameLayerBuilder.

Very broken, but fixes test-positioning problems.  This patch and the last two show where the original plan here was headed.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-10 15:23:18 PST
Moving all resolution handling out of ThebesLayers into FrameLayerBuilder makes sense. You're right, it should have been done that way from the start; I thought we could hide the resolution abstraction behind the ThebesLayer API, but clearly we can't.
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-10 15:25:10 PST
The downside to that is it promotes resolution to a full-class layout citizen alongside the zooms etc., which means it will leak out into a lot more code.  If there's a better way to accomplish the same goal (maybe the goal should be re-evaluated too) while re-using more existing code, I suspect everyone would be happier.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-10 15:51:29 PST
It can probably be contained within FrameLayerBuilder.
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-10 17:39:31 PST
We need at least to take the resolution into account when scrolling, so that the scroll offset maps to a device pixel and we can rotate/self-copy properly.  See the first WIP above.  Is there a better way to do that?
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-10 17:40:05 PST
(Oops, ignoring all the logging gunk in the patches.)
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-10 18:06:16 PST
I can't think of a better way right now!
Comment 20 Brad Lassey [:blassey] (use needinfo?) 2011-03-30 10:55:00 PDT
*** Bug 642282 has been marked as a duplicate of this bug. ***
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-30 14:04:21 PDT
*** Bug 646607 has been marked as a duplicate of this bug. ***
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-30 14:18:29 PDT
IIUC, Rob is proposing to move the scroll-offset-frobbing into FrameLayerBuilder, which would obsolete the first patch above.
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-30 14:23:50 PDT
My basic plan here for FrameLayerBuilder is to pass down an extra scale factor to be applied to child layers as we build the layer tree from the top down. So if a container has a large scaling-up transform, we'll pass that scale-up down to the children, keeping the transforms set on containers to some small scale-down.

That means that when we construct a layer we already know what its resolution is going to be, so we can snap things appropriately when we convert from appunits and we can keep ThebesLayer construction working with nsIntRegions etc. The resolution scale is finally applied either by adjusting the transform on non-ThebesLayer leaf layers, or in DrawThebesLayer.

Scrolling is still going to be a problem. I think the best idea is to support arbitrary appunit scroll offsets, and simply observe when the scroll offset changes by an amount that is not a multiple of layer pixels and rerender the layer in that case. Then we try to avoid that with scrollframes making a best-effort attempt to move their scroll positions by a multiple of layer pixels. This could include peeking at the current layer tree.
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-03-30 17:20:30 PDT
I'm not working on this anymore.
Comment 25 Brad Lassey [:blassey] (use needinfo?) 2011-05-03 13:08:04 PDT
roc, what's the plan/status here?
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-03 15:17:11 PDT
I've been working on this for a couple of weeks. I'm nearly done with the move of resolution handling into FrameLayerBuilder. I'm not sure what else needs to be done here once I've finished that.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-12 22:35:51 PDT
Created attachment 532123 [details] [diff] [review]
Part 1: Don't snap BasicThebesLayer effective transforms when we're not retaining layers
Comment 28 Timothy Nikkel (:tnikkel) 2011-05-13 12:05:43 PDT
Comment on attachment 532123 [details] [diff] [review]
Part 1: Don't snap BasicThebesLayer effective transforms when we're not retaining layers

Seems reasonable enough.
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:03:05 PDT
Created attachment 533241 [details] [diff] [review]
Part 2: Add BaseRect::ScaleInverseRoundOut API
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:03:51 PDT
Created attachment 533242 [details] [diff] [review]
Part 3: Add nsPoint::ScaleToNearestPixels, nsRect::ScaleToNearestPixels, nsRect::ScaleToInsidePixels and nsRect::ScaleToOutsidePixels APIs
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:04:38 PDT
Created attachment 533243 [details] [diff] [review]
Part 4: Add nsRegion::ScaleInverseRoundOut and nsRegion::ScaleToOutsidePixels APIs
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:06:29 PDT
Created attachment 533244 [details] [diff] [review]
Part 5: Create ContainerParameters structure to carry resolution scale factors through layer creation
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:07:44 PDT
Created attachment 533245 [details] [diff] [review]
Part 6: Implement resolution scaling in FrameLayerBuilder
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:08:23 PDT
Created attachment 533246 [details] [diff] [review]
Part 7: Remove resolution support from D3D10 layers
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:09:03 PDT
Created attachment 533248 [details] [diff] [review]
Part 8: Remove resolution support from D3D9 layers
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:09:56 PDT
Created attachment 533250 [details] [diff] [review]
Part 9: Remove resolution support from BasicLayers, ShadowLayers and GL layers
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:11:49 PDT
Created attachment 533251 [details] [diff] [review]
Part 10: Remove mX/YResolution from ThebesLayer
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:12:30 PDT
Created attachment 533252 [details] [diff] [review]
Part 11: Remove ExtendForScaling from nsRect and nsRegion
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:13:37 PDT
Created attachment 533253 [details] [diff] [review]
Part 12: Dump layer trees and display lists to stdout instead of stderr so that they stay in order with window.dump() and other output

Not really needed in this bug, but I got annoyed with the output and I have to put the patch somewhere. Anyway it's trivial.
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:24:41 PDT
Created attachment 533254 [details] [diff] [review]
Part 13: Allow snapping of text baselines to occur when there's a scale in the current transform
Comment 41 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:27:18 PDT
Created attachment 533256 [details] [diff] [review]
Part 14: Try to use snappable rects to draw solid borders instead of using stroke, when a scaling transform is present
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:29:30 PDT
Created attachment 533257 [details] [diff] [review]
Part 15: Don't round mOuterRect/mInnerRect if there's a scale factor in the current transform
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:31:24 PDT
Created attachment 533258 [details] [diff] [review]
Part 16: Mark newly-passing reftests
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 05:35:15 PDT
Created attachment 533259 [details] [diff] [review]
Part 17: Tighten up AreLayersMarkedActive so we track exactly what properties are changing. Also, clamp resolution to power-of-2 only if the frame's transform has a scale
Comment 45 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 06:14:44 PDT
Created attachment 533263 [details] [diff] [review]
Part 18: Support computing the "residual transform" for a ThebesLayer --- the difference between its snapped transform and the ideal transform --- and use it to align ThebesLayer drawing for transform
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 06:17:05 PDT
Created attachment 533265 [details] [diff] [review]
Part 19: Test (from bug 633344)
Comment 47 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 06:18:42 PDT
Created attachment 533266 [details] [diff] [review]
Part 20: Allow fast scrolling within transformed content

Now that resolution-handling has moved into layout, we can start to tackle the issues this bug is actually about. This patch lets us try to use accelerated scrolling within transformed content, so we can test scrolling with non-identity resolution.
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 06:21:17 PDT
Created attachment 533267 [details] [diff] [review]
Part 21: Skip invalidation if aRegionToInvalidateIsEmpty (note that ScaleRoundOut on an empty bounds rect can return a non-empty rect)

This fixes a bug that crept in earlier and adds some warnings to the ScaleRoundOut methods.

I considered making ScaleRoundOut return an empty rect for an empty input rect, but that relies on interpreting rectangles as areas, not sets of points, and we sometimes use the latter interpretation. We might need a different method, ScaleRoundOutEdges or something like that, but I don't want to deal with that here.
Comment 49 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 06:22:57 PDT
Created attachment 533268 [details] [diff] [review]
Part 22: Detect when the contents of a ThebesLayer have shifted by a subpixel amount and repaint the entire layer when that happens
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-18 06:29:36 PDT
Phew!

This patch queue passes on tryserver.

Part 22 forces us to repaint entire ThebesLayers if a scroll operation ends up not being ThebesLayer-pixel-aligned. That's better than the current trunk situation where we always repaint everything if there's a non-identity scale. But we can do even better by having our various scroll operations try to choose scroll amounts that are ThebesLayer-pixel-aligned. This is possible both for user-initiated scrolling (where we have some flexibility in how much to scroll) and for script-initiated scrolling (where setting scrollLeft/scrollTop to some value N only requires that the underlying scroll offset return N after rounding to the nearest CSS pixel).

However, let's do that in a separate bug, if and when we do it.
Comment 51 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-19 13:54:58 PDT
*** Bug 633344 has been marked as a duplicate of this bug. ***
Comment 52 Timothy Nikkel (:tnikkel) 2011-05-19 16:03:54 PDT
I little outline of what are are trying to accomplish by adding resolution to layers and how we do it might be helpful here as I didn't follow the original introduction of resolution into layers very closely.
Comment 53 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-20 07:11:42 PDT
When a transform scales content up, if we just render the content at its normal resolution and scale that up, it will look pixellated. We can avoid this by rendering the content at a higher resolution, preferably one matching the resolution at which the content will eventually be drawn. The idea is that one "device pixel" as seen by layout will map to more than one pixel in the ThebesLayer into which we render the content. When we draw into that ThebesLayer, we draw with a scale in the CTM of the gfxContext used for drawing. Make sense so far?

When a transform scales content down, instead of worrying about quality we worry about buffer size. By using less than one pixel in the ThebesLayer per "device pixel" as seen by layout, we can save memory (and possibly quality too if we can avoid having to resample the ThebesLayer when we composite it).

When a transform's scale is constantly changing, we don't want to keep rerendering the ThebesLayer due to resolution changes. So in some cases we round up the resolution to the nearest power of two in each direction. This guarantees that during compositing the content is being scaled down (less lossy than scaling up), but it's being scaled down by less than a factor of 2 (scaling down by more than that means we may be wasting VRAM on oversized buffers; also, many scaling algorithms produce poor quality results for scale-down by more than a factor of 2).
Comment 54 Timothy Nikkel (:tnikkel) 2011-05-20 10:17:02 PDT
Besides CSS transforms, where is this resolution support all used?
Comment 55 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-21 06:06:36 PDT
The presshell resolution API also triggers it.
Comment 56 Timothy Nikkel (:tnikkel) 2011-05-24 21:29:05 PDT
Comment on attachment 533245 [details] [diff] [review]
Part 6: Implement resolution scaling in FrameLayerBuilder

>@@ -905,23 +926,24 @@ ContainerState::FindOpaqueBackgroundColo
>+    rect.ScaleInverseRoundOut(mParameters.mXScale, mParameters.mYScale);

Why do we need to scale this rect? Aren't we comparing it only with things in the same container layer and so same scale?
Comment 57 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-24 22:23:40 PDT
Appunits are always before scaling, integer (layer) coordinates are always after scaling. Here we're going from layer coordinates to appunits so we have to inverse-scale.
Comment 58 Timothy Nikkel (:tnikkel) 2011-05-25 13:02:13 PDT
Ah, okay. If there was a ScaleInverseToAppUnits that did this in one step it would have been clearer.
Comment 59 Timothy Nikkel (:tnikkel) 2011-05-30 23:07:32 PDT
Comment on attachment 533245 [details] [diff] [review]
Part 6: Implement resolution scaling in FrameLayerBuilder

@@ -1676,30 +1778,31 @@ FrameLayerBuilder::BuildContainerLayerFo
+  if (aChildren.IsOpaque() && !aChildren.NeedsTransparentSurface()) {
+    bounds.ScaleRoundOut(scaleParameters.mXScale, scaleParameters.mYScale);
+    if (bounds.Contains(pixBounds.ToAppUnits(appUnitsPerDevPixel))) {
+      // Clear CONTENT_COMPONENT_ALPHA
+      flags = Layer::CONTENT_OPAQUE;
+    }

Shouldn't this be ScaleRoundIn? Or should we convert pixBounds to appunits and then scale it and then compare it to bounds?
Comment 60 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-31 02:36:19 PDT
It should be ScaleRoundIn, good catch.
Comment 61 Timothy Nikkel (:tnikkel) 2011-05-31 21:52:56 PDT
Comment on attachment 533245 [details] [diff] [review]
Part 6: Implement resolution scaling in FrameLayerBuilder

@@ -562,24 +562,30 @@ void nsDisplayList::PaintForFrame(nsDisp
+  // Root is being scaled up by the X/Y resolution. Scale it back down.
+  gfx3DMatrix rootTransform = root->GetTransform()*
+    gfx3DMatrix::Scale(1.0f/containerParameters.mXScale,
+                       1.0f/containerParameters.mYScale, 1.0f);

And then we don't do anything with rootTransform.
Comment 62 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-31 22:10:17 PDT
Created attachment 536528 [details] [diff] [review]
Part 6 v2

Good catch. Here's the updated patch with that fixed. We just need to set that transform on the root layer.
Comment 63 Joe Drew (not getting mail) 2011-06-02 07:25:17 PDT
Comment on attachment 533241 [details] [diff] [review]
Part 2: Add BaseRect::ScaleInverseRoundOut API

NS_ceil and NS_floor are both gone now; just use the standard C functions.
Comment 64 Joe Drew (not getting mail) 2011-06-02 07:41:18 PDT
Comment on attachment 533242 [details] [diff] [review]
Part 3: Add nsPoint::ScaleToNearestPixels, nsRect::ScaleToNearestPixels, nsRect::ScaleToInsidePixels and nsRect::ScaleToOutsidePixels APIs

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

::: gfx/src/nsPoint.h
@@ -71,3 @@
>    return nsIntPoint(
> -      NSToIntRoundUp(NSAppUnitsToDoublePixels(x, aAppUnitsPerPixel)),
> -      NSToIntRoundUp(NSAppUnitsToDoublePixels(y, aAppUnitsPerPixel)));

It is weird to me that this is "Nearest" pixels but we just explicitly round up. Obviously this is something that has existed for a while though :)
Comment 65 Joe Drew (not getting mail) 2011-06-02 07:43:08 PDT
Comment on attachment 533243 [details] [diff] [review]
Part 4: Add nsRegion::ScaleInverseRoundOut and nsRegion::ScaleToOutsidePixels APIs

Review of attachment 533243 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 66 Joe Drew (not getting mail) 2011-06-02 07:44:18 PDT
Comment on attachment 533251 [details] [diff] [review]
Part 10: Remove mX/YResolution from ThebesLayer

Review of attachment 533251 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 67 Joe Drew (not getting mail) 2011-06-02 07:47:39 PDT
Comment on attachment 533252 [details] [diff] [review]
Part 11: Remove ExtendForScaling from nsRect and nsRegion

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

one r+ for every line removed
Comment 68 Joe Drew (not getting mail) 2011-06-02 07:54:06 PDT
Comment on attachment 533256 [details] [diff] [review]
Part 14: Try to use snappable rects to draw solid borders instead of using stroke, when a scaling transform is present

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

This is a weak r+; I don't know enough about nsCSSRenderingBorders to make it stronger.
Comment 69 Timothy Nikkel (:tnikkel) 2011-06-02 08:47:21 PDT
(In reply to comment #64)
> It is weird to me that this is "Nearest" pixels but we just explicitly round
> up. Obviously this is something that has existed for a while though :)

The "Up" refers to the handling of rounding 0.5, and we have to pick one way to round it.
Comment 70 Timothy Nikkel (:tnikkel) 2011-06-02 23:01:22 PDT
Comment on attachment 536528 [details] [diff] [review]
Part 6 v2

There are a few places where we convert from nsInt* to ns* by the two step process ToAppUnits then ScaleInverse. Why not make a function that does this?
Comment 71 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 23:07:32 PDT
I see only two places where we could use that.

In DrawThebesLayer it would be hard to use that convenience function, because we need to apply the offset as well. Adding the offset to aRegionToDraw would require the creation of an extra temporary region.
Comment 72 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-02 23:07:52 PDT
(but I'll do it for those two places if you want)
Comment 73 Oleg Romashin (:romaxa) 2011-06-03 12:20:04 PDT
I've tested these patches with mobile browser, and that looks mostly broken.
1) Initial scale is wrong
2) Offsets are wrong when you scroll it down.
3) some black areas...

I was testing on fennec with accelerated layers enabled.
without these patches it works fine (but slow updates)
Comment 74 Timothy Nikkel (:tnikkel) 2011-06-03 14:02:25 PDT
Comment on attachment 536528 [details] [diff] [review]
Part 6 v2

Ok, fair enough.
Comment 75 Timothy Nikkel (:tnikkel) 2011-06-15 12:49:29 PDT
Comment on attachment 533266 [details] [diff] [review]
Part 20: Allow fast scrolling within transformed content

Part 22 is what allows us to do this?
Comment 76 Timothy Nikkel (:tnikkel) 2011-06-15 12:57:58 PDT
Comment on attachment 533263 [details] [diff] [review]
Part 18: Support computing the "residual transform" for a ThebesLayer --- the difference between its snapped transform and the ideal transform --- and use it to align ThebesLayer drawing for transform

+  /**
+   * True when
+   */
+  bool mAllowResidualTranslation;

You trailed off there.

In bug 630835 we fixed some problems where we reported our layer as opaque but it wasn't actually opaque because we didn't quite paint everything. Is adding this residual translation going to cause problems like that?

Why do we want to snap the layer compositing transform and then perform a "fixup" before drawing into the thebes layer, as opposed to just not snapping the layer compositing transform?
Comment 77 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-15 18:07:45 PDT
(In reply to comment #75)
> Comment on attachment 533266 [details] [diff] [review] [review]
> Part 20: Allow fast scrolling within transformed content
> 
> Part 22 is what allows us to do this?

Yes, part 22 fixes a regression that part 20 causes.

(In reply to comment #76)
> +  /**
> +   * True when
> +   */
> +  bool mAllowResidualTranslation;
> 
> You trailed off there.

I'll change it to "See SetAllowResidualTranslation."

> In bug 630835 we fixed some problems where we reported our layer as opaque
> but it wasn't actually opaque because we didn't quite paint everything. Is
> adding this residual translation going to cause problems like that?

No. See the comment above FrameLayerBuilder::DrawThebesLayer. As long as the opaque stuff is snapped we should be fine.

> Why do we want to snap the layer compositing transform and then perform a
> "fixup" before drawing into the thebes layer, as opposed to just not
> snapping the layer compositing transform?

Because unsnapped transforms, e.g. a translation by half a pixel, cause horrible blur due to resampling (and are slow if you don't have a GPU). And you still have the problem that pixels are being affected that you didn't expect; e.g. nearest-neighbour sampling doesn't blur, but it behaves just like snapping the transform except you don't know you're doing it.
Comment 78 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-19 17:12:03 PDT
The reason this breaks Fennec is confusion about whether FrameMetrics::mViewport/mContentSize/mViewportScrollOffset/mDisplayPort are CSS pixels or layer coordinates. Currently they're the same in Fennec, but the work in this bug makes layer coordinates not always be CSS pixels.
Comment 79 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-19 18:07:57 PDT
I think I'll make FrameMetrics coordinates always be in layer coordinates, that makes sense given that they are a property of the ContainerLayer.
Comment 80 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 05:15:19 PDT
Created attachment 540437 [details] [diff] [review]
Part 23: fix incorrect use of 'abs'

This fixes an obvious bug. The test fails without the patch, passes with it.
Comment 81 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 05:59:57 PDT
Created attachment 540442 [details] [diff] [review]
Part 24: fix scale/translate order
Comment 82 Timothy Nikkel (:tnikkel) 2011-06-20 14:26:41 PDT
Comment on attachment 540442 [details] [diff] [review]
Part 24: fix scale/translate order

I remember thinking about this during the review, but it got lost in my head somewhere.
Comment 83 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 18:39:31 PDT
Created attachment 540649 [details] [diff] [review]
Part 25: Add nsSize ScaleToNearestPixels
Comment 84 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 18:40:35 PDT
Created attachment 540650 [details] [diff] [review]
Part 26: Ensure that FrameMetrics coordinates are correctly stored as layer coordinates
Comment 85 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 18:46:35 PDT
Created attachment 540652 [details] [diff] [review]
27: Fix RenderFrameParent rendering to handle transforms on the root layer, and fix bugs with parameters being modified

Even though I reviewed this RenderFrameParent code, now that I've worked on it, I don't like it very much :-). It could probably be rewritten to be much nicer. It also sort of assumes that transforms are just scales + translations, which isn't good. But I don't want to rewrite it now, especially not in this bug, and I think its problems are fixable within RenderFrameParent (and possibly nsContentView and the Fennec chrome code); the FrameMetrics interface shouldn't need to be changed.

I thought BuildListForLayer would need to be changed, but event handling seems to work fine. As far as I can tell, Fennec works great at this point.
Comment 86 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 18:52:13 PDT
Created attachment 540657 [details]
manual testcase

I used this testcase to help debug the Fennec problems. Do we have automated tests for the nsContentView APIs?
Comment 87 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 19:32:06 PDT
Created attachment 540663 [details]
modified testcase

Here's a slightly modified testcase that tests non-root-viewport scrolling. It exposes some more bugs :-(
Comment 88 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 19:34:24 PDT
Created attachment 540664 [details] [diff] [review]
Part 28: Fix nsDisplayScrollInfoLayer abort by ensuring nsDisplayScrollInfoLayer is before all other nsDisplayScrollInfos in z-order

Didn't this get changed recently? Anyway, it's broken. BorderBackground is the bottom-most list so this should be fine.
Comment 89 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 20:59:33 PDT
Hmm, all the bugs I can reproduce using the testcase in comment #87 exist in trunk Fennec already, so I'm not going to try to fix them here.
Comment 90 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-20 21:10:15 PDT
Comment on attachment 540664 [details] [diff] [review]
Part 28: Fix nsDisplayScrollInfoLayer abort by ensuring nsDisplayScrollInfoLayer is before all other nsDisplayScrollInfos in z-order

I thought this looked familiar, you fixed it in bug 656041.
Comment 91 :Ms2ger (⌚ UTC+1/+2) 2011-06-21 03:46:32 PDT
Comment on attachment 533257 [details] [diff] [review]
Part 15: Don't round mOuterRect/mInnerRect if there's a scale factor in the current transform

>From: Robert O'Callahan <robert@ocallahan.org>
>
>Bug 673852. Part 15: Don't round mOuterRect/mInnerRect if there's a scale factor in the current transform. r=joe

Bug number is wrong.
Comment 92 Benjamin Stover (:stechz) 2011-06-21 15:43:13 PDT
Comment on attachment 540652 [details] [diff] [review]
27: Fix RenderFrameParent rendering to handle transforms on the root layer, and fix bugs with parameters being modified

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

Overall, looks good except I have some confusion about your new use of ComputeShadowTreeTransform.

::: layout/ipc/RenderFrameParent.cpp
@@ +230,1 @@
>                    nsDisplayListBuilder* aBuilder,

Yes, this is more readable if we aren't changing aTransform. As I grok it, the only change you made in the function is this refactoring. Am I right?

@@ +303,5 @@
>  
>      ViewTransform viewTransform = ComputeShadowTreeTransform(
>        aFrame, aFrameLoader, metrics, view->GetViewConfig(),
> +      1 / (GetXScale(currentTransform)*layerTransform.mXScale),
> +      1 / (GetYScale(currentTransform)*layerTransform.mYScale)

I'm confused by this. BuildListForLayer only includes the inverse scale for the transforms up to but not including the current layer, but this includes the current layer. Why? I should say I'm having a really hard time following this code now that I've been out of it for a while...

@@ +314,5 @@
> +      layerTransform.mTranslation = viewTransform.mTranslation;
> +      // Apply the root frame translation *before* we do the rest of the transforms.
> +      nsIntPoint rootFrameOffset = GetRootFrameOffset(aFrame, aBuilder);
> +      shadowTransform = shadowTransform *
> +          gfx3DMatrix::Translation(float(rootFrameOffset.x), float(rootFrameOffset.y), 0.0);

OK, this makes sense. Root frame translation precedes normal layer transformation precedes shadow transformation. Good catch.
Comment 93 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-21 19:49:37 PDT
(In reply to comment #92)
> ::: layout/ipc/RenderFrameParent.cpp
> @@ +230,1 @@
> >                    nsDisplayListBuilder* aBuilder,
> 
> Yes, this is more readable if we aren't changing aTransform. As I grok it,
> the only change you made in the function is this refactoring. Am I right?

Yes.

> @@ +303,5 @@
> >  
> >      ViewTransform viewTransform = ComputeShadowTreeTransform(
> >        aFrame, aFrameLoader, metrics, view->GetViewConfig(),
> > +      1 / (GetXScale(currentTransform)*layerTransform.mXScale),
> > +      1 / (GetYScale(currentTransform)*layerTransform.mYScale)
> 
> I'm confused by this. BuildListForLayer only includes the inverse scale for
> the transforms up to but not including the current layer, but this includes
> the current layer. Why? I should say I'm having a really hard time following
> this code now that I've been out of it for a while...

In TransformShadowTree, the layer's transform is applied on top of the viewTransform, so the inverse transform should include the layer's transform. Does that make sense?

I don't know why BuildListForLayer doesn't need that. I think it should for the same reasons, but I didn't find any bugs with that so I didn't change it.
Comment 94 Benjamin Stover (:stechz) 2011-06-21 20:04:48 PDT
Comment on attachment 540652 [details] [diff] [review]
27: Fix RenderFrameParent rendering to handle transforms on the root layer, and fix bugs with parameters being modified

> In TransformShadowTree, the layer's transform is applied on top of the
> viewTransform, so the inverse transform should include the layer's
> transform. Does that make sense?

Ah. Yes.

> 
> I don't know why BuildListForLayer doesn't need that. I think it should for
> the same reasons, but I didn't find any bugs with that so I didn't change it.

This worries me. Let's put an XXX comment in BuildListForLayer, as I would expect it to be symmetric with your logic here.

r+ with XXX comment and space-separated * operator mentioned above. Let's get this landed! :D
Comment 95 :Ehsan Akhgari 2011-06-22 12:52:45 PDT
http://hg.mozilla.org/mozilla-central/rev/7853e5cf72f7
http://hg.mozilla.org/mozilla-central/rev/c9dd59391c7d
http://hg.mozilla.org/mozilla-central/rev/b53df216be61
http://hg.mozilla.org/mozilla-central/rev/1da2c78a1a72
http://hg.mozilla.org/mozilla-central/rev/31c47102a6fc
http://hg.mozilla.org/mozilla-central/rev/45b7622bc948
http://hg.mozilla.org/mozilla-central/rev/198d6364abb9
http://hg.mozilla.org/mozilla-central/rev/602d13dcab53
http://hg.mozilla.org/mozilla-central/rev/123d2c2f6260
http://hg.mozilla.org/mozilla-central/rev/5b2a58c9c279
http://hg.mozilla.org/mozilla-central/rev/a63a96b9571e
http://hg.mozilla.org/mozilla-central/rev/e552be420a02
http://hg.mozilla.org/mozilla-central/rev/2c7a42271f31
http://hg.mozilla.org/mozilla-central/rev/9c05bcab628f
http://hg.mozilla.org/mozilla-central/rev/9ae31ef1ae05
http://hg.mozilla.org/mozilla-central/rev/c6c5f217fedf
http://hg.mozilla.org/mozilla-central/rev/500265c61f37
http://hg.mozilla.org/mozilla-central/rev/4c323a5e40c2
http://hg.mozilla.org/mozilla-central/rev/e96e2e5829cd
http://hg.mozilla.org/mozilla-central/rev/c9f644aa2fa5
http://hg.mozilla.org/mozilla-central/rev/3d7fda340878
http://hg.mozilla.org/mozilla-central/rev/6256bcafbdf2
http://hg.mozilla.org/mozilla-central/rev/dcfa8de9746a
http://hg.mozilla.org/mozilla-central/rev/c18bdf8b0b76
http://hg.mozilla.org/mozilla-central/rev/8b1e793fbf45
http://hg.mozilla.org/mozilla-central/rev/21a67ac790f1
http://hg.mozilla.org/mozilla-central/rev/e302cef502f6
Comment 96 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-05 17:52:17 PDT
The Mac Tp4 regression was caused by part 22.
Comment 97 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-07-10 18:25:36 PDT
I did some experiments on try.

It's definitely this block:
886   if (activeScrolledRootTopLeft != data->mActiveScrolledRootPosition) {
887     data->mActiveScrolledRootPosition = activeScrolledRootTopLeft;
888     nsIntRect invalidate = layer->GetValidRegion().GetBounds();
889     layer->InvalidateRegion(invalidate);
890   }
Setting the condition to false makes the regression go away.

I added some instrumentation to see when that's getting hit with nonempty valid region. I get results like this:
http://tinderbox.mozilla.org/showlog.cgi?log=Try/1310106496.1310107273.15185.gz&fulltext=1

NOISE: Cycle 8: loaded http://localhost/page_load_test/tp4/www.ku6.com/www.ku6.com/index.html (next: http://localhost/page_load_test/tp4/www.interia.pl/www.interia.pl/index.html)
NOISE: NOTE! Invalidating area 0,0,1009,654, offset was 0.000000,0.000000, is 0.000000,0.433333
NOISE: NOTE! Invalidating area 0,0,1009,656, offset was 0.000000,0.433333, is 0.000000,-0.500000
NOISE: NOTE! Invalidating area 0,0,1009,667, offset was 0.000000,-0.500000, is 0.000000,0.066667
NOISE: NOTE! Invalidating area 0,0,1009,673, offset was 0.000000,0.066667, is 0.000000,0.433333
NOISE: NOTE! Invalidating area 0,0,1009,676, offset was 0.000000,0.433333, is 0.000000,0.466667
NOISE: NOTE! Invalidating area 0,0,1009,679, offset was 0.000000,0.466667, is 0.000000,0.300000
NOISE: NOTE! Invalidating area 0,0,1009,680, offset was 0.000000,0.300000, is 0.000000,0.033333
NOISE: NOTE! Invalidating area 0,0,1009,681, offset was 0.000000,0.033333, is 0.000000,0.000000

But I downloaded that same build and ran it with Tp4 on my 10.5 machine locally (standalone Talos), and got no output like that whatsoever. Conditional breakpoints show the same thing on Mac and Windows, I can never get it to  invalidate anything there.

So I don't seem to be able to reproduce the regression.
Comment 98 Trif Andrei-Alin[:AlinT] 2011-08-19 07:12:48 PDT
So it seems that before this bug can be fixed, first the blockers must be removed.
so it remains resolved fixed?
Thanks.
Comment 99 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-08-20 04:56:20 PDT
the bugs blocked by this bug need to be retested. Most or all of them should be fixed.

Note You need to log in before you can comment on or make changes to this bug.