Last Comment Bug 539356 - (dlbi) Replace Invalidate() calls in reflow with display list analysis
(dlbi)
: Replace Invalidate() calls in reflow with display list analysis
Status: RESOLVED FIXED
[Snappy:P2]
: perf
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal with 26 votes (vote)
: mozilla18
Assigned To: Matt Woodrow (:mattwoodrow) (PTO until 27 June)
:
Mentors:
: 119311 (view as bug list)
Depends on: 795692 799018 802176 802180 812871 823377 864435 885348 1277484 542361 565245 725580 725886 731858 739490 739671 739743 741682 746421 746422 746890 747718 749055 758505 761884 766059 768766 769922 769935 769942 769944 769969 769975 769992 770000 770001 770002 770011 770021 770041 770052 770056 770058 770061 770062 770068 770069 770081 770083 770086 770096 770106 770144 770198 770201 770334 770349 770364 770381 770404 770424 770517 770524 770526 770572 770582 770617 770629 770639 770652 770686 770707 771407 775179 776690 776695 777260 777924 780385 780728 781362 782505 783126 786904 788044 789862 791699 CVE-2012-5829 792314 793501 794103 795322 795576 795577 795591 795611 795619 795626 795631 795646 795648 795653 795679 795694 795695 795722 795728 795734 795749 795796 795819 795826 795833 795899 796115 796159 796847 797059 797254 797255 797355 797950 798010 798244 798675 798761 798802 798964 799165 799506 799579 800198 800206 801365 801784 802100 802183 802321 802382 802385 802410 802440 805197 805487 805507 805696 806583 807934 808204 808409 808466 809178 809334 810275 810592 810876 815030 815602 820839 822115 825116 831780 833033 838580 846181 848078 855316 861042 900049 906405 991285
Blocks: 318474 352367 454254 546362 559396 595574 608648 626927 626963 691645 693105 702485 725437 747561 748220 748228 771822 775912 929484 119311 270264 511163 519339 585258 712883 722261 722888 723315 730509 748219 750417 774508 779269 786667 787397 790505 792413 793998 795318 798171 798182 807002 845437
  Show dependency treegraph
 
Reported: 2010-01-12 17:14 PST by Michael Ventnor
Modified: 2016-06-02 05:30 PDT (History)
90 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Patch 1 - Infrastructure (102.14 KB, patch)
2010-01-12 17:16 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 2 - Remove Invalidate() calls (66.67 KB, patch)
2010-01-12 17:17 PST, Michael Ventnor
no flags Details | Diff | Review
Remove GetUsedX Assertion (11.23 KB, patch)
2010-01-26 15:09 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 1 - Infrastructure (28.45 KB, patch)
2010-01-27 15:17 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 1 - Infrastructure (29.14 KB, patch)
2010-01-27 19:56 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 1 - Infrastructure (30.12 KB, patch)
2010-02-01 17:02 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 2 - Remove Invalidate() calls (72.30 KB, patch)
2010-02-01 17:02 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 3 - Manage text-shadow display items better (12.31 KB, patch)
2010-02-01 17:03 PST, Michael Ventnor
no flags Details | Diff | Review
Patch 4 - Optimise border invalidation (3.05 KB, patch)
2010-02-01 17:03 PST, Michael Ventnor
no flags Details | Diff | Review
Part 1 - Allow LayerManagers to have multiple user data objects (5.36 KB, patch)
2012-03-12 15:41 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 2 - Add new API to BasicLayers (7.21 KB, patch)
2012-03-12 15:42 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 3 - Make GetParentPresContext() succeed when the current PresContext has no frames (1.56 KB, patch)
2012-03-12 15:42 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
bzbarsky: review+
Details | Diff | Review
Part 4 - Disable subpixel AA with BasicLayers so that empty transactions always succeed (1.84 KB, patch)
2012-03-12 15:44 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 5 - Change SVG effects painting to use a LayerManager transaction (13.24 KB, patch)
2012-03-12 15:45 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 6 - Add compositing paint flashing to BasicLayers (4.14 KB, patch)
2012-03-12 15:45 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 7 - Store FrameLayerBuilder objects on the LayerManager instead of nsDisplayListBuilder (25.82 KB, patch)
2012-03-12 15:46 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event (50.11 KB, patch)
2012-03-12 15:47 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
bzbarsky: review-
joe: review-
Details | Diff | Review
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it (26.45 KB, patch)
2012-03-12 18:24 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9b - Add new frame invalidation API (7.83 KB, patch)
2012-03-12 18:25 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9c - Remove old invalidation code (165.55 KB, patch)
2012-03-12 18:26 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
bzbarsky: review-
Details | Diff | Review
Part 9d - Make SVG support the new invalidation model (13.17 KB, patch)
2012-03-12 18:26 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
jwatt: review-
Details | Diff | Review
Part 9e - FrameLayerBuilder changes for display list invalidation (81.39 KB, patch)
2012-03-12 18:27 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9f - Compute the invalid area of the layer tree and pass this to the widget (21.44 KB, patch)
2012-03-12 18:28 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9g - Modify MozAfterPaint code to work with the new invalidation model (6.42 KB, patch)
2012-03-12 18:29 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
patch to reinstate the foreignObject registering code removed in bug 734079 (7.87 KB, patch)
2012-03-28 17:59 PDT, Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13)
matt.woodrow: review+
Details | Diff | Review
Part 1 - Allow LayerManagers to have multiple user data objects v2 (4.36 KB, patch)
2012-04-25 20:21 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review-
Details | Diff | Review
Part 2 - Add new API to BasicLayers v2 (8.10 KB, patch)
2012-04-25 20:22 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review-
Details | Diff | Review
Part 5 - Change SVG effects painting to use a LayerManager transaction v2 (12.90 KB, patch)
2012-04-25 20:25 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 6 - Add compositing paint flashing to BasicLayers v2 (4.04 KB, patch)
2012-04-25 20:25 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 7 - Store FrameLayerBuilder objects on the LayerManager instead of nsDisplayListBuilder v2 (33.61 KB, patch)
2012-04-25 20:26 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event v2 (46.60 KB, patch)
2012-04-25 20:27 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v2 (30.36 KB, patch)
2012-04-25 20:28 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9b - Add new frame invalidation API v2 (7.80 KB, patch)
2012-04-25 20:29 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9c - Remove old invalidation code v2 (193.76 KB, patch)
2012-04-25 20:30 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
bzbarsky: review+
Details | Diff | Review
Part 9d - Make SVG support the new invalidation model (11.32 KB, patch)
2012-04-25 20:30 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
jwatt: review+
Details | Diff | Review
Part 9e - FrameLayerBuilder changes for display list invalidation v2 (79.63 KB, patch)
2012-04-25 20:31 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v2 (30.99 KB, patch)
2012-04-25 20:32 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v2 (6.17 KB, patch)
2012-04-25 20:36 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review-
Details | Diff | Review
Part 10: Test modifications required for DLBI (61.94 KB, patch)
2012-04-25 20:38 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 11: Reimplement empty transactions (10.44 KB, patch)
2012-04-25 20:39 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 12: Remove unnecessary LayerManagerLayerBuilder indirection (6.06 KB, patch)
2012-04-25 20:47 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 13 - Only repaint widgets that have had changes since the last paint (3.53 KB, patch)
2012-04-25 20:48 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review-
Details | Diff | Review
Part 14 - Handle multiple widget layer managers retaining data for the same frame. (20.55 KB, patch)
2012-04-25 20:49 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 15 - Add table invalidation test (5.19 KB, patch)
2012-04-30 21:27 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
bzbarsky: review+
Details | Diff | Review
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v3 (30.97 KB, patch)
2012-05-03 18:53 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9b - Add new frame invalidation API v3 (8.56 KB, patch)
2012-05-03 18:54 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v4 (32.18 KB, patch)
2012-05-13 14:54 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 9b - Add new frame invalidation API v4 (16.06 KB, patch)
2012-05-13 14:55 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9b - Add new frame invalidation API v5 (15.50 KB, patch)
2012-05-13 18:20 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9e - FrameLayerBuilder changes for display list invalidation v3 (83.42 KB, patch)
2012-05-13 21:07 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 9b - Add new frame invalidation API v6 (18.56 KB, patch)
2012-05-13 21:17 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9b - Add new frame invalidation API v7 (20.49 KB, patch)
2012-05-15 20:03 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 9e - FrameLayerBuilder changes for display list invalidation v4 (82.73 KB, patch)
2012-05-15 20:04 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
matt.woodrow: review+
Details | Diff | Review
Part 9f-1 - Refactor FrameLayerBuilder to not set Layer properties twice (52.89 KB, patch)
2012-05-15 20:05 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9f-2 - Compute the invalid area of the layer tree and pass this to the widget v3 (32.11 KB, patch)
2012-05-15 20:05 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v2 (22.94 KB, patch)
2012-05-15 20:06 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v4 (40.36 KB, patch)
2012-05-21 16:32 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v5 (44.80 KB, patch)
2012-05-21 19:49 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 2 - Add new API to BasicLayers v3 (8.10 KB, patch)
2012-05-21 20:10 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 1 - Allow LayerManagers to have multiple user data objects v3 (8.77 KB, patch)
2012-05-21 20:10 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v3 (21.71 KB, patch)
2012-05-21 20:11 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Optimize invalidation regions (WR) (1.02 KB, patch)
2012-05-22 00:42 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v6 (48.12 KB, patch)
2012-05-22 16:57 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v3 (8.91 KB, patch)
2012-05-22 16:58 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 11: Reimplement empty transactions v2 (11.59 KB, patch)
2012-05-22 16:58 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v4 (21.89 KB, patch)
2012-05-22 16:59 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v7 (47.81 KB, patch)
2012-05-22 17:49 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
A bit more smart region calculation (2.89 KB, patch)
2012-05-22 23:48 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Part 10: Test modifications required for DLBI v2 (69.30 KB, patch)
2012-06-06 20:50 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 16 - Revoke any pending ViewManager flushes when we do one (sometimes we get this called from Will Paint events) (1.34 KB, patch)
2012-06-06 20:51 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 17 - Don't paint widgets that an invisible or empty bounds (1.55 KB, patch)
2012-06-06 20:52 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 18 - Mark frames with only invalid children as an optimization to use when invalidating further frames (3.65 KB, patch)
2012-06-06 20:52 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 19 - Only repaint retained layers after the previous repainted has been drawn to the window (9.70 KB, patch)
2012-06-06 20:52 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 20 - Simplify regions to avoid excessive region calculation (9.07 KB, patch)
2012-06-06 20:53 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 21 - BasicLayers should always retain content (2.71 KB, patch)
2012-06-06 20:54 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Part 22 - Force a background color when running android reftests (3.74 KB, patch)
2012-06-06 20:55 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 19 - Only repaint retained layers after the previous repainted has been drawn to the window v2 (10.23 KB, patch)
2012-06-07 15:57 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 21 - BasicLayers should always retain content v2 (3.70 KB, patch)
2012-06-07 15:58 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 10: Test modifications required for DLBI v3 (69.25 KB, patch)
2012-06-07 16:03 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 23 - Fix MovePixels crash when our surface is in an error state. (1.44 KB, patch)
2012-06-10 21:39 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
jmuizelaar: review+
Details | Diff | Review
Part 24 - Don't paint android widgets if they aren't at the front, or compositing is disabled (2.70 KB, patch)
2012-06-27 19:11 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 25 - Invalidate display items that have a changed clip (13.73 KB, patch)
2012-06-27 19:12 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 26 - Send invalidations for hidden documents. (11.18 KB, patch)
2012-06-28 05:47 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 27 - Make nsImageFrame's overflow include the AltFeedback image if it will use one (5.00 KB, patch)
2012-06-28 05:48 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 28 - Cached nsDisplayBackground rasterizations with BasicLayers (19.15 KB, patch)
2012-06-29 03:16 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 29 - Catch OOM exception in java screenshot code (1.56 KB, patch)
2012-06-29 19:26 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
matt.woodrow: review+
Details | Diff | Review
Part 30 - Make ShadowContainerLayerD3D10 hold references to child layers. (8.71 KB, patch)
2012-07-27 19:09 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 31 - Call Azure canvas DidTransactionCallback even when painting from DrawWindow(). (2.83 KB, patch)
2012-07-27 19:10 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 32 - Change some SVG deferred animation tests to not require calling GetBaseValue. (2.21 KB, patch)
2012-07-27 19:11 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
matt.woodrow: review+
Details | Diff | Review
Part 33 - Change test-overflow/single-value reftest to use MozReftestInvalidate (2.71 KB, patch)
2012-07-27 19:11 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Make paint timing changes prefable (11.50 KB, patch)
2012-08-11 21:34 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Part 9e - FrameLayerBuilder changes for display list invalidation v5 (105.53 KB, patch)
2012-09-25 16:52 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Add an option for frames to invalid just a rect instead of the frame bounds (12.85 KB, patch)
2012-09-25 16:53 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Make the table code use rect invalidation to avoid over invalidation (47.26 KB, patch)
2012-09-25 16:54 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Correctly invalidate SVG observers (5.61 KB, patch)
2012-09-25 16:57 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Use rect invalidation in XUL tree's to avoid over invalidation (3.32 KB, patch)
2012-09-25 16:59 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Handled scrolled inactive layers trees correctly (3.46 KB, patch)
2012-09-25 17:00 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Avoid some causes of unnecessary painting (2.25 KB, patch)
2012-09-25 17:02 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Fix selectAtPoint.html test (1.20 KB, patch)
2012-09-26 17:54 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Review
Fix mActiveScrolledRootOffset to be offset to the reference frame (17.09 KB, patch)
2012-09-26 23:43 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Review
Part 9e - FrameLayerBuilder changes for display list invalidation v6 (110.49 KB, patch)
2012-09-27 02:31 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Add an option for frames to invalid just a rect instead of the frame bounds v2 (12.80 KB, patch)
2012-09-27 02:32 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Make the table code use rect invalidation to avoid over invalidation v2 (49.59 KB, patch)
2012-09-27 02:33 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
no flags Details | Diff | Review
Add HasRetainedDataFor (9.66 KB, patch)
2012-09-27 02:35 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Avoid some causes of unnecessary painting v2 (9.06 KB, patch)
2012-09-27 02:36 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Fix mActiveScrolledRootOffset to be offset to the reference frame (23.28 KB, patch)
2012-09-27 02:37 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
matt.woodrow: review+
Details | Diff | Review
Correctly invalidate SVG observers v2 (12.44 KB, patch)
2012-09-27 03:50 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Make the table code use rect invalidation to avoid over invalidation v3 (47.10 KB, patch)
2012-09-27 04:53 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Fix PruneDisplayListForExtraPage (4.10 KB, patch)
2012-09-27 05:28 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Review
Add an option for frames to invalid just a rect instead of the frame bounds v3 (12.95 KB, patch)
2012-09-27 05:37 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review
Disable test_text on android since it causes crashes (1.01 KB, patch)
2012-09-28 04:15 PDT, Matt Woodrow (:mattwoodrow) (PTO until 27 June)
roc: review+
Details | Diff | Review

Description Michael Ventnor 2010-01-12 17:14:48 PST
roc thinks that this will eventually be a DHTML win and assigned me to this. Whenever the geometry of a frame differs during reflow, currently we call Invalidate() on the entirety of the frame.

The aim of this bug is to remove all those calls and replace it with a comparison of display list items before/after each reflow. We can then attempt to match up the items and possibly minimise the area we need to invalidate.

By implementing a certain virtual function for each display list item type, we can calculate the minimal invalidation area case-by-case rather than doing the whole overflow rect.
Comment 1 Michael Ventnor 2010-01-12 17:16:21 PST
Created attachment 421369 [details] [diff] [review]
Patch 1 - Infrastructure

This provides the comparison algorithm and the ability to match up display list items. This patch alone doesn't really cause a behavioural change overall, unlike the next patch.
Comment 2 Michael Ventnor 2010-01-12 17:17:01 PST
Created attachment 421370 [details] [diff] [review]
Patch 2 - Remove Invalidate() calls

Look at all those -'s!
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-12 17:43:42 PST
+      for (PRInt32 n = 0; n < rem; lineFrame = lineFrame->GetNextSibling(), ++n) {
+        lineArea.UnionRect(lineArea, lineFrame->GetOverflowRect());
+      }

This is incorrect, lineFrame->GetOverflowRect() is relative to lineFrame->GetPosition().

+      line->SetCombinedArea(newFrame->GetOverflowRect());

Similar problem here.

The optimizations in nsIFrame::CheckInvalidateSizeChange will need to be reimplemented. Each of the cases in that function are trying to repaint (or not repaint) in some set of situations, and we need to ensure that we repaint (or don't repaint) in those situations.

nsDisplayText::ComputeInvalidationArea should be in the first patch.

       // Resize/move the row to its final size and position
       if (movedFrame) {
         rowFrame->InvalidateOverflowRect();
       }

You missed it!

I think we should make Invalidates assert if called during reflow (use the phase stuff in nsPresContext).
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-12 17:53:23 PST
Can you pull out the code that adds types to all the display items into a separate patch?

+  if (GetStyleText()->mTextShadow) {
+    nsresult rv = aBelowTextDecorations->AppendNewToTop(new (aBuilder)
+      nsDisplayTextShadow(this, decorations, aLine));
+    NS_ENSURE_SUCCESS(rv, rv);

Put the code that puts all shadows into one display item into its own patch.

-  nsDisplayWrapList* GetStoredList() { return &mStoredList; }
-#endif
-
+  virtual nsDisplayList* GetList() { return mStoredList.GetList(); }

We don't want to do this for nsDisplayTransform, because GetList should only return a list when the items in the list are in the same coordinate system as the other items. One option here is to make GetStoredList virtual and have all the nsDisplayListWrappers implement it ... so the items returned by GetStoredList are not guaranteed to be in the same coordinate system as other items.
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-12 18:13:59 PST
+      // what type it is, and its heirarchy in the display list.

hierarchy

+nsDisplayInvalidation::OnFrameDestroy(nsIFrame* aFrame)
+{
+  nsDisplayTableLinkEntry* entry = mBefore.GetEntry(aFrame);
+  if (!entry)
+    return;
+
+  for (PRUint32 i = 0; i < entry->Length(); i++) {
+    if (GetFrameForItem(entry->Item(i).Get()) == aFrame)

How could this "if" fail?

+  while (aEntry->Length() != 0) {
+    // If the frame is gone (and the frame pointer is rubbish), nothing to invalidate
+    if (aEntry->Item(0).IsFrameDestroyed()) {
+      aEntry->Items().RemoveElementAt(0);
+      continue;
+    }

Seems like it would be simpler and more efficient to just iterate through the array.

Don't we need to invalidate for destroyed frames?

Some name changes are required. IsEquivalent probably isn't a good name.

+    nsTArray<nsDisplayTableLink*> lhsEquivalentItems;
+    nsTArray<nsDisplayTableLink*> rhsEquivalentItems;

I don't think these are needed. Just iterate over the lists in the hashtable instead of building these temporary lists.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-12 18:19:18 PST
+    for (PRUint32 i = 0; i < lhsMatchedItems.Length(); i++) {
+      aEntry->Items().RemoveElement(*lhsMatchedItems[i]);
+    }

I don't think we'll need to do this, if we iterate over the lists in the hashtable directly.
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-12 18:29:53 PST
+nsRegion
+nsDisplayText::ComputeInvalidationArea(nsDisplayListBuilder* aBuilder,
+                                       nsDisplayItem* aOther,
+                                       const nsRect& aOtherVisibleRect,
+                                       const nsRect& aOtherBounds) {
+  // XXX We need to unconditionally invalidate, because it's possible for
+  // two different strings to have the same geometric width.
+  // Eventually, we can change this to analyse the string itself and get
+  // more performance wins.
+  // We also use the same antialiasing hack as in Paint()

We really need to do something here. Right now it looks like when any reflow happens, all visible text in the window will have to be repainted. We probably should be invalidating in nsTextFrameThebes::CharacterDataChanged.

The optimizations from CheckInvalidateSizeChange will need to be implemented in nsDisplayBackground::ComputeInvalidationArea and nsDisplayBorder::ComputeInvalidationArea.

+    nsDisplayItem::Type mParentType;
+    nsIFrame* mParentFrame;

I'm not sure why we need to store and check these.

Overall, this looks very promising!

Some suggested renames:
nsDisplayInvalidation -> nsDisplayListInvalidator
nsDisplayTableLink -> move it into nsDisplayListInvalidator, call it Item?
nsDisplayTableLinkEntry -> move it into nsDisplayListInvalidator, call it ItemEntry?
GetLastGoodUnderlyingFrame -> GetNearestUnderlyingFrame?
IsEquivalent -> MatchesFrameAndType

Let's have some design documentation in nsDisplayListInvalidator.h

+    friend PLDHashOperator CompareItemsCallback(nsDisplayTableLinkEntry *aEntry, void *aArg);
+    friend PLDHashOperator InvalidateLeftoversCallback(nsDisplayTableLinkEntry *aEntry, void *aArg);

You can avoid 'friend' by making these static functions in the class.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-12 18:31:41 PST
When shell->IsPaintingSuppressed is true, we shouldn't do any of the work to create display lists or compare them.
Comment 9 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-12 18:36:28 PST
You need to check all the different display types and figure out what parts of the frame geometry they depend on. For example, nsDisplayBackground background positions can depend on the content, border or padding box, and background clipping can depend on the content, border or padding box as well (I think), so if any of those boxes changes, we need to repaint. Right now you only repaint if the border-box changes. The border-box might stay the same while the content-box changes (e.g. if CSS 'width' increases and 'padding' decreases). You should write an invalidation reftest for this.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-12 19:45:55 PST
For the layout/tables changes, a number of the callers of nsTableFrame::InvalidateFrame were saving the original rect and overflow rect just so they could make those calls.  So there's more code to be removed there, possibly (in a followup if desired).
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-13 20:20:10 PST
A pretty large problem with this approach is that we trigger a lot of assertions about calling GetUsedBorder/Padding on a dirty frame. This is a real issue; we can't accurately determine the "before reflow" dislay list state if style changes have already destroyed our ability to know what the pre-reflow content-box and padding-box were.

Would it be possible to get rid of these assertions once and for all, by having style changes to margin, border and padding widths store the old used-margin/used-border/used-padding in frame properties, which are then removed (if possible) when reflow happens? There's obviously a performance hit for dynamic margin/border/padding changes --- but how common are they? In exchange, we'd get guilt-free painting of dirty frame trees, plus it would make this invalidation approach work.

If we don't want to do that, we could still fix this invalidation approach by ensuring changes to border/padding widths cause repainting, and having pre-reflow-display-list construction suppress the assertions.
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-01-13 20:23:58 PST
> but how common are they

Not very.
Comment 13 Michael Ventnor 2010-01-24 16:10:17 PST
(In reply to comment #7)
> nsDisplayTableLink -> move it into nsDisplayListInvalidator, call it Item?

I can't use friend nsDisplayListInvalidator::Item on nsDisplayItem, which I need to access the visible rect.
Comment 14 Michael Ventnor 2010-01-24 18:45:38 PST
D'oh, wait, it'd probably be part of the friend nsDisplayListInvalidator line, wouldn't it?
Comment 15 Michael Ventnor 2010-01-26 15:09:37 PST
Created attachment 423658 [details] [diff] [review]
Remove GetUsedX Assertion

One of the requirements for this project to work is to make sure we handle post-stylechange-pre-reflow properly.

I think I got it right...
Comment 16 Michael Ventnor 2010-01-26 15:15:45 PST
Comment on attachment 423658 [details] [diff] [review]
Remove GetUsedX Assertion

Roc wants this in a new bug.
Comment 17 Michael Ventnor 2010-01-27 15:17:46 PST
Created attachment 423868 [details] [diff] [review]
Patch 1 - Infrastructure

This is the main patch, the rest is just code churning which is likely to get r+ anyway. My plan is to upload them once this gets r+ so if there are review comments I can fix them easily.
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-27 17:34:15 PST
+  if (!invalFrame)
+    invalFrame = aLink.GetNearestUnderlyingFrame();

{} around the if body. There are lots of these.

+            mFrameData.mBounds = aItem->GetBounds(aBuilder);
+            if (itemFrame) {
+              mInvalidArea = GetReferenceRectForRect(aItem->mVisibleRect,
+                                                     itemFrame,
+                                                     aListFrameTree);
+              mFrameData.mPaddingRect = itemFrame->GetPaddingRect() -
+                                itemFrame->GetPosition() +
+                                aBuilder->ToReferenceFrame(itemFrame);

I think we should have a virtual method on nsDisplayItem, say nsDisplayItem::SaveGeometry(nsDisplayItemGeometry* aGeometry), which stashes the bounds in aGeometry, and for display items which are going to need more than that (e.g. borders and backgrounds), is overridden to also store the padding or content rect. This also means we get a slight optimization where we don't have to save the padding rect for most display items.

+  nsRect mPaddingRect;

So I'd make this something like "mExtraRect". This would be interpreted however the item wishes. For backgrounds this would be the background positioning area, see http://www.w3.org/TR/css3-background/#background-positioning-area

+    static void InvalidateRegion(nsDisplayListBuilder* aBuilder,
+                                 const Item& aLHS,
+                                 const Item& aRHS);
+
+    void InvalidateWholeItem(const Item& aLink);
+    nsresult FillHashtable(nsTHashtable<ItemEntry>* aTable,
+                           nsDisplayList* aList,
+                           PRBool aIsFirstPass,
+                           nsTArray<nsIFrame*>& aListFrameTree);
+

These methods need comments somewhere.
Comment 19 Michael Ventnor 2010-01-27 19:56:47 PST
Created attachment 423938 [details] [diff] [review]
Patch 1 - Infrastructure
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-27 20:39:26 PST
+            mFrameData.mBounds = aItem->GetBounds(aBuilder);

You don't need this anymore.

Some renaming:
GetReferenceRectForRect => GetReferenceRectToInvalidate
GetInvalidationFrame => GetFrameToInvalidate
SetPrevious => SetPreviousDisplayList
ItemEntry => ItemList

I think there is a problem when we destroy a frame that is the mNearestUnderlyingFrame for some item. We're going to crash. Please make up a testcase for this, it should be something like
<span style="opacity:0.5">Hello Kitty</span>
with a dynamic change where we start with a linebreak between the two words, and then a reflow puts the text on one line.

+        nsDisplayListInvalidator::Item& lhsItem = aEntry->GetItem(i);

This can be outside the j loop.

+          nsDisplayListInvalidator::InvalidateRegion(inval->mBuilder,
+                                                  lhsItem, rhsItem);

Fix indent.

+          j--;

Not needed, since you break out of the loop immediately anyway.

+    inval->mAfter.RawRemoveEntry(rhsEntry);
+  }
+
+  return PL_DHASH_REMOVE;

I don't think we need to bother removing the entries. We're going to kill off the entire hashtable(s) anyway once CompareAndInvalidate is done.

+    // If this is a text frame and the frame is dirty, we need to invalidate it now

Explain why.

I think the Item constructor should be moved out of the header file, taking GetFrameForItem and GetReferenceRectForRect. Also, why mark GetFrameForItem as inline?

+  nsTArray<nsIFrame*> listFrameTree;

might as well make this nsAutoTArray<nsIFrame*,8>. It won't usually be very deep.

+        nsDisplayItem* mItem;
+        nsIFrame* mNearestUnderlyingFrame;
+        nsRect mInvalidArea;
+        nsDisplayItemGeometry mFrameData;

These need comments.

I think it would make sense to move the ComputeVisibility calls out to nsPresShell, or else move more of the display list construction into nsDisplayListInvalidator (I prefer moving into nsPresShell). They belong with display list construction.

+    // Fills a hashtable with all items in aList. aListFrameTree
+    // records the ancestry of items in aList, since this is a
+    // recursive function.

Needs to describe aListFrameTree better, as described.
Comment 21 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-27 20:42:49 PST
(In reply to comment #20)
> I think there is a problem when we destroy a frame that is the
> mNearestUnderlyingFrame for some item. We're going to crash. Please make up a
> testcase for this, it should be something like
> <span style="opacity:0.5">Hello Kitty</span>
> with a dynamic change where we start with a linebreak between the two words,
> and then a reflow puts the text on one line.

The testcase might need to be something more like
<span style="opacity:0.5">Hello <b>Kitty</b></span>

so the frame for the <b>Kitty</b> isn't destroyed, but the second <span> frame that was its parent and its mNearestUnderlyingFrame *is* destroyed.
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-27 21:03:57 PST
GetInvalidationFrame can be replaced with GetFrameForItem (if we add an assertion to nsDisplayItem::GetUnderlyingFrame that only nsDisplayClip items can have null mFrames). Then we don't need mNearestUnderlyingFrame and the crash problem would go away.
Comment 23 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-01-28 03:23:34 PST
One pretty big problem is going to be table painting and nsDisplayTableBorderBackground, because that display item paints all the backgrounds for all the table parts (via nsTablePainter), and the table border, and all cell borders too if border-collapsing is in effect. So it depends on the geometry of everything in the table. To get reasonable invalidation here we probably need to rework table background painting so that individual nsDisplayBackground items are created (which would simplify other code), and possibly also do something with collapsed borders, although that may be less important.
Comment 24 Michael Ventnor 2010-02-01 17:02:04 PST
Created attachment 424689 [details] [diff] [review]
Patch 1 - Infrastructure

Uploading what I've done so far.
Comment 25 Michael Ventnor 2010-02-01 17:02:30 PST
Created attachment 424690 [details] [diff] [review]
Patch 2 - Remove Invalidate() calls
Comment 26 Michael Ventnor 2010-02-01 17:03:08 PST
Created attachment 424691 [details] [diff] [review]
Patch 3 - Manage text-shadow display items better
Comment 27 Michael Ventnor 2010-02-01 17:03:32 PST
Created attachment 424692 [details] [diff] [review]
Patch 4 - Optimise border invalidation
Comment 28 Michael Ventnor 2010-07-09 09:47:07 PDT
(In reply to comment #24)
> Created an attachment (id=424689) [details]
> Patch 1 - Infrastructure
> 
> Uploading what I've done so far.

Oh damn, this patch doesn't add types for each display list item type. I don't have the code with me right now so you may need to scavenge some of it from the obsolete patch :(
Comment 29 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-02 22:27:12 PDT
We might want to finish this to fix issues in bug 579323.

We can address comment #23 by setting a flag on a table when any of its components change geometry during reflow, and invalidating the entire table in that case.

Things have changed a bit since these patches were done. FrameLayerBuilder now builds a display list for the entire window every time we paint. Possibly we could harvest geometry information at that time and use that as our "before-reflow state", just rebuilding a display list after reflow and compare the geometry. That would save one display list construction per reflow.

FrameLayerBuilder is also storing a per-frame list of (display item key, layer) pairs representing the visible content of the window (except for content in inactive layers, but that could be fixed), as frame properties, plus a set of pointers to the frames that have such properties. This could be used as the basis for the saved display list state.
Comment 30 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-08 19:22:39 PDT
I have some new patches, but it's not done yet and I think it's going to be too risky for FF4. Post FF4 we can look at moving *all* invalidation to be display-list based.

My idea is that when invalidation is needed we simply ensure a paint event is queued in the refresh driver. When the refresh driver decides to paint, we build a new display list, update the layer tree to that display list, and during layer tree reconstruction, collect an invalid region based on the display list differences, and also update the layer tree contents by repainting ThebesLayer items as usual. Then and only then, force a synchronous paint of the native widget (unless we're a content process whose layer tree is being composited elsewhere). When painting a native widget, we'd just recomposite the layer tree and not do any flushing or Thebes drawing.

Doing invalidation during layer tree reconstruction has several advantages. It means we can always invalidate the right ThebesLayers. It will centralize the code to track display items and their changes. And it minimizes the amount of display list construction; we'll construct just one display list per paint (and the paint rate will be throttled of course).
Comment 31 Jonas Sicking (:sicking) PTO Until July 5th 2010-08-09 10:21:19 PDT
Is there anything we can do about the new-tab animation in the FF4 timeframe? It's looking pretty ugly right now, and sometimes even leaves turds.
Comment 32 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-08-09 16:11:00 PDT
Yes, there is a patch in bug 579323.
Comment 33 Jet Villegas (:jet) 2011-12-07 16:51:35 PST
Adding [Snappy] search string. DisplayList Analysis aims to fix a number of cases that lead to excessive/redundant invalidation, which should improve perceived performance.

BTW, do we need to reassign this bug?
Comment 34 [:jesup] on pto until 2016/7/5 Randell Jesup 2011-12-07 20:11:16 PST
[snappy] != perf
This is a perf bug.  So far as I know, this is not a user-response-time bug.
Comment 35 Dietrich Ayala (:dietrich) 2011-12-08 16:16:41 PST
(In reply to Randell Jesup [:jesup] from comment #34)
> [snappy] != perf
> This is a perf bug.  So far as I know, this is not a user-response-time bug.

Apparently this will affect how quickly the UI appears to respond for things like opening/closing/switching/moving tabs, since we currently redraw the whole tab-bar for those.
Comment 36 Dietrich Ayala (:dietrich) 2011-12-08 19:19:25 PST
Roc says Matt Woodrow will be taking this bug w/ a new approach in January.
Comment 37 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-02-09 15:00:58 PST
I've started working on this now.

I've got a prototype working locally, fixing performance/correctness bugs at the moment. I'll try to have something demoable within the next week or so.
Comment 38 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-13 11:13:38 PST
I tried to write a test for bug 725580 but was unsuccessful.  There's a manual testcase that shows the problem quite clearly with paint flashing enabled, which can be checked here.
Comment 39 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-02-19 21:34:15 PST
Making good progress on this, passes almost all mochitests.

A couple of issues that I'm struggling with:

MozAfterPaint - I've got this working to generate an invalidation region for the window, but I'm not sure how this is going to work for subdocuments, IFrames etc. Invalidation is all relative to a ThebesLayer, so finding the changed area for a component of this isn't easy. Relatedly, INVALIDATE_CROSS_DOC no longer exists, so I'm not sure how to only show rects that content should be allowed to see.

Flush_Layout (from getClientBoundingRect) - Reftests are using this to determine if anything needs to be repainted. Since we never do any invalidation detection until we paint (refresh driver tick), this doesn't actually invalidate anything. Reftests thus think that nothing will be invalidated.

Ideas anyone?
Comment 40 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-02-19 21:40:41 PST
Thinking about the second one, we could have invalidated frames (caused by style changes etc) setup a pending paint event - either with the entire window as the dirty region, or somehow fill this in later after a refresh driver tick.

I think we need this to implement empty transaction support, and it should work for style changes. Will reftests ever attempt to 'flush layout' with a change that would only be caught by display-list comparison?
Comment 41 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-02-20 02:31:30 PST
Alternatively we could teach the reftest harness to wait until a refresh driver tick before checking for paint events (do we have events that would work for this?) and hope there aren't other things relying on the existing behaviour.
Comment 42 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-20 12:14:23 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #39)
> MozAfterPaint - I've got this working to generate an invalidation region for
> the window, but I'm not sure how this is going to work for subdocuments,
> IFrames etc. Invalidation is all relative to a ThebesLayer, so finding the
> changed area for a component of this isn't easy. Relatedly,
> INVALIDATE_CROSS_DOC no longer exists, so I'm not sure how to only show
> rects that content should be allowed to see.

How about we change MozAfterPaint so that toplevel chrome documents see everything, toplevel content documents see repainting inside them (possible because all their content is always in an nsDisplayOwnLayer), and no other documents see anything.

We could attach identifying information to userdata for the container layer generated by an nsDisplayOwnLayer in nsSubDocumentFrame to indicate which prescontext it belongs to. Then you can walk up the layer tree and find which prescontext(s) need to be notified about the repainting.

This would be mostly compatible with our tests and other uses of MozAfterPaint, and hopefully be reasonably easy and efficient to implement. There might be one or two tests that use MozAfterPaint in an <iframe> that need to have some tests removed.
Comment 43 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-02-20 12:20:02 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #40)
> Thinking about the second one, we could have invalidated frames (caused by
> style changes etc) setup a pending paint event - either with the entire
> window as the dirty region, or somehow fill this in later after a refresh
> driver tick.
> 
> I think we need this to implement empty transaction support, and it should
> work for style changes. Will reftests ever attempt to 'flush layout' with a
> change that would only be caught by display-list comparison?

I'm not sure what this question is really about. Are you asking how to implement isMozAfterPaintPending?

One option for that would be to return true whenever there's a refresh driver tick pending. Then we'd need to require that every refresh driver tick fires a MozAfterPaint event (if there's a listener).

(BTW feel free to remove clearMozAfterPaintEvents if necessary, it's only used by one test.)
Comment 44 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-02-23 14:55:38 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #42) 
> How about we change MozAfterPaint so that toplevel chrome documents see
> everything, toplevel content documents see repainting inside them (possible
> because all their content is always in an nsDisplayOwnLayer), and no other
> documents see anything.
> 
> We could attach identifying information to userdata for the container layer
> generated by an nsDisplayOwnLayer in nsSubDocumentFrame to indicate which
> prescontext it belongs to. Then you can walk up the layer tree and find
> which prescontext(s) need to be notified about the repainting.
> 
> This would be mostly compatible with our tests and other uses of
> MozAfterPaint, and hopefully be reasonably easy and efficient to implement.
> There might be one or two tests that use MozAfterPaint in an <iframe> that
> need to have some tests removed.

This has the downside that toplevel content documents can see invalidations from subdocuments (iframes, svg), which they couldn't do previously without privileges.

I can't see any easy way to avoid this without forcing all documents into their own layers, and even then it won't be easy passing the regions back up through SVG's painting code.
Comment 45 Jet Villegas (:jet) 2012-02-23 15:46:48 PST
+cc: jwatt. He is moving the SVG scene graph to use the displayList as the render and hit-test tree.
Comment 46 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-02-23 17:20:17 PST
Mochitests also run within an iframe, so we don't get any events to them. I've got most of them passing when run standalone though.

Is there a way to run the mochitest in it's own tab/window?
Comment 47 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-02-24 02:38:31 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #44)
> it won't be easy passing the regions back up through SVG's painting code.

There's a helper nsSVGUtils::TransformFrameRectToOuterSVG that should help with that.
Comment 48 JP Rosevear [:jpr] 2012-03-12 10:09:15 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #37)
> I've started working on this now.
> 
> I've got a prototype working locally, fixing performance/correctness bugs at
> the moment. I'll try to have something demoable within the next week or so.

Hey Matt - is there a demo up of this somewhere, it would be nice to share with Facebook so they can verify the problems reported to dev-platform are indeed solved by this.
Comment 49 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 15:41:52 PDT
Created attachment 605171 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects
Comment 50 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 15:42:30 PDT
Created attachment 605172 [details] [diff] [review]
Part 2 - Add new API to BasicLayers
Comment 51 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 15:42:56 PDT
Created attachment 605174 [details] [diff] [review]
Part 3 - Make GetParentPresContext() succeed when the current PresContext has no frames
Comment 52 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 15:44:33 PDT
Created attachment 605175 [details] [diff] [review]
Part 4 - Disable subpixel AA with BasicLayers so that empty transactions always succeed
Comment 53 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 15:45:00 PDT
Created attachment 605176 [details] [diff] [review]
Part 5 - Change SVG effects painting to use a LayerManager transaction
Comment 54 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 15:45:21 PDT
Created attachment 605177 [details] [diff] [review]
Part 6 - Add compositing paint flashing to BasicLayers
Comment 55 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 15:46:01 PDT
Created attachment 605178 [details] [diff] [review]
Part 7 - Store FrameLayerBuilder objects on the LayerManager instead of nsDisplayListBuilder
Comment 56 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 15:47:14 PDT
Created attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

Not sure who to get to review most of these as roc is away. I've selected him for most of them anyway, happy for someone else to take them if they're keen :)

More patches coming soon!
Comment 57 Timothy Nikkel (:tnikkel) 2012-03-12 15:50:53 PDT
Comment on attachment 605174 [details] [diff] [review]
Part 3 - Make GetParentPresContext() succeed when the current PresContext has no frames

Why do you need this? You probably want to use views here.
Comment 58 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 15:56:48 PDT
(In reply to Timothy Nikkel (:tn) from comment #57)
> Comment on attachment 605174 [details] [diff] [review]
> Part 3 - Make GetParentPresContext() succeed when the current PresContext
> has no frames
> 
> Why do you need this? You probably want to use views here.

Finding the root (painting) refresh driver from any arbitrary frame. What is the advantage of using views here? Happy to change it if you want.
Comment 59 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 18:24:48 PDT
Created attachment 605261 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it

Part 9: All of The Things.

I've split this up into a few patches for review, they won't compile or run on their own.
Comment 60 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 18:25:34 PDT
Created attachment 605262 [details] [diff] [review]
Part 9b - Add new frame invalidation API
Comment 61 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 18:26:06 PDT
Created attachment 605264 [details] [diff] [review]
Part 9c - Remove old invalidation code
Comment 62 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 18:26:56 PDT
Created attachment 605265 [details] [diff] [review]
Part 9d - Make SVG support the new invalidation model
Comment 63 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 18:27:54 PDT
Created attachment 605266 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation
Comment 64 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 18:28:31 PDT
Created attachment 605267 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget
Comment 65 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-12 18:29:55 PDT
Created attachment 605268 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model
Comment 66 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-14 15:11:39 PDT
Comment on attachment 605264 [details] [diff] [review]
Part 9c - Remove old invalidation code

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

::: dom/base/nsDOMWindowUtils.cpp
@@ +238,5 @@
>  
>        PRIntervalTime iStart = PR_IntervalNow();
>  
>        for (PRUint32 i = 0; i < aCount; i++)
> +        rootFrame->InvalidateFrame();

You can remove "r".

@@ +351,5 @@
>          rootScrollFrame ? rootScrollFrame->GetContent() : nsnull;
>        nsRect rootDisplayport;
>        bool usingDisplayport = rootContent &&
>          nsLayoutUtils::GetDisplayPort(rootContent, &rootDisplayport);
> +      rootFrame->InvalidateFrame();

You can remove rootDisplayPort and usingDisplayPort and rootContent.

::: layout/base/nsCaret.cpp
@@ +644,5 @@
>        if (block) {
>          const nsStyleTextReset* style = block->GetStyleTextReset();
>          if (style->mTextOverflow.mLeft.mType != NS_STYLE_TEXT_OVERFLOW_CLIP ||
>              style->mTextOverflow.mRight.mType != NS_STYLE_TEXT_OVERFLOW_CLIP) {
> +          block->InvalidateFrame();

Do we need nsCaret::InvalidateTextOverflowBlock or can it just go away? I would have expected it to just go away.

@@ +1163,5 @@
>                                nsIFrame *aFrame)
>  {
>    NS_ASSERTION(aFrame, "Must have a frame to invalidate");
>    nsRect rect;
>    rect.UnionRect(aRect, aHook);

Get rid of rect, aRect and aHook parameters.

::: layout/base/nsImageLoader.cpp
@@ +278,5 @@
>  
>  #endif
>  
>    if (mFrame->GetStyleVisibility()->IsVisible()) {
> +    mFrame->InvalidateFrame();

Get rid of all the code that computes "bounds".

Stopping here ... I think generally there's a lot more code that can be removed here!
Comment 67 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-14 15:52:08 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #63)
> Created attachment 605266 [details] [diff] [review]
> Part 9e - FrameLayerBuilder changes for display list invalidation

Just a bit of context, since this is probably the most complex patch in the series. This is fundamentally two changes (that could probably be separated, but it's a non-trivial amount of work).

1) Make inactive layers retain their LayerManagers and Layers (but not buffers).

2) Compute invalid regions of ThebesLayers using display list comparison.


Will work on removing more of the old invalidation code.
Comment 68 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-14 17:30:26 PDT
Comment on attachment 605171 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects

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

Can you just modify LayerUserDataSet to use gfx/2d/UserData? The gfx/2d code doesn't follow our naming conventions so modifying LayerUserDataSet would isolate that.
Comment 69 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-14 17:32:39 PDT
Comment on attachment 605172 [details] [diff] [review]
Part 2 - Add new API to BasicLayers

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

Looks OK, but I don't understand why you're removing #ifdef DEBUG around mPhase. And you need to document the new IsWidgetLayerManager method.
Comment 70 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-14 17:34:39 PDT
Comment on attachment 605175 [details] [diff] [review]
Part 4 - Disable subpixel AA with BasicLayers so that empty transactions always succeed

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

I not confident we can get away with this :-(.

I've forgotten exactly what we discussed about this. What about adding support for component-alpha layers to BasicLayers?
Comment 71 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-15 16:05:18 PDT
 
> I not confident we can get away with this :-(.
> 
> I've forgotten exactly what we discussed about this. What about adding
> support for component-alpha layers to BasicLayers?

Really? :(

That's a pain. I guess I can look into implementing this, not sure how to do the alpha recovery part efficiently though.

 
> Looks OK, but I don't understand why you're removing #ifdef DEBUG around
> mPhase. And you need to document the new IsWidgetLayerManager method.

Making mPhase available in opt builds is required for the InTransaction() call. It's used later on in the patch queue.
Comment 72 Dão Gottwald [:dao] 2012-03-15 20:42:24 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #70)
> Comment on attachment 605175 [details] [diff] [review]
> Part 4 - Disable subpixel AA with BasicLayers so that empty transactions
> always succeed
> 
> Review of attachment 605175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I not confident we can get away with this :-(.

Does this do what I think it does, i.e. permanently disable subpixel AA when not using accelerated layers? That wouldn't be acceptable, imho.
Comment 73 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-15 21:04:56 PDT
Not all subpixel AA, just when we attempt to retain text in a layer with a transparent background.

Probably the worst case will be text on top of a fixed position background.

I'm looking into other solutions.
Comment 74 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-15 21:53:27 PDT
Comment on attachment 605174 [details] [diff] [review]
Part 3 - Make GetParentPresContext() succeed when the current PresContext has no frames

>+++ b/layout/base/nsPresContext.cpp
>+  // Not sure if this is always strictly the parent, but it works for GetRootPresContext
>+  // where the current pres context has no frames.

This seems to be correct at first glance, now that we always unify viewmanager trees when document trees are unified.  I think.

Which raises the question of why the first block (the one using GetCrossDocParentFrame) is needed at all.  I suspect it's not, but maybe double-check with roc?  Removing it in a followup is ok.

> nsPresContext::GetRootPresContext()
>-    if (!parent)
>+    if (!parent || parent == pc)

Hrm.  Why is this needed?  |parent == pc| shouldn't ever happen, as far as I can tell.
Comment 75 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-15 21:54:27 PDT
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

>+++ b/gfx/layers/basic/BasicLayers.cpp
>@@ -1619,16 +1619,23 @@ BasicLayerManager::EndTransactionInterna
>+    // TODO: We should really just set mTarget to null and make sure we can handle that further down the call chain

Please file this bug?

>+    nsRefPtr<gfxContext> ctx = new gfxContext(surf);
>+    mTarget = ctx;

You don't need the |ctx| temporary, right?

Someone who really knows how this layer stuff works should probably review the remaining changes to this file and the other gfx/layers changes.

>+++ b/layout/base/nsPresShell.cpp

The changes to this file should also be reviewed by someone who knows how views and widgets fit together nowadays (so :tn or roc, probably).

>+++ b/view/public/nsIViewManager.h
>+  virtual bool ProcessPendingUpdates()=0;

Document the return value?

>+++ b/view/src/nsViewManager.cpp

I'm not quite sure why we're calling PresShell:Paint() in ProcessPendingUpdatesForView...  At the very least, document clearly!

> nsViewManager::ProcessPendingUpdates()
>+  //if (mHasPendingUpdates) {

Why the comment?  If that line is no longer needed (why?) just remove it?
Comment 76 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-03-16 00:55:10 PDT
Comment on attachment 605264 [details] [diff] [review]
Part 9c - Remove old invalidation code

>+++ b/content/base/src/nsFrameLoader.cpp
>@@ -188,17 +187,16 @@ nsContentView::Update(const ViewConfig&

The long comment explaining why the InvalidateFrame call is the way it is can go away now that the InvalidaFrame call is going away, right?

That said, why is this InvalidateFrame call going away?

>@@ -1718,17 +1716,16 @@ nsFrameLoader::SetRenderMode(PRUint32 aR

Likewise, why can this call go away?  If it can, remove the comment too.

> +++ b/layout/base/nsCSSFrameConstructor.cpp
>@@ -8053,16 +8044,17 @@ nsCSSFrameConstructor::ContentStateChang
>+    primaryFrame->SchedulePaint();

Why is that call unconditional?  That seems pretty odd.  Why do we need that it at all?

>+++ b/layout/base/nsCaret.cpp
> void nsCaret::InvalidateRects(const nsRect &aRect, const nsRect &aHook,

The rects no longer seem to be needed here.... at least take out the UnionRect call?  But it seems like if we can invalidate just the rect, we should.

>+++ b/layout/base/nsImageLoader.cpp
>-    mFrame->Invalidate(bounds);
>+    mFrame->InvalidateFrame();

Do we not want to limit the invalidate to the actual damage area?  We certainly have bugs on making that actually work here!

>+++ b/layout/base/nsPresContext.cpp
>     // FrameLayerBuilder caches invalidation-related values that depend on the
>     // appunits-per-dev-pixel ratio, so ensure that all ThebesLayer drawing
>     // is completely flushed.
>-    FrameLayerBuilder::InvalidateThebesLayersInSubtree(rootFrame);
>+    rootFrame->InvalidateFrameSubtree();

Does the comment still makes sense?

>@@ -2046,22 +2046,19 @@ nsPresContext::FireDOMPaintEvent()
>-      notifyContent = false;
>+      notifyContent = true;

The changes to this function make no sense.  What are you trying to do here?

>+++ b/layout/generic/nsBlockReflowState.cpp
>@@ -821,17 +821,17 @@ nsBlockReflowState::FlowAndPlaceFloat(ns
>-    FrameLayerBuilder::InvalidateThebesLayersInSubtree(aFloat);
>+    aFloat->InvalidateFrameSubtree();

Why do we still need to invalidate here?

>+++ b/layout/generic/nsGfxScrollFrame.cpp
>@@ -1789,19 +1642,19 @@ void nsGfxScrollFrameInner::ScrollVisual
>-  mOuter->InvalidateWithFlags(invalidateRect, flags);
>+  mOuter->InvalidateFrame();

Again, this is invalidating a larger area than invalidateRect.  Do we still need to compute invalidateRect, assuming invalidating the larger area is correct?

>+++ b/layout/generic/nsImageFrame.cpp
>@@ -613,18 +613,17 @@ nsImageFrame::OnDataAvailable(imgIReques
>-  Invalidate(r);
>+  InvalidateFrame();

Similar here.

>@@ -693,17 +690,17 @@ nsImageFrame::FrameChanged(imgIRequest *
>-  Invalidate(r);
>+  InvalidateFrame();

And here.

>+++ b/layout/generic/nsImageMap.cpp
>@@ -989,17 +989,17 @@ nsImageMap::HandleEvent(nsIDOMEvent* aEv
>-            mImageFrame->Invalidate(dmgRect);
>+            mImageFrame->InvalidateFrame();

Then why do we need dmgRect?

>+++ b/layout/generic/nsTextFrameThebes.cpp
>@@ -4293,16 +4293,17 @@ nsTextFrame::CharacterDataChanged(Charac
>+    textFrame->InvalidateFrame();

Why this change?

>@@ -7732,17 +7733,18 @@ nsTextFrame::ReflowText(nsLineLayout& aL
>+  //XXX: Can we assume this is correct?
>+  //InvalidateFrame();

I suspect no, but I'd like to understand why this is commented out first....

>+++ b/layout/ipc/RenderFrameParent.cpp
>@@ -539,18 +539,16 @@ RenderFrameParent::ShadowLayersUpdated()

The comments here can all go away, right?

>+++ b/layout/svg/base/src/nsSVGForeignObjectFrame.cpp
>@@ -661,17 +643,17 @@ nsSVGForeignObjectFrame::InvalidateDirty
>-  aOuter->InvalidateWithFlags(rect, aFlags);
>+  aOuter->InvalidateFrame();

Can we stop computing rect, then?  Alternately, we should be using it here somehow...

>@@ -679,14 +661,13 @@ nsSVGForeignObjectFrame::FlushDirtyRegio
>-  InvalidateDirtyRect(outerSVGFrame, mSubDocDirtyRegion.GetBounds(),
>-                      aFlags | INVALIDATE_CROSS_DOC);
>+  InvalidateDirtyRect(outerSVGFrame, mSubDocDirtyRegion.GetBounds(), aFlags);

Why this change?

>+++ b/layout/svg/base/src/nsSVGOuterSVGFrame.cpp
>@@ -540,31 +540,23 @@ nsDisplaySVG::Paint(nsDisplayListBuilder
>+    ancestor->InvalidateFrameSubtree();

Why not InvalidateFrame()?

>+++ b/layout/tables/nsTableCellFrame.cpp
>@@ -896,17 +888,16 @@ NS_METHOD nsTableCellFrame::Reflow(nsPre

Can you stop saving origRect and/or origVisualOverflow here?

>+++ b/layout/tables/nsTableCellFrame.h
>+  virtual void InvalidateFrame()
...
>+    if (FrameHasBorderOrBackground(this)) {
>+      nsTableFrame *tableFrame = nsTableFrame::GetTableFrame(this);
>+      tableFrame->InvalidateFrame();

Did the old code end up doing this too, somehow?

>+++ b/layout/tables/nsTableColFrame.cpp
>+nsTableColFrame::InvalidateFrame()

Same question here.

>+++ b/layout/tables/nsTableColGroupFrame.cpp
>+nsTableColGroupFrame::InvalidateFrame()

And here.

>+++ b/layout/tables/nsTableFrame.cpp
>@@ -1427,17 +1424,17 @@ nsTableFrame::ProcessRowInserted(nscoord
>-          Invalidate(damageRect);
>+          InvalidateFrame();

Do you still need damageRect, then?  What about damageY?

> void nsTableFrame::PlaceChild(nsTableReflowState&  aReflowState,
>                               nsIFrame*            aKidFrame,
>                               nsHTMLReflowMetrics& aKidDesiredSize,
>                               const nsRect&        aOriginalKidRect,
>                               const nsRect&        aOriginalKidVisualOverflow)

Some of those arguments are no longer needed, right?

>+++ b/layout/tables/nsTableOuterFrame.cpp
>@@ -948,18 +948,16 @@ NS_METHOD nsTableOuterFrame::Reflow(nsPr
>   nsRect origInnerRect = InnerTableFrame()->GetRect();
>   nsRect origInnerVisualOverflow = InnerTableFrame()->GetVisualOverflowRect();

Do you still need those?

>   nsRect origCaptionRect;
>   nsRect origCaptionVisualOverflow;
>   bool captionFirstReflow;

And those?

>+++ b/layout/tables/nsTableRowFrame.cpp
>@@ -841,18 +838,16 @@ nsTableRowFrame::ReflowChildren(nsPresCo
>     nsRect kidRect = kidFrame->GetRect();
>     nsRect kidVisualOverflow = kidFrame->GetVisualOverflowRect();

And do you need those?

>@@ -1240,33 +1221,26 @@ nsTableRowFrame::CollapseRowIfNecessary(
>-          nsTableFrame::InvalidateFrame(cellFrame, oldCellRect,
>-                                        oldCellVisualOverflow,

oldCellRect and oldCellVisualOverflow are probably unused now...

>-  nsTableFrame::InvalidateFrame(this, oldRect, oldVisualOverflow, false);

Likewise for oldRect and oldVisualOverflow.

>+nsTableRowFrame::InvalidateFrame()

The usual question here.

>+++ b/layout/tables/nsTableRowGroupFrame.cpp
> nsTableRowGroupFrame::PlaceChild(nsPresContext*         aPresContext,

This has some extraneous arguments too, now.  Followup bug to remove them?

>@@ -426,24 +420,24 @@ nsTableRowGroupFrame::ReflowChildren(nsP
>-              Invalidate(dirtyRect);
>+              InvalidateFrame();

The changes here don't make sense.  Why do we need to InvalidateFrame() twice?  Why do we compute an unused dirtyRect?

>@@ -483,17 +477,17 @@ nsTableRowGroupFrame::ReflowChildren(nsP
>       // XXX We should change CalculateRowHeights() to return the bounding
>       // rect of what changed. Or whether anything moved or changed size...
>       nsRect  dirtyRect(0, 0, mRect.width, mRect.height);
>-      Invalidate(dirtyRect);
>+      InvalidateFrame();

Fix the comment?  And do we still need dirtyRect?

>+nsTableRowGroupFrame::InvalidateFrame()

Usual question.

>+++ b/layout/xul/base/src/nsBox.cpp
>@@ -637,20 +637,19 @@ nsIFrame::Redraw(nsBoxLayoutState& aStat
>+  InvalidateFrameSubtree();

>   // nsStackLayout, at least, expects us to repaint descendants even
>   // if a damage rect is provided

We're no longer providing a damage rect...  Should this comment precede the InvalidateFrameSubtree call?

>+++ b/layout/xul/base/src/nsSliderFrame.cpp
>@@ -711,17 +711,17 @@ nsSliderFrame::CurrentPositionChanged(ns
>+  //InvalidateFrame();

Why is this commented out?

>+++ b/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
>@@ -702,17 +702,17 @@ nsTreeBodyFrame::InvalidateColumn(nsITre
>+      nsIFrame::InvalidateFrame();

Why the nsIFrame qualifier?

>@@ -723,17 +723,17 @@ nsTreeBodyFrame::InvalidateRow(PRInt32 a
>+  nsIFrame::InvalidateFrame();

Here too.

>@@ -753,17 +753,17 @@ nsTreeBodyFrame::InvalidateCell(PRInt32 
>+    nsIFrame::InvalidateFrame();

And here.

>@@ -786,17 +786,17 @@ nsTreeBodyFrame::InvalidateRange(PRInt32
>-  nsIFrame::Invalidate(rangeRect);
>+  nsIFrame::InvalidateFrame();

And here.  Plus, do we need rangeRect here and various other places in this file?

>@@ -829,17 +829,17 @@ nsTreeBodyFrame::InvalidateColumnRange(P
>+  nsIFrame::InvalidateFrame();

And here.

>@@ -2573,26 +2573,26 @@ nsTreeBodyFrame::HandleEvent(nsPresConte
>+      //if (mMouseOverRow != -1)
>+        //InvalidateRow(mMouseOverRow);

Why?

>+      //if (mMouseOverRow != -1)
>+        //InvalidateRow(mMouseOverRow);

And here.

>+      //InvalidateRow(mMouseOverRow);

And here.

It's nice to see a bunch of that ugly code go away!
Comment 77 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-18 16:58:31 PDT
Comment on attachment 605177 [details] [diff] [review]
Part 6 - Add compositing paint flashing to BasicLayers

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +721,5 @@
> +      if (groupContext->GetFlags() & gfxContext::FLAG_DISABLE_SNAPPING) {
> +        gfxUtils::ClipToRegion(aContext, toDraw);
> +      } else {
> +        gfxUtils::ClipToRegionSnapped(aContext, toDraw);
> +      }

Why do we need to add this clipping? This adds overhead that we don't need if it's just for paint flashing.

@@ +1695,5 @@
> +
> +  if (!sPaintFlashingPrefCached) {
> +    sPaintFlashingPrefCached = true;
> +    mozilla::Preferences::AddBoolVarCache(&sPaintFlashingEnabled, 
> +                                          "nglayout.debug.composite_flashing");

Can we call this something else, say nglayout.debug.widget_update_flashing?

Will need to add the pref to all.js, with a comment that it only affects BasicLayers. And call this FlashWidgetUpdateArea --- we need to try hard to make sure this doesn't get confused with the other paint flashing.
Comment 78 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-18 17:21:44 PDT
Comment on attachment 605178 [details] [diff] [review]
Part 7 - Store FrameLayerBuilder objects on the LayerManager instead of nsDisplayListBuilder

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

::: layout/base/FrameLayerBuilder.cpp
@@ +94,5 @@
> +  MOZ_COUNT_DTOR(LayerManagerLayerBuilder);
> +  if (mDelete) {
> +    delete mLayerBuilder;
> +  }
> +};

stray semicolon

@@ +866,5 @@
>    // ThebesLayer to ensure that snapping exactly matches the ideal transform.
>    layer->SetAllowResidualTranslation(
>        mParameters.mInTransformedSubtree && !mParameters.mInActiveTransformedSubtree);
>  
> +  GetLayerBuilderForManager(mManager)->SaveLastPaintOffset(layer);

I think we should just give ContainerState a pointer to its FrameLayerBuilder in its constructor.

::: layout/base/FrameLayerBuilder.h
@@ +93,5 @@
> +
> +static inline FrameLayerBuilder *GetLayerBuilderForManager(layers::LayerManager* aManager)
> +{
> +  LayerManagerLayerBuilder *data = static_cast<LayerManagerLayerBuilder*>(aManager->GetUserData(&gLayerManagerLayerBuilder));
> +  return data->mLayerBuilder;

data ? data->mLayerBuilder : nsnull

::: layout/base/nsDisplayList.cpp
@@ +603,5 @@
>      BuildContainerLayerFor(aBuilder, layerManager, aForFrame, nsnull, *this,
>                             containerParameters, nsnull);
> +
> +  if (!root) {
> +    layerManager->SetUserData(&gLayerManagerLayerBuilder, nsnull);

RemoveUserData?

@@ +646,5 @@
>      FrameLayerBuilder::InvalidateAllLayers(layerManager);
>    }
>  
>    nsCSSRendering::DidPaint();
> +  layerManager->SetUserData(&gLayerManagerLayerBuilder, nsnull);

RemoveUserData?
Comment 79 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-18 17:22:37 PDT
Also I think we can give FrameLayerBuilder a pointer to its layer manager, now that there's only one, and remove a bunch of aManager parameters.
Comment 80 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-18 17:27:17 PDT
Maybe it would make sense to have the FrameLayerBuilder always be heap-allocated and owned by the layer manager user data? Seems a bit simpler.
Comment 81 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-20 11:47:55 PDT
Matt, can we remove the invalidation in nsFrameManager::RemoveFrame?
Comment 82 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-20 15:07:23 PDT
Yes we can, I've removed it locally.
Comment 83 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-03-22 04:09:47 PDT
Comment on attachment 605176 [details] [diff] [review]
Part 5 - Change SVG effects painting to use a LayerManager transaction

This looks okay to me, but I don't feel familiar enough with layers to review it. Can you get review from someone who does?

One suggestion regarding the patch - rename LayerState::LAYER_SVG to LayerState::LAYER_SVG_EFFECTS? Otherwise it sounds like it's a layer for SVG.
Comment 84 Timothy Nikkel (:tnikkel) 2012-03-26 16:14:31 PDT
Comment on attachment 605267 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget

>+  // TODO: nsSubDocumentFrame sometimes creates nsDisplayOwnLayer objects
>+  // with a child frame pointer. Do we want to catch all nsDisplayOwnLayer
>+  // objects here, or should we add a way to detect if these were created
>+  // by an nsSubDocumentFrame.
>+  //if (mFrame->GetType() == nsGkAtoms::subDocumentFrame) {
>+    ContainerLayerPresContext* pres = new ContainerLayerPresContext;
>+    pres->mPresContext = mFrame->PresContext();
>+    layer->SetUserData(&gContainerLayerPresContext, pres);
>+  //}
>   return layer.forget();

I think you only want to do this for sub document frames. But subdocument frames create nsDisplayZoom items when the zoom is different on the child document, so I think you'll want to handle those as well.
Comment 85 Timothy Nikkel (:tnikkel) 2012-03-26 17:35:51 PDT
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

>-void nsViewManager::ProcessPendingUpdatesForView(nsView* aView,
>+bool nsViewManager::ProcessPendingUpdatesForView(nsView* aView,

It looks like the only way this can return true is if it's passed a null aView or somehow comes across one when recursing through child views. Does this need to return a bool?
Comment 86 Timothy Nikkel (:tnikkel) 2012-03-26 18:08:26 PDT
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

> PresShell::Paint(nsIView*           aViewToPaint,
>-    if (!(frame->GetStateBits() & NS_FRAME_UPDATE_LAYER_TREE)) {
>-      if (layerManager->EndEmptyTransaction()) {
>-        frame->UpdatePaintCountForPaintedPresShells();
>-        presContext->NotifyDidPaintForSubtree();
>-        return;
>-      }
>+    if (aType == PaintType_Composite) {
>+      DebugOnly<bool> result = layerManager->EndEmptyTransaction();
>+      NS_ASSERTION(result, "Must complete empty transaction when compositing!");
>+      return;
>     }
> 
>     frame->RemoveStateBits(NS_FRAME_UPDATE_LAYER_TREE);

So NS_FRAME_UPDATE_LAYER_TREE will be obsolete with these patches? It doesn't look like it gets fully removed by sum result of all the patches here so far.
Comment 87 Timothy Nikkel (:tnikkel) 2012-03-26 18:10:36 PDT
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

Joe, someone familiar with the accelerated layer backends should review the changes in that code. Could you find someone appropriate?
Comment 88 Joe Drew (not getting mail) 2012-03-27 12:17:56 PDT
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

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

Bas, can you look at the D3D{9,10} changes here? I've reviewed BasicLayers and OpenGL layers.

::: gfx/layers/basic/BasicLayers.cpp
@@ +1662,5 @@
>  
> +    if (aFlags & END_NO_COMPOSITE) {
> +      if (IsRetained()) {
> +        mTarget->Clip(gfxRect(0, 0, 0, 0));
> +        PaintLayer(mTarget, mRoot, aCallback, aCallbackData, nsnull);

This will need a comment explaining why we paint the layer clipped to nothing.

::: gfx/layers/opengl/LayerManagerOGL.cpp
@@ +444,5 @@
>      mRoot->ComputeEffectiveTransforms(gfx3DMatrix());
>  
>      mThebesLayerCallback = aCallback;
>      mThebesLayerCallbackData = aCallbackData;
> +    SetCompositingDisabled(aFlags & END_NO_COMPOSITE);

Why not just return, instead of going through the rest of the mechanics of rendering?

If you do just return, you don't need the weirdness in ContainerLayerOGL.

::: gfx/layers/opengl/ThebesLayerOGL.cpp
@@ +510,5 @@
>        mTexImage = nsnull;
>        mTexImageOnWhite = nsnull;
>        mBufferRect.SetRect(0, 0, 0, 0);
>        mBufferRotation.MoveTo(0, 0);
> +      printf("Clearing EVERYTHING\n");

debugging printf
Comment 89 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-03-27 12:43:44 PDT
(In reply to Joe Drew (:JOEDREW!) from comment #88)

> >      mRoot->ComputeEffectiveTransforms(gfx3DMatrix());
> >  
> >      mThebesLayerCallback = aCallback;
> >      mThebesLayerCallbackData = aCallbackData;
> > +    SetCompositingDisabled(aFlags & END_NO_COMPOSITE);
> 
> Why not just return, instead of going through the rest of the mechanics of
> rendering?
> 
> If you do just return, you don't need the weirdness in ContainerLayerOGL.
> 

Wouldn't this skip ThebesLayer drawing? We still need to make sure that all ThebesLayers have fully valid contents.
Comment 90 Joe Drew (not getting mail) 2012-03-27 13:01:03 PDT
Hm, good point. Ugh. It'd be nice if compositing and validation were explicitly separated.
Comment 91 Timothy Nikkel (:tnikkel) 2012-03-27 15:29:56 PDT
Comment on attachment 605179 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event

>diff --git a/layout/base/nsDisplayList.h b/layout/base/nsDisplayList.h
>--- a/layout/base/nsDisplayList.h
>+++ b/layout/base/nsDisplayList.h
>   enum {
>     PAINT_DEFAULT = 0,
>     PAINT_USE_WIDGET_LAYERS = 0x01,
>     PAINT_FLUSH_LAYERS = 0x02,
>-    PAINT_EXISTING_TRANSACTION = 0x04
>+    PAINT_EXISTING_TRANSACTION = 0x04,
>+    PAINT_NO_COMPOSITE = 0x08,
>+    PAINT_USE_CACHED_LAYER_MANAGER = 0x10

PAINT_USE_CACHED_LAYER_MANAGER doesn't seem to be used in this patch or any other patch in this bug.
Comment 92 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-03-28 17:57:37 PDT
Comment on attachment 605265 [details] [diff] [review]
Part 9d - Make SVG support the new invalidation model

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

If we have to use InvalidatedInSubtree() to crawl the descendants of all nsSVGForeignObjectFrame frames looking to see if they contain a dirty descendant then I guess that's what we have to do. That part will only have a perf impact on SVGs that contain foreignObjects after all. We shouldn't slow down _all_ SVGs by crawling the entire SVG frame tree looking for the nsSVGForeignObjectFrame frames though. Instead we can use the foreignObject registering code that I removed in bug 734079. I'll attach a patch for you that reinstates the parts of that code that you'll need is a minute.
Comment 93 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-03-28 17:59:59 PDT
Created attachment 610380 [details] [diff] [review]
patch to reinstate the foreignObject registering code removed in bug 734079
Comment 94 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-03-28 18:28:39 PDT
When SVG is converted to display lists, it can participate fully in this framework so we won't need to do that crawling.
Comment 95 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-03-28 21:08:15 PDT
Indeed, I've put comments to that affect in the patch.
Comment 96 CruNcher 2012-04-20 14:13:53 PDT
Matt your try build shows significant less frame drops here :)
https://bugzilla.mozilla.org/show_bug.cgi?id=702485
Comment 97 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:21:52 PDT
Created attachment 618520 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v2
Comment 98 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:22:36 PDT
Created attachment 618521 [details] [diff] [review]
Part 2 - Add new API to BasicLayers v2
Comment 99 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:25:04 PDT
Created attachment 618522 [details] [diff] [review]
Part 5 - Change SVG effects painting to use a LayerManager transaction v2
Comment 100 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:25:58 PDT
Created attachment 618523 [details] [diff] [review]
Part 6 - Add compositing paint flashing to BasicLayers v2
Comment 101 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:26:57 PDT
Created attachment 618524 [details] [diff] [review]
Part 7 - Store FrameLayerBuilder objects on the LayerManager instead of nsDisplayListBuilder v2
Comment 102 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:27:40 PDT
Created attachment 618525 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event v2
Comment 103 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:28:34 PDT
Created attachment 618526 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v2
Comment 104 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:29:28 PDT
Created attachment 618527 [details] [diff] [review]
Part 9b - Add new frame invalidation API v2
Comment 105 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:30:03 PDT
Created attachment 618528 [details] [diff] [review]
Part 9c - Remove old invalidation code v2
Comment 106 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:30:50 PDT
Created attachment 618529 [details] [diff] [review]
Part 9d - Make SVG support the new invalidation model
Comment 107 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:31:26 PDT
Created attachment 618530 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v2
Comment 108 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:32:34 PDT
Created attachment 618531 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v2
Comment 109 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:36:23 PDT
Created attachment 618534 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v2
Comment 110 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:38:44 PDT
Created attachment 618536 [details] [diff] [review]
Part 10: Test modifications required for DLBI

Not sure who should review something like this.

I also regret not writing down all my conclusions that lead to these changes. Can try figure them out again if anyone has specific questions.
Comment 111 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:39:34 PDT
Created attachment 618537 [details] [diff] [review]
Part 11: Reimplement empty transactions
Comment 112 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:47:02 PDT
Created attachment 618541 [details] [diff] [review]
Part 12: Remove unnecessary LayerManagerLayerBuilder indirection

This was required in part 7, since inactive layer transactions didn't use their own FrameLayerBuilders.

After part 9, this is no longer the case, so this cleans up the unnecessary code.
Comment 113 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:48:14 PDT
Created attachment 618542 [details] [diff] [review]
Part 13 - Only repaint widgets that have had changes since the last paint

Reduce time spent in building display lists by only repainting widgets that have had changes made.
Comment 114 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:49:29 PDT
Created attachment 618543 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame.

In some cases (ie OSX titlebar painting) we can have multiple retained layer managers storing the same frame.

This updates the frame data storage code to be able to handle this.
Comment 115 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-25 20:50:48 PDT
The currently attached patch queue passes all tests except for Native Android crashes caused by bug 746890.

Patch queue updated here: http://hg.mozilla.org/users/mwoodrow_mozilla.com/dlbi
Comment 116 Timothy Nikkel (:tnikkel) 2012-04-25 22:00:01 PDT
Comment on attachment 618536 [details] [diff] [review]
Part 10: Test modifications required for DLBI

Can you just make the two or three tests that fail in test_panel.xul todo instead of not running the whole test at all on linux?
Comment 117 Timothy Nikkel (:tnikkel) 2012-04-25 22:02:03 PDT
Comment on attachment 618530 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v2

A paragraph or two outlining the changes this patch makes, or the new setup it creates, would be helpful.
Comment 118 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-29 18:32:34 PDT
(In reply to Timothy Nikkel (:tn) from comment #117)
> Comment on attachment 618530 [details] [diff] [review]
> Part 9e - FrameLayerBuilder changes for display list invalidation v2
> 
> A paragraph or two outlining the changes this patch makes, or the new setup
> it creates, would be helpful.

Sure.

This patch removes the remainder of the old invalidation code (ThebesLayerInvalidRegionProperty, SetHasContainerLayer etc).

It also replaces this with the new invalidation code, using display list analysis. Most of this is within InvalidateForLayerChange(), but it also adds code to AddLayerDisplayItem to store the geometries for the current display list, and mark items that were reused from the last paint (mUsed).

It also adds ProcessRemovedDisplayItems to find display items from the previous paint (ones without the mUsed bit set), and invalidate them. This is because the InvalidateForLayerChange code never gets called for these.

It also modifies the RemoveFrameFromLayerManager frame property destructor to invalidate the areas once occupied by display items belonging to that frame.

The other main part of this patch is retaining display item information, layers and layer managers (but not layer buffers) for inactive layers. This is required to be able to do display list comparison for changes within inactive layers. For this we create a new FrameLayerBuilder when executing a inactive layer transaction (and call DidBeginRetainedLayerTransaction), and cache the layer manager in DisplayItemData.

This adds some complexity, since frames are no longer associated with a single layer manager, which the existing frame property setup assumed. We now store the LayerManagerData for the outer-most LayerManager, and you can walk down through inactive layer managers to find the others from there. See the comment above RemoveFrameFromLayerManager for more on how this affects things.


(In reply to Timothy Nikkel (:tn) from comment #116)
> Comment on attachment 618536 [details] [diff] [review]
> Part 10: Test modifications required for DLBI
> 
> Can you just make the two or three tests that fail in test_panel.xul todo
> instead of not running the whole test at all on linux?

Sure, will do.
Comment 119 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-30 21:27:23 PDT
Created attachment 619830 [details] [diff] [review]
Part 15 - Add table invalidation test

More or less the same framework as layout/base/tests/test_bug450930.xhtml. Should be easy to add more tests to this as needed.

Hope this is testing the right thing, works first time with DLBI though :)
Comment 120 Jonathan Watt [:jwatt] (Away Jun. 27 - Jul. 13) 2012-05-01 02:57:20 PDT
Comment on attachment 618529 [details] [diff] [review]
Part 9d - Make SVG support the new invalidation model

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

r=jwatt

::: layout/svg/base/src/nsSVGForeignObjectFrame.cpp
@@ +595,5 @@
> +nsSVGForeignObjectFrame::GetInvalidRegion()
> +{
> +  nsIFrame* kid = GetFirstPrincipalChild();
> +  if (kid->InvalidatedInSubtree()) {
> +    gfxRect r(mRect.x, mRect.y, mRect.width, mRect.height);

This will work for now, but only because there is a bug in nsSVGForeignObjectFrame::UpdateBounds() where it uses gfxRect(0.0, 0.0, w, h) when setting mRect instead of using gfxRect(x, y, w, h). I've fixed that in a patch elsewhere, so to avoid unexpected errors in the event that I land first I'd suggest that you should make that change to this patch too (and move the "// GetCanvasTM includes the x,y translation" comment down to just above the line that sets mCoveredRegion). That done, you can change the line above in this patch here to |gfxRect r(0.0, 0.0, mRect.width, mRect.height)| (and probably add a "// GetCanvasTM includes the x,y translation" there too).
Comment 121 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-01 15:19:35 PDT
Comment on attachment 618520 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v2

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

::: gfx/layers/Layers.h
@@ +200,4 @@
>  
>    void Set(void* aKey, LayerUserData* aValue)
>    {
> +    mUserData.Add((gfx::UserDataKey *)aKey, aValue, LayerManagerUserDataDestroy);

Make these static_casts.

Better still, instead of casting these, why not actually use gfx::UserData in the API and in the callers?
Comment 122 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-01 15:25:59 PDT
Comment on attachment 618521 [details] [diff] [review]
Part 2 - Add new API to BasicLayers v2

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

::: gfx/layers/basic/BasicLayers.cpp
@@ +1436,5 @@
>    MOZ_LAYERS_LOG(("[----- BeginTransaction"));
>    Log();
>  #endif
>  
> +  //NS_ASSERTION(!InTransaction(), "Nested transactions not allowed");

Why is this commented out?
Comment 123 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-01 15:26:36 PDT
Comment on attachment 605175 [details] [diff] [review]
Part 4 - Disable subpixel AA with BasicLayers so that empty transactions always succeed

I believe this is obsolete now
Comment 124 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-03 17:03:08 PDT
Comment on attachment 618526 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v2

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

::: layout/base/nsDisplayList.h
@@ +700,5 @@
> +    nsDisplayItemGenericGeometry* geometry = new nsDisplayItemGenericGeometry();
> +    bool snap;
> +    geometry->mBounds = GetBounds(aBuilder, &snap);
> +    geometry->mBorderRect = GetBorderRect();
> +    return geometry;

I'd prefer it if GetXYZ() functions didn't allocate. Call this AllocateGeometry?

@@ +704,5 @@
> +    return geometry;
> +  }
> +
> +  virtual nsRegion ComputeInvalidationRegion(nsDisplayListBuilder* aBuilder,
> +                                             const nsDisplayItemGeometry* aGeometry)

Both this and GetGeometry need some major documentation.

::: layout/base/nsDisplayListInvalidation.h
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + *
> + * vim: set ts=2 sw=2 et tw=78:
> + * ***** BEGIN LICENSE BLOCK *****
> + * Version: MPL 1.1/GPL 2.0/LGPL 2.1

Update this (and other new files) to use the new license block.

@@ +43,5 @@
> +
> +class nsDisplayItemGeometry
> +{
> +  NS_INLINE_DECL_REFCOUNTING(nsDisplayItemGeometry)
> +public:

This entire file is extremely comment-deficient.
Comment 125 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-03 17:05:39 PDT
Comment on attachment 618527 [details] [diff] [review]
Part 9b - Add new frame invalidation API v2

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

::: layout/generic/nsIFrame.h
@@ +2225,5 @@
>    void InvalidateOverflowRect();
>  
>    /**
> +   * Marks all display items created by this frame as needing a repaint,
> +   * and schedules a view manager flush to trigger painting

Is this leaf display items or all display items (including containers such as nsDisplayTransform etc)?

@@ +2232,5 @@
> +  
> +  /**
> +   * Invalidate the entire frame subtree for this frame.
> +   */
> +  void InvalidateFrameSubtree();

What does this do exactly? Call InvalidateFrame on all descendant frames?

@@ +2237,5 @@
> +  
> +  /**
> +   * Checks if a frame has been marked as invalid.
> +   */
> +  bool Invalidated();

IsInvalid()? And what exactly does this return?

@@ +2243,5 @@
> +  /**
> +   * Check if this frame within the frame subtree (including this frame) 
> +   * are marked as invalid.
> +   */
> +  bool InvalidatedInSubtree();

I don't understand this comment at all

@@ +2248,5 @@
> +
> +  /**
> +   * Schedule a view manager flush on the next refresh driver tick.
> +   */
> +  void SchedulePaint();

Explain in more detail what this does.

@@ +2250,5 @@
> +   * Schedule a view manager flush on the next refresh driver tick.
> +   */
> +  void SchedulePaint();
> +
> +  Layer* InvalidateLayer(const nsRect& aDamageRect, PRUint32 aDisplayItemKey);

This needs a big comment too.
Comment 126 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-03 17:06:40 PDT
I think it would make sense to update those two API patches with better comments before I proceed on to the rest of the reviews.
Comment 127 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-03 17:22:03 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #124)
> 
> ::: layout/base/nsDisplayListInvalidation.h
> @@ +1,5 @@
> > +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> > + *
> > + * vim: set ts=2 sw=2 et tw=78:
> > + * ***** BEGIN LICENSE BLOCK *****
> > + * Version: MPL 1.1/GPL 2.0/LGPL 2.1
> 
> Update this (and other new files) to use the new license block.

What is the new license block? - Or which file can I copy one from.

> ::: layout/generic/nsIFrame.h
> @@ +2225,5 @@
> >    void InvalidateOverflowRect();
> >  
> >    /**
> > +   * Marks all display items created by this frame as needing a repaint,
> > +   * and schedules a view manager flush to trigger painting
> 
> Is this leaf display items or all display items (including containers such
> as nsDisplayTransform etc)?

All display items, including containers.

> 
> @@ +2232,5 @@
> > +  
> > +  /**
> > +   * Invalidate the entire frame subtree for this frame.
> > +   */
> > +  void InvalidateFrameSubtree();
> 
> What does this do exactly? Call InvalidateFrame on all descendant frames?

Yep.

> 
> @@ +2237,5 @@
> > +  
> > +  /**
> > +   * Checks if a frame has been marked as invalid.
> > +   */
> > +  bool Invalidated();
> 
> IsInvalid()? And what exactly does this return?


Returns if Invalidate() has been called on this frame since the last time we painted it.

> 
> @@ +2243,5 @@
> > +  /**
> > +   * Check if this frame within the frame subtree (including this frame) 
> > +   * are marked as invalid.
> > +   */
> > +  bool InvalidatedInSubtree();
> 
> I don't understand this comment at all

Checks if *any* frame within the frame subtree....

> 
> @@ +2248,5 @@
> > +
> > +  /**
> > +   * Schedule a view manager flush on the next refresh driver tick.
> > +   */
> > +  void SchedulePaint();
> 
> Explain in more detail what this does.

Any suggestions? Not sure what else to say about this.

Will update all the comments/naming asap.
Comment 128 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-03 17:27:52 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #127)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #124)
> > Update this (and other new files) to use the new license block.
> 
> What is the new license block? - Or which file can I copy one from.

http://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp

> > Is this leaf display items or all display items (including containers such
> > as nsDisplayTransform etc)?
> 
> All display items, including containers.

Please say so.

> > What does this do exactly? Call InvalidateFrame on all descendant frames?
> 
> Yep.

Please say so.

> > IsInvalid()? And what exactly does this return?
> 
> Returns if Invalidate() has been called on this frame since the last time we
> painted it.

Including via InvalidateFrame() presumably?

> > > +  bool InvalidatedInSubtree();
> > 
> > I don't understand this comment at all
> 
> Checks if *any* frame within the frame subtree....

OK, please fix that.

Would it make sense perhaps to have a frame state bit to indicate "has invalid descendant"? like we do for reflow-dirty?

> > > +  /**
> > > +   * Schedule a view manager flush on the next refresh driver tick.
> > > +   */
> > > +  void SchedulePaint();
> > 
> > Explain in more detail what this does.
> 
> Any suggestions? Not sure what else to say about this.

Say that the view manager flush will update the layer tree and repaint any invalid areas in the layer tree (and that a layer tree composite operation will happen shortly after that to get the results onto the screen).

I presume this also ensures there will be a next refresh driver tick, so you'd better say that as well.
Comment 129 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-03 17:33:10 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #128)
> Including via InvalidateFrame() presumably?

Yes, that's what I meant.

> 
> Would it make sense perhaps to have a frame state bit to indicate "has
> invalid descendant"? like we do for reflow-dirty?

I'm not sure that would be a useful piece of information, unless we want to walk up the frame tree when determining if a display item needs to be repainted.

This hasn't shown up in profiles as being a problem, we could look into faster ways of doing this if it does though.
Comment 130 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-03 18:53:55 PDT
Created attachment 620926 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v3
Comment 131 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-03 18:54:32 PDT
Created attachment 620927 [details] [diff] [review]
Part 9b - Add new frame invalidation API v3
Comment 132 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-06 21:49:30 PDT
Comment on attachment 620926 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v3

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

Could we avoid giving nsDisplayItemGeometry virtual methods by putting those virtual methods on the associated nsDisplayItem, and making nsDisplayItemGeometry essentially a private data blob for the display item? I think that would be a little bit simpler, avoid exposing nsDisplayItemGeometry details, and be more efficient (no vtable, hopefully no refcount).

Also, it might be a good idea to allocate nsDisplayItemGeometry objects from the presshell arena.

::: layout/base/nsDisplayList.cpp
@@ +1184,5 @@
>        *aForceTransparentSurface = disp->mAppearance == NS_THEME_WIN_BORDERLESS_GLASS ||
>                                    disp->mAppearance == NS_THEME_WIN_GLASS;
>      }
>      if (mThemeTransparency == nsITheme::eOpaque) {
> +      result = nsRect(nsPoint(0,0), mFrame->GetSize()) + ToReferenceFrame();

nsRect(ToReferenceFrame(), mFrame->GetSize())

::: layout/base/nsDisplayList.h
@@ +702,5 @@
> +   * and determine if repainting needs to happen.
> +   *
> +   * Subclasses wishing to store more information need to override both this
> +   * and ComputeInvalidationRegion, as well as implementing an nsDisplayItemGeometry
> +   * subclass.

Say something about what the default implementation does here.

@@ +724,5 @@
> +   *
> +   * @param aGeometry The geometry of the matching display item from the 
> +   * previous paint.
> +   * @return The invalidation area required to reflect the changes between 
> +   * the previous and current display items.

Say something about the default implementation here.

@@ +727,5 @@
> +   * @return The invalidation area required to reflect the changes between 
> +   * the previous and current display items.
> +   */
> +  virtual nsRegion ComputeInvalidationRegion(nsDisplayListBuilder* aBuilder,
> +                                             const nsDisplayItemGeometry* aGeometry)

Consider having this take an nsRegion* out parameter, and be defined to add the invalid area to that region. That is a bit more efficient than returning a new region.

@@ +1666,5 @@
>    NS_DISPLAY_DECL_NAME("Background", TYPE_BACKGROUND)
>    // Returns the value of GetUnderlyingFrame()->IsThemed(), but cached
>    bool IsThemed() { return mIsThemed; }
>  
> +  bool RenderingMightDependOnFrameSize();

Document this

@@ +1771,5 @@
> +    if (!geometry->mPaddingRect.IsEqualInterior(GetPaddingRect())) {
> +      // nsDisplayBoxShadowInner is based around the padding rect, but it can
> +      // touch pixels outside of this. We should invalidate the entire bounds.
> +      bool snap;
> +      invalid = invalid.Or(geometry->mBounds, GetBounds(aBuilder, &snap));

don't assign the result of the Or here.

::: layout/base/nsDisplayListInvalidation.h
@@ +10,5 @@
> + * This stores the geometry of an nsDisplayItem, and the area
> + * that will be affected when painting the item.
> + *
> + * It is used to retain information about display items so they
> + * can be compared against new display items in the next paintA

fix typo

@@ +14,5 @@
> + * can be compared against new display items in the next paintA
> + */
> +class nsDisplayItemGeometry
> +{
> +  NS_INLINE_DECL_REFCOUNTING(nsDisplayItemGeometry)

Why do we need to refcount these? That seems unfortunate, and it seems to me we should be able to manually manage these.

@@ +38,5 @@
> +  PRInt32 mAppUnitsPerDevPixel;
> +
> +  /**
> +   * The offset (in pixels) of the TopLeft() of the buffer
> +   * this display item was drawn into.

I'm not quite sure what this means. Is this the display item's GetUnderlyingFrame()->ToReferenceFrame()? Or something else?

@@ +46,5 @@
> +  /**
> +   * The offset (in app units) of the TopLeft() of the buffer
> +   * this display item was drawn into.
> +   */
> +  nsPoint mAppUnitsOffset;

Why do you store both this and mPaintOffset?

::: layout/generic/nsCanvasFrame.h
@@ +246,5 @@
> +    const nsDisplayCanvasBackgroundGeometry* geometry = static_cast<const nsDisplayCanvasBackgroundGeometry*>(aGeometry);
> +    nsRegion invalid;
> +    if (ShouldFixToViewport(aBuilder)) {
> +      // This is incorrect, We definitely need to check more things here. 
> +      return invalid;

So ... what needs to be fixed here? :-)

@@ +259,5 @@
> +        !geometry->mContentRect.IsEqualInterior(GetContentRect())) {
> +      if (!RenderingMightDependOnFrameSize() && geometry->mBounds.TopLeft() == GetBounds(aBuilder, &snap).TopLeft()) {
> +        nsRect vertical, horizontal;
> +        nsLayoutUtils::GetRectDifferenceStrips(geometry->mBounds, GetBounds(aBuilder, &snap), &horizontal, &vertical);
> +        invalid = invalid.Or(vertical, horizontal);

Why not just write invalid.Xor(geometry->mBounds, GetBounds(aBuilder, &snap))?

::: layout/generic/nsTextFrame.h
@@ +542,5 @@
> +    bool operator!=(const LineDecoration& aOther) const {
> +      return mFrame != aOther.mFrame ||
> +             mStyle != aOther.mStyle ||
> +             mColor != aOther.mColor ||
> +             mBaselineOffset != aOther.mBaselineOffset;

return !(*this == aOther);

@@ +567,5 @@
> +    bool operator!=(const TextDecorations& aOther) const {
> +      return mOverlines != aOther.mOverlines ||
> +             mUnderlines != aOther.mUnderlines ||
> +             mStrikes != aOther.mStrikes;
> +    }

Define an operator== and then define operator!= in terms of that.

::: layout/generic/nsTextFrameThebes.cpp
@@ +4385,5 @@
> +  {
> +    aFrame->GetTextDecorations(aFrame->PresContext(), mDecorations);
> +  }
> +  
> +  nsTextFrame::TextDecorations mDecorations;

Add a comment explaining why decorations need to be stored and checked and why style changes aren't enough to pick it up.
Comment 133 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-06 21:54:01 PDT
Comment on attachment 620927 [details] [diff] [review]
Part 9b - Add new frame invalidation API v3

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

Looks like you're maintaining some kind of paint generation counter here. It would be much nicer if you can just keep a frame state bit. Why can't you?

::: layout/generic/nsFrame.cpp
@@ +4753,5 @@
> +void
> +nsIFrame::InvalidateFrame()
> +{
> +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");

Getting the displayRoot here is bad, it requires a walk to the root of the frame tree.

@@ +4766,5 @@
> +{ 
> +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> +  if (displayRoot) {
> +    return mInvalidPaint == displayRoot->PresContext()->PresShell()->GetPaintCount();

This is confusing. So after a call to InvalidateFrame(), IsInvalid returns false?

::: layout/generic/nsIFrame.h
@@ +2853,5 @@
>      VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
>    };
>    bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const;
>  
> +  PRUint32         mInvalidPaint;

What is this? Adding to nsFrame is to be avoided if at all possible.
Comment 134 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-07 02:30:02 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #132)
 
> Could we avoid giving nsDisplayItemGeometry virtual methods by putting those
> virtual methods on the associated nsDisplayItem, and making
> nsDisplayItemGeometry essentially a private data blob for the display item?
> I think that would be a little bit simpler, avoid exposing
> nsDisplayItemGeometry details, and be more efficient (no vtable, hopefully
> no refcount).

The virtual GetInvalidationRegion() on nsDisplayItemGeometry is called when the corresponding item *isn't* added to the thebes layer (ie, content being removed).
So we don't have an nsDisplayItem instance with which to call a virtual function.

> 
> Also, it might be a good idea to allocate nsDisplayItemGeometry objects from
> the presshell arena.

Out of interest, what are the advantages of doing this? Disadvantages?

> > +{
> > +  NS_INLINE_DECL_REFCOUNTING(nsDisplayItemGeometry)
> 
> Why do we need to refcount these? That seems unfortunate, and it seems to me
> we should be able to manually manage these.

I added these when debugging a leak, they can probably go now.

> 
> @@ +38,5 @@
> > +  PRInt32 mAppUnitsPerDevPixel;
> > +
> > +  /**
> > +   * The offset (in pixels) of the TopLeft() of the buffer
> > +   * this display item was drawn into.
> 
> I'm not quite sure what this means. Is this the display item's
> GetUnderlyingFrame()->ToReferenceFrame()? Or something else?

No, this is the ThebesLayer offset. Maybe I should put that instead of calling it the 'buffer'.

> 
> @@ +46,5 @@
> > +  /**
> > +   * The offset (in app units) of the TopLeft() of the buffer
> > +   * this display item was drawn into.
> > +   */
> > +  nsPoint mAppUnitsOffset;
> 
> Why do you store both this and mPaintOffset?

Good question, I can probably remove one of these.

> 
> ::: layout/generic/nsCanvasFrame.h
> @@ +246,5 @@
> > +    const nsDisplayCanvasBackgroundGeometry* geometry = static_cast<const nsDisplayCanvasBackgroundGeometry*>(aGeometry);
> > +    nsRegion invalid;
> > +    if (ShouldFixToViewport(aBuilder)) {
> > +      // This is incorrect, We definitely need to check more things here. 
> > +      return invalid;
> 
> So ... what needs to be fixed here? :-)

I still haven't figured that out yet, will have another think about it.
Comment 135 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-07 02:37:17 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #133)
> Looks like you're maintaining some kind of paint generation counter here. It
> would be much nicer if you can just keep a frame state bit. Why can't you?

Since each frame can have multiple display items (spread amongst multiple layer manager/frame layer builder transactions), it's hard to know when to clear the bit.

The original implementation had state bits, and then walked the entire frame tree after painting clearing them. This was (unsurprisingly) really slow.

Suggestions appreciated.

> 
> ::: layout/generic/nsFrame.cpp
> @@ +4753,5 @@
> > +void
> > +nsIFrame::InvalidateFrame()
> > +{
> > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> 
> Getting the displayRoot here is bad, it requires a walk to the root of the
> frame tree.

We need to access the painting refresh driver and associated presshell/context. Is there a faster way to do this?

> 
> @@ +4766,5 @@
> > +{ 
> > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> > +  if (displayRoot) {
> > +    return mInvalidPaint == displayRoot->PresContext()->PresShell()->GetPaintCount();
> 
> This is confusing. So after a call to InvalidateFrame(), IsInvalid returns
> false?

I guess >= would make more sense here. We don't check this until we've started a paint and incremented the paint counter, so it's 'correct', but maybe not logical.

> 
> ::: layout/generic/nsIFrame.h
> @@ +2853,5 @@
> >      VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
> >    };
> >    bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const;
> >  
> > +  PRUint32         mInvalidPaint;
> 
> What is this? Adding to nsFrame is to be avoided if at all possible.

This is the aforementioned generation counter.
Comment 136 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-08 19:43:51 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #134)
> The virtual GetInvalidationRegion() on nsDisplayItemGeometry is called when
> the corresponding item *isn't* added to the thebes layer (ie, content being
> removed).
> So we don't have an nsDisplayItem instance with which to call a virtual
> function.

When is it not mBounds? Can we make it always be mBounds?

> > Also, it might be a good idea to allocate nsDisplayItemGeometry objects from
> > the presshell arena.
> 
> Out of interest, what are the advantages of doing this? Disadvantages?

Probably none. Forget it for now.

> > I'm not quite sure what this means. Is this the display item's
> > GetUnderlyingFrame()->ToReferenceFrame()? Or something else?
> 
> No, this is the ThebesLayer offset. Maybe I should put that instead of
> calling it the 'buffer'.

Yes, just explain it better.

(In reply to Matt Woodrow (:mattwoodrow) from comment #135)
> Since each frame can have multiple display items (spread amongst multiple
> layer manager/frame layer builder transactions), it's hard to know when to
> clear the bit.
> 
> The original implementation had state bits, and then walked the entire frame
> tree after painting clearing them. This was (unsurprisingly) really slow.

If you have two state bits --- "I'm dirty" and "I have a dirty descendant" --- then walking the frame tree to clear them would be efficient since you only have to walk the has-dirty-descendant frames.

> > ::: layout/generic/nsFrame.cpp
> > @@ +4753,5 @@
> > > +void
> > > +nsIFrame::InvalidateFrame()
> > > +{
> > > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> > 
> > Getting the displayRoot here is bad, it requires a walk to the root of the
> > frame tree.
> 
> We need to access the painting refresh driver and associated
> presshell/context. Is there a faster way to do this?

Good question. One approach would be to use Yet Another State Bit to indicate when a frame is inside a popup (so its display root is not the root frame of the root document). Then this could be fast in the common case --- PresContext()->GetRootPresContext() etc.

> > @@ +4766,5 @@
> > > +{ 
> > > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> > > +  if (displayRoot) {
> > > +    return mInvalidPaint == displayRoot->PresContext()->PresShell()->GetPaintCount();
> > 
> > This is confusing. So after a call to InvalidateFrame(), IsInvalid returns
> > false?
> 
> I guess >= would make more sense here. We don't check this until we've
> started a paint and incremented the paint counter, so it's 'correct', but
> maybe not logical.

Right.
Comment 137 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-08 19:59:30 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #136)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #134)
> > The virtual GetInvalidationRegion() on nsDisplayItemGeometry is called when
> > the corresponding item *isn't* added to the thebes layer (ie, content being
> > removed).
> > So we don't have an nsDisplayItem instance with which to call a virtual
> > function.
> 
> When is it not mBounds? Can we make it always be mBounds?

Items like borders, box shadow outer etc shouldn't need to invalidate the entirety of the bounds.

I think I avoided that for now, since a few corner cases *do* require the bounds to be invalidated, but it seems like an optimizations we'd likely do in the near future.
 
> (In reply to Matt Woodrow (:mattwoodrow) from comment #135)
> > Since each frame can have multiple display items (spread amongst multiple
> > layer manager/frame layer builder transactions), it's hard to know when to
> > clear the bit.
> > 
> > The original implementation had state bits, and then walked the entire frame
> > tree after painting clearing them. This was (unsurprisingly) really slow.
> 
> If you have two state bits --- "I'm dirty" and "I have a dirty descendant"
> --- then walking the frame tree to clear them would be efficient since you
> only have to walk the has-dirty-descendant frames.

Not sure I'm convinced that this is cleaner than a paint generation counter, but sure :)

> 
> > > ::: layout/generic/nsFrame.cpp
> > > @@ +4753,5 @@
> > > > +void
> > > > +nsIFrame::InvalidateFrame()
> > > > +{
> > > > +  nsIFrame *displayRoot = nsLayoutUtils::GetDisplayRootFrame(this);
> > > > +  NS_ASSERTION(displayRoot, "Need a display root to schedule a paint!");
> > > 
> > > Getting the displayRoot here is bad, it requires a walk to the root of the
> > > frame tree.
> > 
> > We need to access the painting refresh driver and associated
> > presshell/context. Is there a faster way to do this?
> 
> Good question. One approach would be to use Yet Another State Bit to
> indicate when a frame is inside a popup (so its display root is not the root
> frame of the root document). Then this could be fast in the common case ---
> PresContext()->GetRootPresContext() etc.

Is there an easy way to set this bit? Or should I wait until I need it, walk up the frame tree the first time and set the bit then if it's valid?
Comment 138 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-08 21:01:05 PDT
Comment on attachment 619830 [details] [diff] [review]
Part 15 - Add table invalidation test

This looks pretty good, by why do you need that enablePrivilege call?
Comment 139 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-08 21:11:57 PDT
(In reply to Boris Zbarsky (:bz) from comment #138)
> Comment on attachment 619830 [details] [diff] [review]
> Part 15 - Add table invalidation test
> 
> This looks pretty good, by why do you need that enablePrivilege call?

With DLBI, only privileged content can retrieve the invalidation rects from a MozAfterPaint event (since we no longer have a way to determine if the rect is due to an invalidation from a sub-document).
Comment 140 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-08 21:24:27 PDT
Ah.  In that case, would it work to remove the enablePrivilege bit and do this instead:

  event = SpecialPowers.wrap(event);

?  That basically puts a system-privileged proxy around the event which then makes all your gets and method calls happen with chrome privileges, as I understand.
Comment 141 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-08 21:48:36 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #134)
> > > +{
> > > +  NS_INLINE_DECL_REFCOUNTING(nsDisplayItemGeometry)
> > 
> > Why do we need to refcount these? That seems unfortunate, and it seems to me
> > we should be able to manually manage these.
> 
> I added these when debugging a leak, they can probably go now.

I take that back, nsTArray requires a copy constructor, which we can't have without refcounting.

ISTR running into issues with the hashtable too.

(In reply to Boris Zbarsky (:bz) from comment #140)
> Ah.  In that case, would it work to remove the enablePrivilege bit and do
> this instead:
> 
>   event = SpecialPowers.wrap(event);
> 
> ?  That basically puts a system-privileged proxy around the event which then
> makes all your gets and method calls happen with chrome privileges, as I
> understand.

Will try it!
Comment 142 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-08 21:51:52 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #141)
> I take that back, nsTArray requires a copy constructor, which we can't have
> without refcounting.

Wouldn't nsTArray<nsAutoPtr<nsDisplayItemGeometry> > work?
Comment 143 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-09 03:56:12 PDT
> > Good question. One approach would be to use Yet Another State Bit to
> > indicate when a frame is inside a popup (so its display root is not the root
> > frame of the root document). Then this could be fast in the common case ---
> > PresContext()->GetRootPresContext() etc.
> 
> Is there an easy way to set this bit? Or should I wait until I need it, walk
> up the frame tree the first time and set the bit then if it's valid?

You could inherit it from parents to children in nsFrame::Init (where we do similar with other bits). Then you just need to set it when constructing root frames for popups.
Comment 144 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-09 03:57:08 PDT
And I guess you'd need to set it when constructing the root frame for a document that's inside a popup. That's not hard, ViewportFrame::Init can search the ancestors.
Comment 145 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-09 14:30:53 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #137)
> I think I avoided that for now, since a few corner cases *do* require the
> bounds to be invalidated, but it seems like an optimizations we'd likely do
> in the near future.

How about, for now, make it nonvirtual and just return the bounds, and then later if we do that optimization you can make it virtual?
Comment 146 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-10 21:53:21 PDT
As a followup, it might be worth ripping out the call DoApplyRenderingChangeToTree behavior in UpdateViewsForTree (replace it with a call to UpdateViewsForTree or something) and making InvalidateFrameSubtree walk through placeholders.

Should IsInvalidInSubtree walk through placeholders?
Comment 147 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-05-10 22:42:50 PDT
Comment on attachment 618528 [details] [diff] [review]
Part 9c - Remove old invalidation code v2

Why is the SchedulePaint() call in nsCSSFrameConstructor::ContentStateChanged needed?  I guess if all that does is set up a refresh tick that's not too bad, but still, would be good to at least have a comment.

>@@ -241,44 +241,12 @@ nsImageLoader::DoRedraw(const nsRect* aD

We're going to need some other way to fix bug 698297.  :(

>@@ -611,17 +611,17 @@ nsFieldSetFrame::Reflow(nsPresContext*  

Why do you need to invalidate in there?

It's worth documenting why rows/rowgroups/cols/colgroups need to override InvalidateFrame() and invalidate the entire table.

r=me modulo these nits.
Comment 148 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-13 14:54:30 PDT
Created attachment 623526 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v4
Comment 149 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-13 14:55:03 PDT
Created attachment 623527 [details] [diff] [review]
Part 9b - Add new frame invalidation API v4
Comment 150 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-13 14:56:56 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #134)
> > 
> > ::: layout/generic/nsCanvasFrame.h
> > @@ +246,5 @@
> > > +    const nsDisplayCanvasBackgroundGeometry* geometry = static_cast<const nsDisplayCanvasBackgroundGeometry*>(aGeometry);
> > > +    nsRegion invalid;
> > > +    if (ShouldFixToViewport(aBuilder)) {
> > > +      // This is incorrect, We definitely need to check more things here. 
> > > +      return invalid;
> > 
> > So ... what needs to be fixed here? :-)

I'll handle this in a separate patch, working on it at the moment.
Comment 151 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-13 16:47:55 PDT
Comment on attachment 623526 [details] [diff] [review]
Part 9a - Add new display list invalidation API to nsDisplayItem and implement it v4

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

r+ with that fixed.

::: layout/base/nsDisplayList.cpp
@@ +1437,5 @@
> +  if (!hasBG)
> +    return false;
> +  const nsStyleBackground* bg = bgSC->GetStyleBackground();
> +
> +  if (!bg->IsTransparent()) {

Why this check? Probably just take it out.

::: layout/base/nsDisplayList.h
@@ +772,5 @@
> +   * @param aGeometry Geometry to be shifted.
> +   * @param aOffset Offset to shift by.
> +   */
> +  virtual void MoveGeometryBy(nsDisplayItemGeometry* aGeometry, const nsPoint& aOffset)
> +  {

Shouldn't this be a virtual method on nsDisplayItemGeometry?

::: layout/base/nsDisplayListInvalidation.h
@@ +32,5 @@
> +
> +  /**
> +   * The appunits per dev pixel for the item's frame.
> +   */
> +  PRInt32 mAppUnitsPerDevPixel;

Might as well make this an nscoord
Comment 152 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-13 16:58:51 PDT
Comment on attachment 623527 [details] [diff] [review]
Part 9b - Add new frame invalidation API v4

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

::: layout/generic/nsIFrame.h
@@ +311,5 @@
>  // context?
>  #define NS_FRAME_FONT_INFLATION_FLOW_ROOT           NS_FRAME_STATE_BIT(42)
>  
> +// Frame is marked as invalid (needs repainting)
> +#define NS_FRAME_INVALID                            NS_FRAME_STATE_BIT(43)

how about calling this NS_FRAME_NEEDS_PAINT

@@ +313,5 @@
>  
> +// Frame is marked as invalid (needs repainting)
> +#define NS_FRAME_INVALID                            NS_FRAME_STATE_BIT(43)
> +
> +// Frame has a descendant frames that is invalid

/frames/frame/

@@ +314,5 @@
> +// Frame is marked as invalid (needs repainting)
> +#define NS_FRAME_INVALID                            NS_FRAME_STATE_BIT(43)
> +
> +// Frame has a descendant frames that is invalid
> +#define NS_FRAME_HAS_INVALID_CHILDREN               NS_FRAME_STATE_BIT(44)

NS_FRAME_DESCENDANT_NEEDS_PAINT

@@ +1377,5 @@
>  
>    /**
> +   * Checks if the current frame-state includes all of the listed bits
> +   */
> +  bool HasStateBits(nsFrameState aBits) { return (mState & aBits) == aBits; }

HasAllStateBits

@@ +2249,5 @@
> +   *
> +   * This includes all display items created by this frame, including
> +   * container types.
> +   */
> +  virtual void InvalidateFrame(bool aSchedulePaint = true);

Use a flags word instead of a boolean parameter.

@@ +2255,5 @@
> +  /**
> +   * Calls InvalidateFrame() on all frames descendant frames (including
> +   * this one).
> +   */
> +  void InvalidateFrameSubtree(bool aSchedulePaint = true);

Here too

@@ +2267,5 @@
> +  /**
> +   * Check if any frame within the frame subtree (including this frame) 
> +   * returns true for IsInvalid().
> +   */
> +  bool IsInvalidInSubtree();

HasInvalidFrameInSubtree

@@ +2274,5 @@
> +  /**
> +   * Removes the invalid state from the current frame and all
> +   * descendant frames.
> +   */
> +  void ClearInvalidations();

ClearInvalidationStateBits

@@ +2879,5 @@
>      VISIBILITY_CROSS_CHROME_CONTENT_BOUNDARY = 0x01
>    };
>    bool IsVisibleConsideringAncestors(PRUint32 aFlags = 0) const;
>  
> +  PRUint32         mInvalidPaint;

Shouldn't this be gone?
Comment 153 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-13 18:20:11 PDT
Created attachment 623545 [details] [diff] [review]
Part 9b - Add new frame invalidation API v5
Comment 154 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-13 19:45:01 PDT
Comment on attachment 623545 [details] [diff] [review]
Part 9b - Add new frame invalidation API v5

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

In nsSubDocumentFrame::BeginSwapDocShells, assert that either both frames have NS_FRAME_WITHIN_POPUP or neither do. I don't think it's worth supporting swapDocShells when one shell is in a popup and the other isn't.

Add a comment to nsLayoutUtils::GetDisplayRootFrame to indicate that it can be optimized to use this new NS_FRAME_WITHIN_POPUP bit.

::: layout/generic/nsFrame.cpp
@@ +4772,5 @@
> +      nsIFrame* root = subdocumentFrame->GetSubdocumentRootFrame();
> +      if (root) {
> +        childListArray.AppendElement(nsIFrame::ChildList(
> +          nsFrameList(root, nsLayoutUtils::GetLastSibling(root)),
> +          nsIFrame::kPrincipalList));

We need to share this code with InternalInvalidateThebesLayersInSubtree (and ClearInvalidationStateBits!). Either add a method to nsIFrame (e.g. GetCrossDocChildLists, next to GetChildLists) or add a helper to nsLayoutUtils.

@@ +4871,5 @@
> +    InvalidateFrame();
> +    return nsnull;
> +  }
> +
> +  SchedulePaint();

Does InvalidateLayer actually need to schedule a view manager flush? I would have thought it doesn't need to.

@@ +4878,5 @@
> +
> +bool 
> +nsIFrame::HasInvalidFrameInSubtree()
> +{
> +  return HasAnyStateBits(NS_FRAME_NEEDS_PAINT | NS_FRAME_DESCENDANT_NEEDS_PAINT);

Might as well make this inline in nsIFrame.h

@@ +8069,5 @@
> +         f && !f->HasAnyStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT);
> +         f = nsLayoutUtils::GetCrossDocParentFrame(f)) {
> +      f->AddStateBits(NS_FRAME_DESCENDANT_NEEDS_PAINT);
> +    }
> +  }

It's probably worth fixing NS_FRAME_WITHIN_POPUP here too.

::: layout/generic/nsIFrame.h
@@ +313,5 @@
>  
> +// Frame is marked as needing painting
> +#define NS_FRAME_NEEDS_PAINT                        NS_FRAME_STATE_BIT(43)
> +
> +// Frame has a descendant frame that needs painting

Note that this includes cross-doc descendants.

@@ +317,5 @@
> +// Frame has a descendant frame that needs painting
> +#define NS_FRAME_DESCENDANT_NEEDS_PAINT             NS_FRAME_STATE_BIT(44)
> +
> +// Frame is a descendant of a popup
> +#define NS_FRAME_WITHIN_POPUP                       NS_FRAME_STATE_BIT(45)

just NS_FRAME_IN_POPUP I think
Comment 155 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-13 20:05:50 PDT
Comment on attachment 618530 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v2

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

Address these and then we'll go around again. Also I expect this patch needs to be updated to account for nsDisplayItemGeometry not being refcounted anymore.

::: layout/base/FrameLayerBuilder.h
@@ +288,5 @@
>     */
>    void AddLayerDisplayItem(Layer* aLayer,
>                             nsDisplayItem* aItem,
> +                           LayerState aLayerState,
> +                           nsDisplayListBuilder* aBuilder,

Let's just give FrameLayerBuilder an mDisplayListBuilder so we don't have to keep passing it around.

@@ +289,5 @@
>    void AddLayerDisplayItem(Layer* aLayer,
>                             nsDisplayItem* aItem,
> +                           LayerState aLayerState,
> +                           nsDisplayListBuilder* aBuilder,
> +                           LayerManager* aManager = nsnull);

Clarify in the comment why aManager is not aLayer->Manager().

@@ +314,5 @@
>     * that renders many display items.
>     */
> +  Layer* GetOldLayerFor(nsIFrame* aFrame, PRUint32 aDisplayItemKey, nsDisplayItemGeometry** aOldGeometry = nsnull);
> +
> +  LayerManager* GetInactiveLayerManagerFor(nsDisplayItem* aItem);

Needs a comment.

@@ +352,5 @@
>     * into a retained layer.
>     * Returns false if it was rendered into a temporary layer manager and then
>     * into a retained layer.
>     */
> +  static bool HasRetainedLayerFor(nsIFrame* aFrame, PRUint32 aDisplayItemKey, LayerManager* aManager);

This is a bit confusing. Clarify what aManager is for.

@@ +448,5 @@
>      PRUint32        mDisplayItemKey;
>      LayerState      mLayerState;
> +    nsRefPtr<LayerManager> mInactiveManager;
> +    nsRefPtr<nsDisplayItemGeometry> mGeometry;
> +    bool            mUsed;

Put the pointer members together at the start of the class.

And document what mUsed means.

@@ +484,5 @@
>  
>    // LayerManagerData needs to see DisplayItemDataEntry.
>    friend class LayerManagerData;
>  
> +  void StoreDataForFrame(nsIFrame* aFrame, const DisplayItemData& data);

Needs a comment.

@@ +529,4 @@
>      nsDisplayItem* mItem;
>      Clip mClip;
> +    nsRefPtr<LayerManager> mInactiveLayer;
> +    bool mShouldPaint;

Document these and move the mInactiveLayer pointer below mItem

@@ +562,5 @@
>    static PLDHashOperator UpdateDisplayItemDataForFrame(DisplayItemDataEntry* aEntry,
>                                                         void* aUserArg);
> +public:
> +  static PLDHashOperator ProcessRemovedDisplayItems(DisplayItemDataEntry* aEntry,
> +                                                       void* aUserArg);

Fix indent

::: layout/base/nsDisplayList.h
@@ +694,5 @@
>    nsRect GetContentRect() {
>      return GetUnderlyingFrame()->GetContentRectRelativeToSelf() + ToReferenceFrame();
>    }
>  
> +  virtual bool Invalidated() { return mFrame ? mFrame->Invalidated() : false; }

Rename to IsInvalidated like we did with nsIFrame
Comment 156 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-13 20:34:51 PDT
Comment on attachment 618531 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v2

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

::: gfx/layers/Layers.h
@@ +974,5 @@
>    nsIntRect mClipRect;
>    nsIntRect mTileSourceRect;
>    PRUint32 mContentFlags;
> +  nsIntRegion mInvalidRegion;
> +  nsIntRegion mLocalInvalidRegion;

Yikes, that makes each layer a lot bigger.

I wonder if we could just give each layer an nsIntRect mInvalidRect, nothing more. This rect would be in the same coordinate space as the mVisibleRect. When changing various layer properties we add to mInvalidRect on the layer, or for transform and clip changes, on its ancestor. Then, when we want to compute the invalid rect for the whole tree, we walk all layers and collect their trnasformed mInvalidRects (clearing them as we go). Would that work and be simpler than this?

::: layout/base/nsDisplayList.cpp
@@ +665,5 @@
> +      nsRect rect(presContext->DevPixelsToAppUnits(r->x),
> +                  presContext->DevPixelsToAppUnits(r->y),
> +                  presContext->DevPixelsToAppUnits(r->width),
> +                  presContext->DevPixelsToAppUnits(r->height));
> +      view->GetViewManager()->InvalidateViewNoSuppression(view, rect);

Can we invalidate the widget directly here instead of going through the view system? That would let us remove a bunch of view code I think.
Comment 157 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-13 20:37:59 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #156)
> I wonder if we could just give each layer an nsIntRect mInvalidRect, nothing
> more. This rect would be in the same coordinate space as the mVisibleRect.
> When changing various layer properties we add to mInvalidRect on the layer,
> or for transform and clip changes, on its ancestor. Then, when we want to
> compute the invalid rect for the whole tree, we walk all layers and collect
> their trnasformed mInvalidRects (clearing them as we go). Would that work
> and be simpler than this?

One problem with that approach would be cases where we temporarily set layer properties to their non-final values, e.g. transforms on video ImageLayers where we set a transform in BuildLayer and update it in ProcessDisplayItems. Similar when PopThebesLayerData calls ConfigureLayer and then updates the transform. But I think those can fairly easily be refactored so that the layer transform is set only once.
Comment 158 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-13 20:48:52 PDT
Comment on attachment 618543 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame.

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

Instead of doing all this, it would be simpler and probably more efficient to give each LayerManager a piece of userdata which is its own dynamically-allocated FramePropertyDescriptor. Then you can just get that and do a direct property lookup to get the LayerManagerData.

::: layout/base/FrameLayerBuilder.cpp
@@ +526,5 @@
> +  for (PRUint32 i = 0; i < array->Length(); i++) {
> +    LayerManagerDataEntry& entry = array->ElementAt(i);
> +    if (entry.mOwner == sWidgetManager) {
> +      array->RemoveElementAt(i);
> +      return;

Should do something to remove the last entry if necessary.

@@ +533,5 @@
> +}
> +
> +void
> +FrameLayerBuilder::ClearManagerData(nsIFrame* aFrame, LayerManagerData* aData)
> +{

This can share code with the sWidgetManager version, right?

::: layout/base/FrameLayerBuilder.h
@@ +295,5 @@
> +    NS_ASSERTION(!sWidgetManager, "Should not be reentering widget transactions!");
> +    sWidgetManager = aManager;
> +  }
> +
> +  static void LeaveWidgetManagerTransaction()

WidgetManager sounds odd. Let's expand this out to EnterWidgetLayerManagerTransaction etc.

@@ +626,5 @@
>  
>    PRUint32                            mContainerLayerGeneration;
>    PRUint32                            mMaxContainerLayerGeneration;
> +
> +  static LayerManager*                sWidgetManager;

Document
Comment 159 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-13 21:07:50 PDT
Created attachment 623575 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v3
Comment 160 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-13 21:17:28 PDT
Created attachment 623576 [details] [diff] [review]
Part 9b - Add new frame invalidation API v6
Comment 161 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-13 22:00:21 PDT
Comment on attachment 623575 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v3

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

With mDisplayListBuilder now in FrameLayerBuilder, we can remove a bunch of parameters from existing methods. Followup.

r+ with the following fixed...

::: layout/base/FrameLayerBuilder.cpp
@@ +601,5 @@
> +
> +  nsIntRegionRectIterator it(r);
> +  s += "< ";
> +  while (const nsIntRect* sr = it.Next())
> +    AppendToString(s, *sr) += "; ";

{}

@@ +650,5 @@
>  /* static */ void
>  FrameLayerBuilder::RemoveFrameFromLayerManager(nsIFrame* aFrame,
>                                                 void* aPropertyValue)
>  {
> +  sDestroyedFrame = aFrame;

Move this down to just above the RemoveEntry call so it's more clear what it's protecting.

@@ +765,5 @@
>    if (!newDisplayItems) {
>      // This frame was visible, but isn't anymore.
>      bool found;
> +    if (data == managerData) {
> +      props.Remove(LayerManagerDataProperty(), &found);

drop 'found' parameter

@@ +1596,5 @@
> +   * display item
> +   * @param aOpaqueRect if non-null, a region of the display item that is opaque
> +   * @param aSolidColor if non-null, indicates that every pixel in aVisibleRect
> +   * will be painted with aSolidColor by the item
> +   */

Why is this comment here? these parameters don't even make sense for this function?

@@ +1907,3 @@
>              GetTranslationForThebesLayer(newLayer));
>        }
>      }

Just return here to avoid overindenting.

@@ +1907,5 @@
>              GetTranslationForThebesLayer(newLayer));
>        }
>      }
> +  } else if (aNewLayer) {
> +    ThebesLayer* newLayer = aNewLayer->AsThebesLayer();

newThebesLayer

Seems like we're going to do a bunch of work even in the case where aNewLayer is brand new. Worth checking if GetValidRegion() is empty here?

@@ +1921,5 @@
> +#ifdef DEBUG_INVALIDATIONS
> +        printf("Display item type %s(%p) added to layer %p!\n", aItem->Name(), f, aNewLayer);
> +#endif
> +      } else if (aItem->IsInvalid()) {
> +        combined = combined.Or(geometry->ComputeInvalidationRegion(), oldGeometry->ComputeInvalidationRegion());

Get rid of unnecessary assignment

@@ +1990,5 @@
> +    if (managerData) {
> +      DisplayItemDataEntry *entry = managerData->mFramesWithLayers.GetEntry(aItem->GetUnderlyingFrame());
> +      if (entry) {
> +        for (PRUint32 i = 0; i < entry->mData.Length(); ++i) {
> +          if (entry->mData[i].mDisplayItemKey == aItem->GetPerFrameKey()) {

Seems like it might be useful to have a helper function to get the DisplayItemDataEntry for a given item and layer manager.

::: layout/base/FrameLayerBuilder.h
@@ +474,5 @@
>     */
>    class DisplayItemData {
>    public:
>      DisplayItemData(Layer* aLayer, PRUint32 aKey, LayerState aLayerState)
> +      : mLayer(aLayer), mDisplayItemKey(aKey), mLayerState(aLayerState), mUsed(false) {}

Fix order to match new declaration order
Comment 162 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-13 22:08:28 PDT
Comment on attachment 623576 [details] [diff] [review]
Part 9b - Add new frame invalidation API v6

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

::: layout/generic/nsFrame.cpp
@@ +1238,5 @@
> +void
> +nsIFrame::GetCrossDocChildLists(nsTArray<ChildList>* aLists) const
> +{
> +  if (!GetFirstPrincipalChild()) {
> +    nsSubDocumentFrame* subdocumentFrame = do_QueryFrame(this);

This doesn't actually save a virtual call. So either call GetChildLists first and then scan it for a first-principal-child, or just remove the GetFirstPrincipalChild check.

@@ +4866,5 @@
> +    InvalidateFrame();
> +    return nsnull;
> +  }
> +
> +  SchedulePaint();

> Does InvalidateLayer actually need to schedule a view
> manager flush? I would have thought it doesn't need to.

@@ +8061,5 @@
> +    }
> +  }
> +
> +  if (aParent->HasAnyStateBits(NS_FRAME_IN_POPUP)) {
> +    AddStateBits(NS_FRAME_IN_POPUP);

You need to set this in all descendants too.

And if the parent's bit is not set, you need to clear this in all descendants that have it set and aren't themselves popups.

::: layout/generic/nsSubDocumentFrame.cpp
@@ +931,5 @@
>      return NS_ERROR_NOT_IMPLEMENTED;
>    }
>  
> +  NS_ASSERTION((HasAnyStateBits(NS_FRAME_IN_POPUP) && other->HasAnyStateBits(NS_FRAME_IN_POPUP)) ||
> +               (!HasAnyStateBits(NS_FRAME_IN_POPUP) && !other->HasAnyStateBits(NS_FRAME_IN_POPUP)),

HasAnyStateBits(NS_FRAME_IN_POPUP) == other->HasAnyStateBits(NS_FRAME_IN_POPUP)
Comment 163 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-14 02:15:33 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #162)

> @@ +4866,5 @@
> > +    InvalidateFrame();
> > +    return nsnull;
> > +  }
> > +
> > +  SchedulePaint();
> 
> > Does InvalidateLayer actually need to schedule a view
> > manager flush? I would have thought it doesn't need to.
> 

Yes it does. This is needed to composite the layers to the screen.
Comment 164 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-15 20:03:36 PDT
Created attachment 624286 [details] [diff] [review]
Part 9b - Add new frame invalidation API v7
Comment 165 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-15 20:04:21 PDT
Created attachment 624287 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v4

Carrying forward r=roc
Comment 166 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-15 20:05:09 PDT
Created attachment 624288 [details] [diff] [review]
Part 9f-1 - Refactor FrameLayerBuilder to not set Layer properties twice
Comment 167 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-15 20:05:58 PDT
Created attachment 624289 [details] [diff] [review]
Part 9f-2 - Compute the invalid area of the layer tree and pass this to the widget v3
Comment 168 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-15 20:06:32 PDT
Created attachment 624290 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v2
Comment 169 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-15 20:48:35 PDT
Comment on attachment 624286 [details] [diff] [review]
Part 9b - Add new frame invalidation API v7

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

::: layout/generic/nsFrame.cpp
@@ +8062,5 @@
> +RemoveInPopupStateBitFromDescendants(nsIFrame* aFrame)
> +{
> +  if (!aFrame->HasAnyStateBits(NS_FRAME_IN_POPUP) ||
> +      aFrame->GetType() == nsGkAtoms::listControlFrame ||
> +      aFrame->GetType() == nsGkAtoms::menuPopupFrame) {

Call nsLayoutUtils::IsPopup
Comment 170 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-15 20:49:55 PDT
Comment on attachment 618520 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v2

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

pending comments in comment #121
Comment 171 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-15 20:50:33 PDT
Comment on attachment 618521 [details] [diff] [review]
Part 2 - Add new API to BasicLayers v2

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

Pending comments in comment #122
Comment 172 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-15 21:04:57 PDT
Comment on attachment 624288 [details] [diff] [review]
Part 9f-1 - Refactor FrameLayerBuilder to not set Layer properties twice

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1178,3 @@
>  {
> +  if (aItemVisible.IsEmpty()) {
> +    aLayer->SetVisibleRegion(aChildBounds);

This doesn't seem right. Why do we need to do this?

If we really don't know what the visible rect is, instead of passing an empty rect, make aItemVisible a const nsIntRect* and pass null when we don't know. But why not just have the caller pass aChildBounds in that case?
Comment 173 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-15 21:24:52 PDT
Comment on attachment 624289 [details] [diff] [review]
Part 9f-2 - Compute the invalid area of the layer tree and pass this to the widget v3

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

I think we need to invalidate in SetClipRect/IntersectClipRect. Hmm, but does that mean we need to get rid of IntersectClipRect and make sure that we only SetClipRect once on the layer?

You need to collect invalidation when SetCurrentImage is called on an image container.

This is better than the previous approach to computing the invalid area, but I'm not totally happy about it. Need to think a bit.

::: gfx/layers/ImageLayers.h
@@ +593,5 @@
> +    changed.Or(mVisibleRegion, aRegion);
> +    if (!changed.IsEmpty()) {
> +      InvalidateRectInSelf(changed.GetBounds());
> +    }
> +

As below, we shouldn't need to invalidate just for a visible region change.

Isn't this actually the same as the inherited method anyway?

::: gfx/layers/Layers.h
@@ +653,5 @@
> +    nsIntRegion changed;
> +    changed.Xor(mVisibleRegion, aRegion);
> +    if (!changed.IsEmpty()) {
> +      InvalidateRectInSelf(changed.GetBounds());
> +    }

Changing the visible region should not in itself require invalidation.

@@ +762,2 @@
>      mTransform = aMatrix;
> +    InvalidateBoundsInParent();

Don't we need to avoid doing any invalidation if mTransform == aMatrix? Otherwise we'll be repainting everything all the time?

@@ +963,5 @@
>    void SetDebugColorIndex(PRUint32 aIndex) { mDebugColorIndex = aIndex; }
>    PRUint32 GetDebugColorIndex() { return mDebugColorIndex; }
>  #endif
>  
> +  nsIntRect GetInvalidRect();

const nsIntRect&

::: layout/base/FrameLayerBuilder.cpp
@@ +1954,5 @@
> +
> +  if (aLayer->GetType() == Layer::TYPE_CONTAINER) {
> +    ContainerLayer *container = static_cast<ContainerLayer*>(aLayer);
> +    for (Layer* l = container->GetFirstChild(); l; l = l->GetNextSibling()) {
> +      temp.Or(temp, CheckLayerInvalidRegion(l, aGenerateInvalidations));

Would be better to have CheckLayerInvalidRegion simply take an nsIntRegion parameter, and combine its region into that parameter.

Though, wouldn't it make more sense here to just accumulate a single rectangle instead of a region?

@@ +1955,5 @@
> +  if (aLayer->GetType() == Layer::TYPE_CONTAINER) {
> +    ContainerLayer *container = static_cast<ContainerLayer*>(aLayer);
> +    for (Layer* l = container->GetFirstChild(); l; l = l->GetNextSibling()) {
> +      temp.Or(temp, CheckLayerInvalidRegion(l, aGenerateInvalidations));
> +    }

You need to process the mask layer (if any) to collect its invalid region.
Comment 174 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-15 21:34:58 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #173)
> ::: gfx/layers/ImageLayers.h
> @@ +593,5 @@
> > +    changed.Or(mVisibleRegion, aRegion);
> > +    if (!changed.IsEmpty()) {
> > +      InvalidateRectInSelf(changed.GetBounds());
> > +    }
> > +
> 
> As below, we shouldn't need to invalidate just for a visible region change.
> 
> Isn't this actually the same as the inherited method anyway?

It's slightly different, in that it's Or instead of Xor. I needed this to pass a test, but it could have been related to not catching the SetCurrentImage call on an image container.

Either that, or we're scaling the contained image to the size of the visible region, and then a visible region change changes every pixel of the image.

> 
> ::: gfx/layers/Layers.h
> @@ +653,5 @@
> > +    nsIntRegion changed;
> > +    changed.Xor(mVisibleRegion, aRegion);
> > +    if (!changed.IsEmpty()) {
> > +      InvalidateRectInSelf(changed.GetBounds());
> > +    }
> 
> Changing the visible region should not in itself require invalidation.

Why not? If a layer expands its visible region, then the result composited to the screen will be different.

> 
> @@ +762,2 @@
> >      mTransform = aMatrix;
> > +    InvalidateBoundsInParent();
> 
> Don't we need to avoid doing any invalidation if mTransform == aMatrix?
> Otherwise we'll be repainting everything all the time?

Good catch, lost that code when rewriting this.

> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1954,5 @@
> > +
> > +  if (aLayer->GetType() == Layer::TYPE_CONTAINER) {
> > +    ContainerLayer *container = static_cast<ContainerLayer*>(aLayer);
> > +    for (Layer* l = container->GetFirstChild(); l; l = l->GetNextSibling()) {
> > +      temp.Or(temp, CheckLayerInvalidRegion(l, aGenerateInvalidations));
> 
> Would be better to have CheckLayerInvalidRegion simply take an nsIntRegion
> parameter, and combine its region into that parameter.
> 
> Though, wouldn't it make more sense here to just accumulate a single
> rectangle instead of a region?

I was worried about breaking some tests that expect 'accurate' MozAfterPaint rects. This might not actually matter though.

> 
> @@ +1955,5 @@
> > +  if (aLayer->GetType() == Layer::TYPE_CONTAINER) {
> > +    ContainerLayer *container = static_cast<ContainerLayer*>(aLayer);
> > +    for (Layer* l = container->GetFirstChild(); l; l = l->GetNextSibling()) {
> > +      temp.Or(temp, CheckLayerInvalidRegion(l, aGenerateInvalidations));
> > +    }
> 
> You need to process the mask layer (if any) to collect its invalid region.

Will do.
Comment 175 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-15 21:36:26 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #172)
> Comment on attachment 624288 [details] [diff] [review]
> Part 9f-1 - Refactor FrameLayerBuilder to not set Layer properties twice
> 
> Review of attachment 624288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +1178,3 @@
> >  {
> > +  if (aItemVisible.IsEmpty()) {
> > +    aLayer->SetVisibleRegion(aChildBounds);
> 
> This doesn't seem right. Why do we need to do this?
> 
> If we really don't know what the visible rect is, instead of passing an
> empty rect, make aItemVisible a const nsIntRect* and pass null when we don't
> know. But why not just have the caller pass aChildBounds in that case?

Because the callers that need to know don't have aChildBounds either (callers of BuildLayer).

I'll switch BuildLayer to be an nsIntRect*, that should fix it.
Comment 176 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-15 21:39:03 PDT
Comment on attachment 624290 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v2

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

r+ with that fixed.

::: layout/base/FrameLayerBuilder.cpp
@@ +62,5 @@
>  using namespace mozilla::layers;
>  
>  namespace mozilla {
>  
> +nsTArray<FramePropertyDescriptor*> gPropertyList;

We're not supposed to have toplevel constructors/destructors. So I guess just heap-allocate the nsTArray and destroy it (and set gPropertyList to null) when the array is empty.

@@ +113,5 @@
> +/* static */ void
> +FrameLayerBuilder::DestroyDisplayItemDataFor(nsIFrame* aFrame)
> +{
> +  for (PRUint32 i = 0; i < gPropertyList.Length(); i++) {
> +    aFrame->Properties().Delete(gPropertyList.ElementAt(i));

move aFrame->Properties() out of the loop.

It would actually be more efficient to make gPropertyList a hashtable and iterate through aFrame's properties, deleting the ones that are in gPropertyList. As it stands, the more windows you have open the longer DestroyDisplayItemDataFor will take, which is bad.

But I think for now this is fine.
Comment 177 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-15 21:41:19 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #174)
> Either that, or we're scaling the contained image to the size of the visible
> region, and then a visible region change changes every pixel of the image.

I don't think we're doing that. We shouldn't be doing that!

> > Changing the visible region should not in itself require invalidation.
> 
> Why not? If a layer expands its visible region, then the result composited
> to the screen will be different.

Yeah but the visible region is just for optimization, in theory it doesn't change what a layer actually renders.

> > Though, wouldn't it make more sense here to just accumulate a single
> > rectangle instead of a region?
> 
> I was worried about breaking some tests that expect 'accurate' MozAfterPaint
> rects. This might not actually matter though.

We can just fix or disable those tests.
Comment 178 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-21 16:32:56 PDT
Created attachment 625817 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v4

Rewrote to use new LayerTreeInvalidation code instead.

I haven't changed invalidating via nsViewManager though, the coordinates go through multiple shifts in that code and it wasn't obvious how I could avoid that.

Might be better as a followup for someone who knows the view system better.
Comment 179 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-21 17:21:36 PDT
Comment on attachment 625817 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v4

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

I really like this approach.

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "LayerTreeInvalidation.h"
> +#include "FrameLayerBuilder.h"

We really shouldn't depend on layout here. I think you should make NotifySubDocInvalidation a callback function that gets passed as a parameter to ComputeDifference.

@@ +34,5 @@
> +
> +static void 
> +NotifySubDocInvalidation(ContainerLayer* aLayer, const nsIntRegion& aRegion)
> +{
> +  ContainerLayerPresContext *pres = static_cast<ContainerLayerPresContext*>(aLayer->GetUserData(&gContainerLayerPresContext));

80 char limit

@@ +56,5 @@
> + * If any of these are a ContainerLayer that reports invalidations to a PresShell,
> + * then report that the entire bounds have changed.
> + */
> +static void
> +NotifyBoundsSubDocInvalidationInChildren(Layer* aLayer)

Yes, this is a terrible name. How about calling this NotifySubdocumentInvalidationRecursive and the callback aNotifySubdocumentInvalidation?

@@ +109,5 @@
> +  virtual nsIntRect ComputeDifferences(Layer* aRoot, PRUint32 aFlags);
> +
> +  nsIntRect ComputeDifference(Layer* aOther, PRUint32 aFlags)
> +  {
> +    NS_ASSERTION(aOther == mLayer, "Can't compare different layers!");

Why even pass aOther as a parameter then?

Take out the aOther parameter and call this ComputeChange.

@@ +115,5 @@
> +    Layer* otherMask = aOther->GetMaskLayer();
> +    nsIntRect* otherClip = aOther->GetClipRect();
> +    nsIntRect result;
> +    if ((!mMaskLayer && otherMask) ||
> +        (mMaskLayer && mMaskLayer->mLayer != otherMask) ||

I think you could just test (mMaskLayer ? mMaskLayer->mLayer : nsnull) != otherMask

@@ +139,5 @@
> +    result = result.Union(TransformRect(aOther->GetInvalidRect(), mTransform));
> +
> +    if (mMaskLayer && otherMask) {
> +      nsIntRect maskDiff = mMaskLayer->ComputeDifference(otherMask, aFlags);
> +      result = result.Union(TransformRect(maskDiff, mTransform));

I think you can save some code here by accumulating an invalid rectangle in pre-mTransform coordinates and then transforming it once.

@@ +144,5 @@
> +    }
> +
> +    if (mUseClipRect && otherClip) {
> +      if (mClipRect != *otherClip) {
> +        nsIntRect combined = mClipRect.Union(*otherClip);

It might be worth catching the cases where only one edge of the clip rectangle moves and so the difference between the old and new rectangles is still a single rectangle. You could just use nsIntRegion tmp; tmp.Xor(mClipRect, *otherClip); tmp.GetBounds().

@@ +153,5 @@
> +    aOther->ClearInvalidation();
> +    return result;
> +  }
> +
> +  virtual nsIntRect ComputeDifferenceInternal(Layer* aOther, PRUint32 aFlags) { return nsIntRect(); }

Remove the aOther parameter and call this ComputeChangeInternal

@@ +188,5 @@
> +        // swap order.
> +        result.Or(result, TransformRect(child->GetVisibleRegion().GetBounds(), child->GetTransform()));
> +        if (i < mChildren.Length()) {
> +          result.Or(result, TransformRect(mChildren[i]->mVisibleBounds, mChildren[i]->mTransform));
> +        }

As before, let's union the pre-transform rects and then transform the results.

@@ +196,5 @@
> +          ClearInvalidations(child);
> +        }
> +      } else {
> +        // Same child, check for differences within the child
> +        result.Or(result, mChildren[i]->ComputeDifference(child, aFlags));

This rect needs to be transformed, does it not?

Actually, I think we can be more accurate if we pass down a matrix that transforms

@@ +215,5 @@
> +
> +    return TransformRect(result.GetBounds(), aOther->GetTransform());
> +  }
> +
> +  nsTArray<nsAutoPtr<LayerPropertiesBase> > mChildren;

I think nsAutoTArray<1>

@@ +230,5 @@
> +  {
> +    ColorLayer* color = static_cast<ColorLayer*>(aOther);
> +
> +    if (mColor != color->GetColor()) {
> +      return TransformRect(aOther->GetVisibleRegion().GetBounds(), aOther->GetTransform());

Probably worth having a helper function in a base class that returns the transformed visible region

@@ +253,5 @@
> +    ImageLayer* image = static_cast<ImageLayer*>(aOther);
> +
> +    if (mContainer != image->GetContainer() ||
> +        mFilter != image->GetFilter()) {
> +      return TransformRect(aOther->GetVisibleRegion().GetBounds(), aOther->GetTransform());

You want to check mScaleMode and mScaleSize here too.

What about the current image changing in the container? We should track that, but it's tricky because the image could change asynchronously. I think we need an invalid flag on ImageContainer that you can use that gets set whenever the current image changes. Gotta handle RemoteImage too.

::: gfx/layers/LayerTreeInvalidation.h
@@ +36,5 @@
> +  /**
> +   * Compares a set of existing layer tree properties to the current layer
> +   * tree and generates the changed rectangle.
> +   *
> +   * @param aProperties Cached properties data.

This parameter does not exist

::: gfx/layers/Layers.h
@@ +945,5 @@
>  
> +  const nsIntRect&  GetInvalidRect() { return mInvalidRect; }
> +  void Invalidate() { mInvalidRect = GetVisibleRegion().GetBounds(); }
> +  void InvalidateRect(const nsIntRect& aRect) { mInvalidRect = mInvalidRect.Union(aRect); }
> +  void ClearInvalidation() { mInvalidRect.SetEmpty(); }

You need to document what the invalid rect means, and document each of these methods.

I think that instead of Invalidate/InvalidateRect we should call this SetInvalidRectToVisibleRegion and AddInvalidRect. Also ClearInvalidRect. This helps distinguish from ThebesLayer::InvalidateRegion.

::: layout/base/FrameLayerBuilder.cpp
@@ +2756,5 @@
>    }
>  
>    FlashPaint(aContext);
> +  if (!aRegionToInvalidate.IsEmpty()) {
> +    aLayer->Invalidate();

might as well invalidate the bounds of aRegionToInvalidate

::: layout/base/nsDisplayList.cpp
@@ +701,5 @@
> +  nsIntRect invalid;
> +  if (props) {
> +    invalid = props->ComputeDifferences(root, computeInvalidFlags);
> +  } else {
> +    LayerProperties::ClearInvalidations(root); 

Do we even need to clear these?

@@ +2149,5 @@
> +
> +  if (mGenerateInvalidations) {
> +    ContainerLayerPresContext* pres = new ContainerLayerPresContext;
> +    pres->mPresContext = mFrame->PresContext();
> +    layer->SetUserData(&gContainerLayerPresContext, pres);

Where does this get cleared?

Why not just set the nsPresContext* as the data directly?

::: layout/base/nsDisplayList.h
@@ +2057,5 @@
>   */
>  class nsDisplayOwnLayer : public nsDisplayWrapList {
>  public:
>    nsDisplayOwnLayer(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
> +                    nsDisplayList* aList, bool aGenerateInvalidations = false);

Boolean parameters suck. Use a flag.

@@ +2284,5 @@
>     */
>    nsDisplayZoom(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
>                  nsDisplayList* aList,
> +                PRInt32 aAPD, PRInt32 aParentAPD,
> +                bool aGenerateInvalidations = false);

Use a flag here too.

::: layout/base/nsPresContext.h
@@ +860,5 @@
>    // Returns true on success and false on failure (not safe).
>    bool EnsureSafeToHandOutCSSRules();
>  
>    void NotifyInvalidation(const nsRect& aRect, PRUint32 aFlags);
> +  void NotifyInvalidation(const nsIntRect& aRect, PRUint32 aFlags);

Document that this is in device pixels

@@ +1083,5 @@
>    void DoChangeCharSet(const nsCString& aCharSet);
>  
> +  bool MayHavePaintEventListener();
> +
> +  bool MayHavePaintEventListenerInSubDocument();

Document these
Comment 180 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-21 17:57:03 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #179)

> @@ +139,5 @@
> > +    result = result.Union(TransformRect(aOther->GetInvalidRect(), mTransform));
> > +
> > +    if (mMaskLayer && otherMask) {
> > +      nsIntRect maskDiff = mMaskLayer->ComputeDifference(otherMask, aFlags);
> > +      result = result.Union(TransformRect(maskDiff, mTransform));
> 
> I think you can save some code here by accumulating an invalid rectangle in
> pre-mTransform coordinates and then transforming it once.

This isn't easy for the transformChanged case, where we have two rects and two transforms. We can't always return early in this path either, since we might need to walk the children to issue separate nsPresContext invalidations.

> 
> You want to check mScaleMode and mScaleSize here too.
> 
> What about the current image changing in the container? We should track
> that, but it's tricky because the image could change asynchronously. I think
> we need an invalid flag on ImageContainer that you can use that gets set
> whenever the current image changes. Gotta handle RemoteImage too.

I was relying on the user calling InvalidateLayer() here. Hopefully they'll be able to know the changed region (say, for an animated image), rather than just invalidating the bounds as Layers would have to.

> ::: layout/base/nsDisplayList.cpp
> @@ +701,5 @@
> > +  nsIntRect invalid;
> > +  if (props) {
> > +    invalid = props->ComputeDifferences(root, computeInvalidFlags);
> > +  } else {
> > +    LayerProperties::ClearInvalidations(root); 
> 
> Do we even need to clear these?

I believe so, otherwise any invalid rects set on layers would be included next time we have a paint listener.

> 
> @@ +2149,5 @@
> > +
> > +  if (mGenerateInvalidations) {
> > +    ContainerLayerPresContext* pres = new ContainerLayerPresContext;
> > +    pres->mPresContext = mFrame->PresContext();
> > +    layer->SetUserData(&gContainerLayerPresContext, pres);
> 
> Where does this get cleared?
> 
> Why not just set the nsPresContext* as the data directly?

Layer user data has to be a subclass of LayerUserData.
Comment 181 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-21 18:16:58 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #179)

> @@ +188,5 @@
> > +        // swap order.
> > +        result.Or(result, TransformRect(child->GetVisibleRegion().GetBounds(), child->GetTransform()));
> > +        if (i < mChildren.Length()) {
> > +          result.Or(result, TransformRect(mChildren[i]->mVisibleBounds, mChildren[i]->mTransform));
> > +        }
> 
> As before, let's union the pre-transform rects and then transform the
> results.
> 

These are different children, with different transforms.
Comment 182 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-21 19:02:37 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #180)
> This isn't easy for the transformChanged case, where we have two rects and
> two transforms. We can't always return early in this path either, since we
> might need to walk the children to issue separate nsPresContext
> invalidations.

Fine, just do it where it's easy...

> > What about the current image changing in the container? We should track
> > that, but it's tricky because the image could change asynchronously. I think
> > we need an invalid flag on ImageContainer that you can use that gets set
> > whenever the current image changes. Gotta handle RemoteImage too.
> 
> I was relying on the user calling InvalidateLayer() here. Hopefully they'll
> be able to know the changed region (say, for an animated image), rather than
> just invalidating the bounds as Layers would have to.

OK, better document that somewhere.

> > ::: layout/base/nsDisplayList.cpp
> > @@ +701,5 @@
> > > +  nsIntRect invalid;
> > > +  if (props) {
> > > +    invalid = props->ComputeDifferences(root, computeInvalidFlags);
> > > +  } else {
> > > +    LayerProperties::ClearInvalidations(root); 
> > 
> > Do we even need to clear these?
> 
> I believe so, otherwise any invalid rects set on layers would be included
> next time we have a paint listener.

That's probably OK though.

> > Why not just set the nsPresContext* as the data directly?
> 
> Layer user data has to be a subclass of LayerUserData.

Oh right. Thanks.

(In reply to Matt Woodrow (:mattwoodrow) from comment #181)
> These are different children, with different transforms.

Oops, right.
Comment 183 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-21 19:49:53 PDT
Created attachment 625862 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v5
Comment 184 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-21 20:10:00 PDT
Created attachment 625866 [details] [diff] [review]
Part 2 - Add new API to BasicLayers v3
Comment 185 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-21 20:10:33 PDT
Created attachment 625867 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v3
Comment 186 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-21 20:11:18 PDT
Created attachment 625868 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v3
Comment 187 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-21 20:27:42 PDT
Comment on attachment 625862 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v5

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +40,5 @@
> +    static_cast<ContainerLayerPresContext*>(aLayer->GetUserData(&gContainerLayerPresContext));
> +  if (pres) {
> +    nsIntPoint topleft = aLayer->GetVisibleRegion().GetBounds().TopLeft();
> +    pres->mCallback(pres->mPresContext, aRegion, topleft);
> +  }

I was hoping this entire function would become the callback so gfx/layers doesn't have to know about ContainerLayerPresContext at all.

@@ +255,5 @@
> +    ImageLayer* image = static_cast<ImageLayer*>(mLayer);
> +
> +    if (mContainer != image->GetContainer() ||
> +        mFilter != image->GetFilter()) {
> +      return NewTransformedBounds();

I think you missed the comment about mScaleMode/mScaleSize?

::: gfx/layers/Layers.h
@@ +918,5 @@
> +  /**
> +   * Returns the current area of the layer (in layer-space coordinates)
> +   * marked as needed to be recomposited.
> +   */
> +  const nsIntRect&  GetInvalidRect() { return mInvalidRect; }

Remove surplus space

::: layout/base/nsDisplayList.cpp
@@ +701,5 @@
> +  nsIntRect invalid;
> +  if (props) {
> +    invalid = props->ComputeDifferences(root, computeInvalidFlags);
> +  } else {
> +    LayerProperties::ClearInvalidations(root); 

I still think you shouldn't clear here. Delivering a whole bunch of invalidations to the paint listener the first time it's called seems OK.

::: layout/base/nsDisplayList.h
@@ +2057,5 @@
>   */
>  class nsDisplayOwnLayer : public nsDisplayWrapList {
>  public:
>    nsDisplayOwnLayer(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
> +                    nsDisplayList* aList, PRUint32 aFlags = 0);

document possible flag values

@@ +2284,5 @@
>     */
>    nsDisplayZoom(nsDisplayListBuilder* aBuilder, nsIFrame* aFrame,
>                  nsDisplayList* aList,
> +                PRInt32 aAPD, PRInt32 aParentAPD,
> +                PRUint32 aFlags = 0);

Document possible flag values

::: layout/base/nsPresContext.h
@@ +1083,5 @@
>  public:
>    void DoChangeCharSet(const nsCString& aCharSet);
>  
> +  /**
> +   * Checks for MozAfterPaint listeners on the document

... and any subdocuments, except for subdocuments that are non-top-level content documents.

::: layout/generic/nsFrame.cpp
@@ +4687,5 @@
>      return nsnull;
>    }
>  
> +  // TODO: Use aDamageRect instead (need to convert to pixels)
> +  layer->SetInvalidRectToVisibleRegion();

Just do it :-)
Comment 188 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-21 20:32:31 PDT
Comment on attachment 625867 [details] [diff] [review]
Part 1 - Allow LayerManagers to have multiple user data objects v3

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

::: gfx/layers/Layers.h
@@ +198,5 @@
>   */
>  
> +static void LayerManagerUserDataDestroy(void *data)
> +{
> +  delete (LayerUserData*)data;

static_cast

@@ +762,5 @@
>     */
>    nsAutoPtr<LayerUserData> RemoveUserData(void* aKey)
> +  { 
> +    nsAutoPtr<LayerUserData> d(static_cast<LayerUserData*>(mUserData.Remove(static_cast<gfx::UserDataKey*>(aKey)))); 
> +    return d; 

it's clearer to write d.forget()
Comment 189 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-21 20:40:46 PDT
Comment on attachment 625868 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v3

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

::: layout/base/FrameLayerBuilder.cpp
@@ +551,5 @@
> +  FrameProperties props = aFrame->Properties();
> +  if (secondary) {
> +    return static_cast<LayerManagerData*>(props.Get(LayerManagerSecondaryDataProperty()));
> +  } else {
> +    return static_cast<LayerManagerData*>(props.Get(LayerManagerDataProperty()));

Declare a static helper function that returns the correct FramePropertyDescriptor* for the current layer manager. That will simplify code here.

@@ +2617,5 @@
>  FrameLayerBuilder::GetDedicatedLayer(nsIFrame* aFrame, PRUint32 aDisplayItemKey)
>  {
> +  //TODO: This isn't completely correct, since a frame could exist as a layer
> +  // in the normal widget manager, and as a different layer (or no layer)
> +  // in the titlebar manager.

"in the secondary manager"

::: layout/base/FrameLayerBuilder.h
@@ +738,5 @@
>    PRUint32                            mMaxContainerLayerGeneration;
> +
> +  /**
> +   * True if the current top-level LayerManager for the widget being
> +   * painted if marked as being a 'secondary' LayerManager.

"is marked"
Comment 190 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-21 20:41:31 PDT
Comment on attachment 618534 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v2

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

I'll look for a new version of this based on previous changes.
Comment 191 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-21 20:48:02 PDT
Comment on attachment 618536 [details] [diff] [review]
Part 10: Test modifications required for DLBI

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

::: layout/reftests/svg/reftest.list
@@ +229,5 @@
>  == svg-in-foreignObject-02.xhtml svg-in-foreignObject-01-ref.xhtml # reuse -01-ref.xhtml
>  == switch-01.svg pass.svg
>  == suspend-01.svg pass.svg
>  == suspend-02.svg pass.svg
> +random == suspend-03.svg pass.svg # bug 724242

I don't understand why this is failing or should be marked random.
Comment 192 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-21 20:59:33 PDT
Comment on attachment 618537 [details] [diff] [review]
Part 11: Reimplement empty transactions

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

::: layout/base/nsIPresShell.h
@@ +1216,5 @@
>     * Notify that the NS_DID_PAINT event was received. Only fires on the
>     * root pres shell.
>     */
>    virtual void DidPaint() = 0;
> +  virtual void ScheduleViewManagerFlush(bool aThebesChanges) = 0;

Use a flag instead of a bool, and document it

::: layout/generic/nsFrame.cpp
@@ +4633,5 @@
> +      aDisplayItemKey == nsDisplayItem::TYPE_PLUGIN ||
> +      aDisplayItemKey == nsDisplayItem::TYPE_CANVAS) {
> +    thebesChanges = false;
> +  }
> +  SchedulePaint(thebesChanges);

I'm not sure why we'd ever want to pass true here.

::: layout/generic/nsIFrame.h
@@ +2104,5 @@
>  
>    /**
>     * Schedule a view manager flush on the next refresh driver tick.
>     */
> +  void SchedulePaint(bool aThebesChanges = true);

Use a flag instead of a bool, and document it
Comment 193 Oleg Romashin (:romaxa) 2012-05-22 00:42:31 PDT
Created attachment 625914 [details] [diff] [review]
Optimize invalidation regions (WR)

I've been testing http://bubblemark.com/dhtml.htm with this patch queue, and found that we wasting bunch of time in Regions processing.
After some investigation I've found that after each ProcessPendingUpdates we have region in thebesLayer containing ~125 rectangles. and for example amount of nsRegion::InsertInPlace - 1.47817e+06 calls per second....
with current trunk we have only ~20000.
This patch practically restoring behavior with dlbi queue back to trunk and basically having minimal amount of rectangles for each paint, and amount of calls for that function gets back to 11-15k
Comment 194 Oleg Romashin (:romaxa) 2012-05-22 00:55:53 PDT
and possibly it make sense to optimize Thebes layer calculations in future to avoid so huge number of region operations...
Comment 195 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-22 16:57:32 PDT
Created attachment 626253 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v6
Comment 196 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-22 16:58:16 PDT
Created attachment 626254 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v3
Comment 197 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-22 16:58:57 PDT
Created attachment 626255 [details] [diff] [review]
Part 11: Reimplement empty transactions v2
Comment 198 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-22 16:59:31 PDT
Created attachment 626256 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v4
Comment 199 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 17:06:31 PDT
Comment on attachment 626253 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v6

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

::: gfx/layers/LayerTreeInvalidation.cpp
@@ +39,5 @@
> +                         NotifySubDocInvalidationFunc aCallback)
> +{
> +  void* data = aLayer->GetUserData(&gNotifySubDocInvalidationData);
> +  nsIntPoint topLeft = aLayer->GetVisibleRegion().GetBounds().TopLeft();
> +  aCallback(data, aRegion, topLeft);

Why not just pass aLayer to the callback function and let it get the user data and compute the visible region top-left?

::: gfx/layers/LayerTreeInvalidation.h
@@ +16,5 @@
> +/**
> + * Callback for ContainerLayer invalidations.
> + *
> + * @param aUserData UserData stored on the ContainerLayer
> + * @param aRegion Invalidated region in the ContainerLayers coordinate

ContainerLayer's

@@ +18,5 @@
> + *
> + * @param aUserData UserData stored on the ContainerLayer
> + * @param aRegion Invalidated region in the ContainerLayers coordinate
> + * space.
> + * @param aTopLeft TopLeft() of the ContainerLayers visible region bounds.

ContainerLayer's

::: layout/generic/nsFrame.cpp
@@ +4690,5 @@
> +  nsIntRect rect(PresContext()->AppUnitsToDevPixels(aDamageRect.x),
> +                 PresContext()->AppUnitsToDevPixels(aDamageRect.y),
> +                 PresContext()->AppUnitsToDevPixels(aDamageRect.width),
> +                 PresContext()->AppUnitsToDevPixels(aDamageRect.height));
> +  layer->AddInvalidRect(rect);

Just use aDamageRect.ToOutsidePixels
Comment 200 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 17:09:32 PDT
Comment on attachment 626254 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v3

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

::: layout/base/nsPresContext.cpp
@@ +2091,5 @@
> +  if (!IsChrome() && !mSendAfterPaintToContent) {
> +    // Don't tell the window about this event, it should not know that
> +    // something happened in a subdocument. Tell only the chrome event handler.
> +    // (Events sent to the window get propagated to the chrome event handler
> +    // automatically.)

We never send paint events to content now, right? So no need to have a mSendAfterPaintToContent variable.
Comment 201 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 17:15:00 PDT
Comment on attachment 626256 [details] [diff] [review]
Part 14 - Handle multiple widget layer managers retaining data for the same frame. v4

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

::: layout/base/FrameLayerBuilder.cpp
@@ +551,5 @@
> +  if (secondary) {
> +    return LayerManagerSecondaryDataProperty();
> +  } else {
> +    return LayerManagerDataProperty();
> +  }

return secondary ? LayerManagerSecondaryDataProperty() : LayerManagerDataProperty();
Comment 202 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-22 17:25:08 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #200)
> Comment on attachment 626254 [details] [diff] [review]
> Part 9g - Modify MozAfterPaint code to work with the new invalidation model
> v3
> 
> Review of attachment 626254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsPresContext.cpp
> @@ +2091,5 @@
> > +  if (!IsChrome() && !mSendAfterPaintToContent) {
> > +    // Don't tell the window about this event, it should not know that
> > +    // something happened in a subdocument. Tell only the chrome event handler.
> > +    // (Events sent to the window get propagated to the chrome event handler
> > +    // automatically.)
> 
> We never send paint events to content now, right? So no need to have a
> mSendAfterPaintToContent variable.

We do if privileges are enabled. It's just non-top-level content documents that never receive them any more.
Comment 203 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 17:30:11 PDT
Comment on attachment 626254 [details] [diff] [review]
Part 9g - Modify MozAfterPaint code to work with the new invalidation model v3

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

::: layout/base/nsPresContext.cpp
@@ +175,5 @@
> +  if (!mInvalidateRequests.mRequests.IsEmpty()) {
> +    return true;    
> +  }
> +  if (GetRootPresContext()->mRefreshDriver->ViewManagerFlushIsPending()) {
> +    NotifyInvalidation(nsRect(0, 0, 0, 0), 0);

Actually ... why is this NotifyInvalidation necessary?

If that's necessary, then that means we send different mozafterpaint events depending on whether DOMWindowUtils::IsMozAfterPaintPending was called?
Comment 204 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-22 17:45:22 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #203)
> Comment on attachment 626254 [details] [diff] [review]
> Part 9g - Modify MozAfterPaint code to work with the new invalidation model
> v3
> 
> Review of attachment 626254 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsPresContext.cpp
> @@ +175,5 @@
> > +  if (!mInvalidateRequests.mRequests.IsEmpty()) {
> > +    return true;    
> > +  }
> > +  if (GetRootPresContext()->mRefreshDriver->ViewManagerFlushIsPending()) {
> > +    NotifyInvalidation(nsRect(0, 0, 0, 0), 0);
> 
> Actually ... why is this NotifyInvalidation necessary?
> 
> If that's necessary, then that means we send different mozafterpaint events
> depending on whether DOMWindowUtils::IsMozAfterPaintPending was called?

That is true. If we return true for IsMozAfterPaintPending, then we need to guarantee that we actually send one.

This just adds an empty rect, in case DLBI comes up with no actual changes. I guess we could change this to a flag of some sort, or strip the empty rect out if there are actual rects when we send the event.

I don't think it would overly matter?
Comment 205 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 17:48:54 PDT
OK. Please add a comment explaining why this is OK.
Comment 206 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-05-22 17:49:08 PDT
Created attachment 626269 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v7
Comment 207 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-22 17:56:00 PDT
Comment on attachment 626269 [details] [diff] [review]
Part 9f - Compute the invalid area of the layer tree and pass this to the widget v7

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

::: gfx/layers/LayerTreeInvalidation.h
@@ +55,5 @@
> +   *
> +   * @param aRoot Root layer of the layer tree to compare against.
> +   * @param aFlags GENERATE_SUBDOC_INVALIDATIONS: For ContainerLayers with
> +   * ContainerLayerPresContext UserData, call the callback with the local
> +   * invalid region.

aFlags no longer exists. Also, move GENERATE_SUBDOC_INVALIDATIONS to nsDisplayOwnLayer.

::: layout/base/nsPresContext.h
@@ +152,5 @@
> +/**
> + * Layer UserData for ContainerLayers that want to be notified
> + * of local invalidations of them and their descendant layers.
> + * Pass GENERATE_SUBDOC_INVALIDATIONS to ComputeDifferences
> + * to have these called.

Fix comment.
Comment 208 Oleg Romashin (:romaxa) 2012-05-22 23:48:28 PDT
Created attachment 626345 [details] [diff] [review]
A bit more smart region calculation
Comment 209 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-06 20:50:31 PDT
Created attachment 630825 [details] [diff] [review]
Part 10: Test modifications required for DLBI v2
Comment 210 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-06 20:51:23 PDT
Created attachment 630826 [details] [diff] [review]
Part 16 - Revoke any pending ViewManager flushes when we do one (sometimes we get this called from Will Paint events)
Comment 211 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-06 20:52:02 PDT
Created attachment 630827 [details] [diff] [review]
Part 17 - Don't paint widgets that an invisible or empty bounds
Comment 212 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-06 20:52:30 PDT
Created attachment 630829 [details] [diff] [review]
Part 18 - Mark frames with only invalid children as an optimization to use when invalidating further frames
Comment 213 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-06 20:52:59 PDT
Created attachment 630830 [details] [diff] [review]
Part 19 - Only repaint retained layers after the previous repainted has been drawn to the window
Comment 214 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-06 20:53:39 PDT
Created attachment 630831 [details] [diff] [review]
Part 20 - Simplify regions to avoid excessive region calculation

Patch originally by romaxa
Comment 215 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-06 20:54:36 PDT
Created attachment 630832 [details] [diff] [review]
Part 21 - BasicLayers should always retain content

We need this because the BasicLayers component alpha flattening can fail (with LAYER_ACTIVE_FORCE items), and we still end up with component alpha layers.
Comment 216 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-06 20:55:54 PDT
Created attachment 630833 [details] [diff] [review]
Part 22 - Force a background color when running android reftests

The part forcing the background color to white should probably be changed here, I couldn't find which theme was causing the current color (0xF3, 0xF3, 0xF3).

Tried editing a native theme files to no avail.
Comment 217 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-06 21:26:14 PDT
Comment on attachment 630825 [details] [diff] [review]
Part 10: Test modifications required for DLBI v2

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

::: layout/base/tests/chrome/test_transformed_scrolling_repaints.html
@@ +30,5 @@
>        t.scrollTop = 10;
>        waitForAllPaintsFlushed(function () {
>          // Clear paint state now and scroll again.
>          utils.checkAndClearPaintedState(e);
> +        t.scrollTop = 15;

Why are you changing this?

::: layout/base/tests/chrome/test_transformed_scrolling_repaints_2.html
@@ +10,5 @@
>  <body onload="setTimeout(startTest,0)">
>  <div id="t" style="-moz-transform: scale(1.1, 1.1); -moz-transform-origin:top left; width:200px; height:100px; background:yellow; overflow:hidden">
> +  <div style="height:40px;"></div>
> +  <div id="e" style="height:30px; background:lime"></div>
> +  <div style="height:300px; background:yellow"></div>

And why are you changing this?

@@ +30,5 @@
>        t.scrollTop = 10;
>        waitForAllPaintsFlushed(function () {
>          // Clear paint state now and scroll again.
>          utils.checkAndClearPaintedState(e);
> +        t.scrollTop = 20;

And why are you changing this?

::: layout/reftests/svg/reftest.list
@@ +155,5 @@
>  == getElementById-a-element-01.svg pass.svg
> +== gradient-live-01a.svg gradient-live-01-ref.svg # bug 696674
> +== gradient-live-01b.svg gradient-live-01-ref.svg # bug 696674
> +== gradient-live-01c.svg gradient-live-01-ref.svg # bug 696674
> +== gradient-live-01d.svg gradient-live-01-ref.svg # bug 696674

You can remove the bug numbers now that these pass.
Comment 218 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-06 21:32:39 PDT
Comment on attachment 630829 [details] [diff] [review]
Part 18 - Mark frames with only invalid children as an optimization to use when invalidating further frames

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

I think you should do in nsFrame::Init what you do in SetParent. It might not matter but it's very efficient and more clearly correct. r+ with that.
Comment 219 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-06 21:34:43 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #217)
> Comment on attachment 630825 [details] [diff] [review]
> Part 10: Test modifications required for DLBI v2
> 
> Review of attachment 630825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/tests/chrome/test_transformed_scrolling_repaints.html
> @@ +30,5 @@
> >        t.scrollTop = 10;
> >        waitForAllPaintsFlushed(function () {
> >          // Clear paint state now and scroll again.
> >          utils.checkAndClearPaintedState(e);
> > +        t.scrollTop = 15;
> 
> Why are you changing this?

We were getting invalidations when content moved out of sight, and the display item gets removed from the ThebesLayer. Then CheckAndClearPaintedState() returns true for other items in the same ThebesLayer because it doesn't cleck the redraw rect.

This just moves the scroll amount around a bit to avoid that.

> 
> ::: layout/base/tests/chrome/test_transformed_scrolling_repaints_2.html
> @@ +10,5 @@
> >  <body onload="setTimeout(startTest,0)">
> >  <div id="t" style="-moz-transform: scale(1.1, 1.1); -moz-transform-origin:top left; width:200px; height:100px; background:yellow; overflow:hidden">
> > +  <div style="height:40px;"></div>
> > +  <div id="e" style="height:30px; background:lime"></div>
> > +  <div style="height:300px; background:yellow"></div>
> 
> And why are you changing this?

We were hitting component alpha problems, so I removed the text. This shouldn't affect the purpose of the test.

> 
> @@ +30,5 @@
> >        t.scrollTop = 10;
> >        waitForAllPaintsFlushed(function () {
> >          // Clear paint state now and scroll again.
> >          utils.checkAndClearPaintedState(e);
> > +        t.scrollTop = 20;
> 
> And why are you changing this?

As above.
Comment 220 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-06 21:36:13 PDT
But maybe it would slow things down because you'd need to sweep the frame tree to clear those bits after first paint, or something like that?
Comment 221 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-06 21:41:14 PDT
Please do add a comment to NS_FRAME_ALL_DESCENDANTS_NEED_PAINT explaining the issue. The invariant is a bit complicated, it's actually "NS_FRAME_ALL_DESCENDANTS_NEED_PAINT means that all frame descendants either have NS_FRAME_NEEDS_PAINT and NS_FRAME_ALL_DESCENDANTS_NEED_PAINT, or they have no display items."
Comment 222 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-06 21:45:16 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #219)
> We were getting invalidations when content moved out of sight, and the
> display item gets removed from the ThebesLayer. Then
> CheckAndClearPaintedState() returns true for other items in the same
> ThebesLayer because it doesn't cleck the redraw rect.
> 
> This just moves the scroll amount around a bit to avoid that.

It would be much better to increase the size of the scrollport or decrease the size of the scrolled content to avoid that issue. These tests depend on the offsets of the scrolled content rounding in certain ways.

> > ::: layout/base/tests/chrome/test_transformed_scrolling_repaints_2.html
> > @@ +10,5 @@
> > >  <body onload="setTimeout(startTest,0)">
> > >  <div id="t" style="-moz-transform: scale(1.1, 1.1); -moz-transform-origin:top left; width:200px; height:100px; background:yellow; overflow:hidden">
> > > +  <div style="height:40px;"></div>
> > > +  <div id="e" style="height:30px; background:lime"></div>
> > > +  <div style="height:300px; background:yellow"></div>
> > 
> > And why are you changing this?
> 
> We were hitting component alpha problems, so I removed the text. This
> shouldn't affect the purpose of the test.

OK yes.
Comment 223 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-06 21:56:12 PDT
Comment on attachment 630830 [details] [diff] [review]
Part 19 - Only repaint retained layers after the previous repainted has been drawn to the window

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

This is pretty confusing, tracking different kinds of painting state on different views and on the widget. It would be great if we can simplify it, but I don't know how...

::: layout/base/nsRefreshDriver.cpp
@@ +415,5 @@
>      printf("Starting ProcessPendingUpdates\n");
>  #endif
>      mViewManagerFlushIsPending = false;
> +    bool skippedFlush = mPresContext->GetPresShell()->GetViewManager()->ProcessPendingUpdates();
> +    mViewManagerFlushIsPending = mViewManagerFlushIsPending || skippedFlush;

mViewManagerFlushIsPending |= skippedFlush

::: view/src/nsView.h
@@ +174,5 @@
>    void DoResetWidgetBounds(bool aMoveOnly, bool aInvalidateChangedSize);
>  
>    nsRegion*    mDirtyRegion;
> +  bool mPendingRefresh;
> +  bool mNeedsPaint;

Document the difference between these flags

::: view/src/nsViewManager.cpp
@@ +378,3 @@
>    for (nsView* childView = aView->GetFirstChild(); childView;
>         childView = childView->GetNextSibling()) {
> +    skipped = skipped || ProcessPendingUpdatesForView(childView, aFlushDirtyRegion);

skipped |=

@@ +384,5 @@
>    // damage is applied based on the final widget geometry
>    if (aFlushDirtyRegion) {
>      nsIWidget *widget = aView->GetWidget();
>      if (widget && widget->NeedsPaint()) {
> +      if (aView->PendingRefresh() && !aView->NeedsPaint()) {

Why do we need to check !aView->NeedsPaint() here?

::: view/src/nsViewManager.h
@@ +104,5 @@
>  
>    /* Update the cached RootViewManager pointer on this view manager. */
>    void InvalidateHierarchy();
>  
> +  virtual bool ProcessPendingUpdates();

Document the return value

@@ +113,5 @@
>  
>  private:
>  
>    void FlushPendingInvalidates();
> +  bool ProcessPendingUpdatesForView(nsView *aView,

And this one
Comment 224 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-06 21:59:37 PDT
Comment on attachment 630832 [details] [diff] [review]
Part 21 - BasicLayers should always retain content

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

Shouldn't we just be removing MustRetainContent since it's always true now?
Comment 225 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-06 22:01:55 PDT
Comment on attachment 630833 [details] [diff] [review]
Part 22 - Force a background color when running android reftests

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

nsWindow::DrawWindowUnderlay should check that WidgetPaintsBackground() is true before actually drawing. We don't want WidgetPaintsBackground() to return false and still be painting the underlay. r+ with that.
Comment 226 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-07 15:28:10 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away June 9-19) from comment #223)

> @@ +384,5 @@
> >    // damage is applied based on the final widget geometry
> >    if (aFlushDirtyRegion) {
> >      nsIWidget *widget = aView->GetWidget();
> >      if (widget && widget->NeedsPaint()) {
> > +      if (aView->PendingRefresh() && !aView->NeedsPaint()) {
> 
> Why do we need to check !aView->NeedsPaint() here?

Occasionally we don't actually get an OS paint event (for reasons unknown), and we end up waiting forever.

This just makes us skip a maximum of once before just painting again.

 
> It would be much better to increase the size of the scrollport or decrease
> the size of the scrolled content to avoid that issue. These tests depend on
> the offsets of the scrolled content rounding in certain ways.

The new numbers are adjusted correctly to account for the rounding.
Comment 227 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-07 15:57:46 PDT
Created attachment 631192 [details] [diff] [review]
Part 19 - Only repaint retained layers after the previous repainted has been drawn to the window v2
Comment 228 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-07 15:58:22 PDT
Created attachment 631193 [details] [diff] [review]
Part 21 - BasicLayers should always retain content v2
Comment 229 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-07 16:03:28 PDT
Created attachment 631197 [details] [diff] [review]
Part 10: Test modifications required for DLBI v3
Comment 230 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-07 17:17:26 PDT
Comment on attachment 631192 [details] [diff] [review]
Part 19 - Only repaint retained layers after the previous repainted has been drawn to the window v2

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

r+ if you do that

::: view/src/nsView.h
@@ +165,5 @@
> +  void SetPendingRefresh(bool aPending) { mPendingRefresh = aPending; }
> +  bool PendingRefresh() { return mPendingRefresh; }
> +
> +  void SetNeedsPaint(bool aNeedsPaint) { mNeedsPaint = aNeedsPaint; }
> +  bool NeedsPaint() { return mNeedsPaint; }

Call this SetNeedsToTriggerPaintAfterRefresh, NeedsToTriggerPaintAfterRefresh.

@@ +181,5 @@
> +
> +  // True if a paint event was skipped while mPendingRefresh was
> +  // true, and a new one should be scheduled once we get
> +  // nsViewManager::Refresh called.
> +  bool mNeedsPaint;

Call this mNeedsToTriggerPaintAfterRefresh?
Comment 231 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-07 18:11:57 PDT
Comment on attachment 618525 [details] [diff] [review]
Part 8 - Move painting of retained layers to the view manager flush, and only composite on the paint event v2

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

r+ anyway. It's probably a good idea to split this patch up into "layer changes" and "the rest" for landing.

::: gfx/layers/basic/BasicLayers.cpp
@@ +1538,5 @@
>  
>    mTransactionIncomplete = false;
>  
> +  if (aFlags & END_NO_COMPOSITE) {
> +    // TODO: We should really just set mTarget to null and make sure we can handle that further down the call chain

Is that really so hard?
Comment 232 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-07 18:14:37 PDT
Comment on attachment 618542 [details] [diff] [review]
Part 13 - Only repaint widgets that have had changes since the last paint

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

::: widget/nsIWidget.h
@@ +1603,5 @@
>       */
>      virtual bool WidgetPaintsBackground() { return false; }
>  
> +    void SetNeedsPaint(bool aNeedsPaint) { mNeedsPaint = aNeedsPaint; }
> +    bool NeedsPaint() { return mNeedsPaint; }

You really need to document what this means and why it's needed. I don't understand myself. In particular, in what situations would ProcessPendingUpdates find that NeedsPaint is false?
Comment 233 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-07 18:29:57 PDT
ProcessPendingUpdates handles (potentially) multiple widgets, and the invalidations recorded might have been for only one of them.

This stops us from having to call nsPresShell::Paint unnecessarily. Possibly a premature optimization.
Comment 234 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-07 18:43:50 PDT
Comment on attachment 618542 [details] [diff] [review]
Part 13 - Only repaint widgets that have had changes since the last paint

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

Hopefully we don't need this.
Comment 235 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-10 21:39:49 PDT
Created attachment 631802 [details] [diff] [review]
Part 23 - Fix MovePixels crash when our surface is in an error state.
Comment 236 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-10 21:50:11 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/6748b5496059
https://hg.mozilla.org/integration/mozilla-inbound/rev/9824458a41e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/f23c3a7e7ce0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6079b229d306
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7f29fcd1845
https://hg.mozilla.org/integration/mozilla-inbound/rev/eec8ef3ebe48
https://hg.mozilla.org/integration/mozilla-inbound/rev/4859876972f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1d43208825c
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cd288a38085
https://hg.mozilla.org/integration/mozilla-inbound/rev/0adc41ff56dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/313af486e68d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c96b4fd4da
https://hg.mozilla.org/integration/mozilla-inbound/rev/783f298401b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/f943b729ac14
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef00ebcd6c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9b3d41e0360
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e07b09c426
https://hg.mozilla.org/integration/mozilla-inbound/rev/2865904db9fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/a284ccb25b83
https://hg.mozilla.org/integration/mozilla-inbound/rev/85fa80bd9792
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a2e9cf8fd41
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c8121f8d3af
https://hg.mozilla.org/integration/mozilla-inbound/rev/61fd66629c4f
Comment 237 Ed Morley [:emorley] 2012-06-11 01:54:15 PDT
This appears to have caused multiple regressions:
https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/ss9mtWc0Q94

Regression :( Trace Malloc Allocs increase 4.74% on CentOS release 5 (Final) Mozilla-Inbound
--------------------------------------------------------------------------------------------
    Previous: avg 463925.033 stddev 517.186 of 30 runs up to revision 828da55601e7
    New     : avg 485894.800 stddev 300.113 of 5 runs since revision 61fd66629c4f
    Change  : +21969.767 (4.74% / z=42.479)
    Graph   : http://mzl.la/OfNiGG 


Talos Regression :( SVG increase 27.5% on Linux Mozilla-Inbound-Non-PGO
Regression :( SVG increase 27.5% on Linux Mozilla-Inbound-Non-PGO
-----------------------------------------------------------------
    Previous: avg 3795.778 stddev 4.853 of 30 runs up to revision 828da55601e7
    New     : avg 4838.982 stddev 14.399 of 5 runs since revision 61fd66629c4f
    Change  : +1043.204 (27.5% / z=214.944)
    Graph   : http://mzl.la/OfNvJS 


Talos Regression :( SVG, Opacity increase 7.4% on Linux Mozilla-Inbound-Non-PGO
Regression :( SVG, Opacity increase 7.4% on Linux Mozilla-Inbound-Non-PGO
-------------------------------------------------------------------------
    Previous: avg 82.683 stddev 1.163 of 30 runs up to revision 828da55601e7
    New     : avg 88.800 stddev 0.908 of 5 runs since revision 61fd66629c4f
    Change  : +6.117 (7.4% / z=5.258)
    Graph   : http://mzl.la/OfNupe 


Talos Regression :( Trace Malloc Allocs increase 4.67% on CentOS (x86_64) release 5 (Final) Mozilla-Inbound
Regression :( Trace Malloc Allocs increase 4.67% on CentOS (x86_64) release 5 (Final) Mozilla-Inbound
-----------------------------------------------------------------------------------------------------
    Previous: avg 455195.667 stddev 1174.193 of 30 runs up to revision 828da55601e7
    New     : avg 476457.600 stddev 1083.631 of 5 runs since revision 61fd66629c4f
    Change  : +21261.933 (4.67% / z=18.108)
    Graph   : http://mzl.la/OfNxRY 


Talos Regression :( Tp5 Row Major MozAfterPaint increase 10.4% on Linux Mozilla-Inbound-Non-PGO
Regression :( Tp5 Row Major MozAfterPaint increase 10.4% on Linux Mozilla-Inbound-Non-PGO
-----------------------------------------------------------------------------------------
    Previous: avg 281.249 stddev 2.599 of 30 runs up to revision 828da55601e7
    New     : avg 310.368 stddev 2.838 of 5 runs since revision 61fd66629c4f
    Change  : +29.118 (10.4% / z=11.204)
    Graph   : http://mzl.la/OfTwGw 

(and some more)
Comment 238 Ed Morley [:emorley] 2012-06-11 02:19:39 PDT
Ok, so this is pretty sad faces, but I've had to back this out for causing mochitest-4 permaorange (as well as the talos regressions in comment 237):
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=61fd66629c4f

eg https://tbpl.mozilla.org/php/getParsedLog.php?id=12544443&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/f08886a8cf22

It's for times like these that a relanding script would be pretty useful, for automating the qimport of a lange range of changesets (or at least it would make me feel less bad about having to back things like this out). 

Sorry! :-(
Comment 239 Ed Morley [:emorley] 2012-06-11 03:20:29 PDT
I think this also possibly broke several of the native Android talos tests.

eg see here and press the down arrow once:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f08886a8cf22&jobname=Android%20Tegra%20250%20mozilla-inbound%20talos
Comment 240 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-06-11 07:59:26 PDT
Note that there were also some perf wins:

*  Tp5 Row Major MozAfterPaint (%CPU) decrease 4.13% on XP Firefox
*  Paint decrease 8.7% on Linux Mozilla-Inbound-Non-PGO
*  DHTML 2 MozAfterPaint decrease 29% on XP Mozilla-Inbound-Non-PGO
Comment 241 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-27 19:11:07 PDT
Created attachment 637344 [details] [diff] [review]
Part 24 - Don't paint android widgets if they aren't at the front, or compositing is disabled
Comment 242 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-27 19:12:00 PDT
Created attachment 637345 [details] [diff] [review]
Part 25 - Invalidate display items that have a changed clip

I have no idea how we passed all tests without this until this week.
Comment 243 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-27 19:52:08 PDT
Comment on attachment 637344 [details] [diff] [review]
Part 24 - Don't paint android widgets if they aren't at the front, or compositing is disabled

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

::: widget/android/nsWindow.cpp
@@ +2345,5 @@
> +
> +  if (FindTopLevel() == nsWindow::TopWindow()) {
> +    return nsIWidget::NeedsPaint();
> +  }
> +  return false;

Do it the other way around:
  if (sCompositorPaused || FindTopLevel() != nsWindow::TopWindow())
    return false;
  return nsIWidget::NeedsPaint();
Comment 244 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-28 05:47:23 PDT
Created attachment 637472 [details] [diff] [review]
Part 26 - Send invalidations for hidden documents.
Comment 245 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-28 05:48:09 PDT
Created attachment 637473 [details] [diff] [review]
Part 27 - Make nsImageFrame's overflow include the AltFeedback image if it will use one
Comment 246 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-28 06:07:37 PDT
Comment on attachment 637472 [details] [diff] [review]
Part 26 - Send invalidations for hidden documents.

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

::: layout/base/nsPresShell.cpp
@@ +606,5 @@
> +}
> +
> +void
> +nsIPresShell::InvalidatePresShellIfHidden() { 
> +  if (!IsVisible()) {

{ on next line
Comment 247 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-28 06:11:46 PDT
Comment on attachment 637473 [details] [diff] [review]
Part 27 - Make nsImageFrame's overflow include the AltFeedback image if it will use one

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

r+ with that.

::: layout/generic/nsImageFrame.cpp
@@ +876,5 @@
> +  if (!imageOK || !haveSize) {
> +    nsRect altFeedbackSize(0, 0,
> +                           2*(nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+ICON_PADDING+ALT_BORDER_WIDTH)),
> +                           2*(nsPresContext::CSSPixelsToAppUnits(ICON_SIZE+ICON_PADDING+ALT_BORDER_WIDTH)));
> +    aMetrics.mOverflowAreas.UnionAllWith(altFeedbackSize);

Let's just add this to the visual overflow area. Changing the scroll overflow area is unnecessary and might conceivably break something. It would be a behavior change, at least.

Also, the area painted by DisplayAltFeedback is relative to the top-left of the content box, so you need to offset the area by aReflowState.mComputedBorderPadding.left/top.
Comment 248 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-29 03:16:33 PDT
Created attachment 637820 [details] [diff] [review]
Part 28 - Cached nsDisplayBackground rasterizations with BasicLayers
Comment 249 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-29 03:17:16 PDT
Note that I ran out of time to look into replacing this code with the simpler version that you suggested. Happy to do this as a followup.
Comment 250 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-29 03:26:47 PDT
Comment on attachment 637820 [details] [diff] [review]
Part 28 - Cached nsDisplayBackground rasterizations with BasicLayers

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

r+ with that, though I think we should do that simplification to just use a surface the size of the viewport.

::: layout/generic/nsCanvasFrame.cpp
@@ +233,5 @@
> +  if (surf) {
> +    BlitSurface(dest, mDestRect, surf);
> +
> +    surf.get()->AddRef();
> +    GetUnderlyingFrame()->Properties().Set(nsIFrame::CachedBackgroundImage(), surf.get());

surf.forget()? Then you don't need to addref.
Comment 251 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-29 19:26:16 PDT
Created attachment 638073 [details] [diff] [review]
Part 29 - Catch OOM exception in java screenshot code

Carrying forward r=jrmuizel from irc.
Comment 252 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-29 19:27:53 PDT
CC'ing mobile folks because of part 29 being particularly horrible.
Comment 253 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-06-29 20:16:24 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/89f35d1fea6c
https://hg.mozilla.org/integration/mozilla-inbound/rev/75419010ac02
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef47dbb6313a
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a41b53f8ada
https://hg.mozilla.org/integration/mozilla-inbound/rev/eea5704272d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c411daf6633
https://hg.mozilla.org/integration/mozilla-inbound/rev/60affaedccc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d97bd4246317
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0a91621ea9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f83491fc735a
https://hg.mozilla.org/integration/mozilla-inbound/rev/f568fc280fb0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e04abde1b323
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba840bf34511
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5e9fefee19
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f3358435ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c75abcb72ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/f09fdc691c66
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b89bbdd7ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c553c4b10e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e794d5f88e0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1ac88bedc2
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef4557011ad3
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba7021170544
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8e99e92344
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e8c5c011767
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7599b247eef
https://hg.mozilla.org/integration/mozilla-inbound/rev/17cc480ae05d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b59fde84e97f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6234134d4430
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6d52bdf2d8

Note that we are still expecting some Talos regressions with this, notably SVG and scrolling (on non-accelerated layer managers). The SVG regression is because of changed paint timing, and means that we're actually measuring the right thing now. Scrolling is necessary to support both this, and OMTC in the future, not much we can do right now.
Comment 254 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-30 05:35:59 PDT
There was a perma-orange test crash in Windows mochitest-2, in indexedDB test_ipc.html. This had been incorrectly starred on our try-server pushes as a different intermittent bug.

That test is special because it exercises e10s on Windows with D3D10. In LayerManagerD3D10::Initialize, the call to HasShadowManager was returning false in the child process where it should have returned true. This is due to changeset bd0a91621ea9, which resolved an ambiguous name by calling LayerManager::HasShadowManager, which always returns false. Instead of doing that, LayerManagerD3D10 needs the same treatment as BasicLayerManager.

In the meantime, I've fixed the orange (I hope) by just calling ShadowLayerForwarder::HasShadowManager, which is what we want here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ae09d4f1d8
This is only a partial fix; other callers of layerManager->HasShadowManager() will still get false, which is wrong. Fortunately I think there aren't any callers for D3D10 right now. Needs to be fixed in followup though.

I also notice that while that test is running in D3D10, the remote IFRAME does not repaint unless I force it to. That also needs to be fixed in followup.

Other followup we discussed already:
-- Fix -moz-element invalidation
-- Simplify part 28 if possible
Comment 255 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-06-30 05:43:51 PDT
Also, there appears to be a new intermittent reftest failure in 482659-1*.html. So it looks like part 27 didn't fix the bug, or something like that.
Comment 256 Ryan VanderMeulen [:RyanVM] 2012-06-30 08:27:13 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #255)
> Also, there appears to be a new intermittent reftest failure in
> 482659-1*.html. So it looks like part 27 didn't fix the bug, or something
> like that.

Feel free to file it next time ;-)
Comment 257 Ryan VanderMeulen [:RyanVM] 2012-06-30 12:41:00 PDT
https://hg.mozilla.org/mozilla-central/rev/89f35d1fea6c
https://hg.mozilla.org/mozilla-central/rev/75419010ac02
https://hg.mozilla.org/mozilla-central/rev/ef47dbb6313a
https://hg.mozilla.org/mozilla-central/rev/3a41b53f8ada
https://hg.mozilla.org/mozilla-central/rev/eea5704272d0
https://hg.mozilla.org/mozilla-central/rev/2c411daf6633
https://hg.mozilla.org/mozilla-central/rev/60affaedccc3
https://hg.mozilla.org/mozilla-central/rev/d97bd4246317
https://hg.mozilla.org/mozilla-central/rev/bd0a91621ea9
https://hg.mozilla.org/mozilla-central/rev/f568fc280fb0
https://hg.mozilla.org/mozilla-central/rev/e04abde1b323
https://hg.mozilla.org/mozilla-central/rev/ba840bf34511
https://hg.mozilla.org/mozilla-central/rev/ce5e9fefee19
https://hg.mozilla.org/mozilla-central/rev/d9f3358435ba
https://hg.mozilla.org/mozilla-central/rev/0c75abcb72ff
https://hg.mozilla.org/mozilla-central/rev/f09fdc691c66
https://hg.mozilla.org/mozilla-central/rev/b7b89bbdd7ab
https://hg.mozilla.org/mozilla-central/rev/3c553c4b10e9
https://hg.mozilla.org/mozilla-central/rev/e794d5f88e0c
https://hg.mozilla.org/mozilla-central/rev/cb1ac88bedc2
https://hg.mozilla.org/mozilla-central/rev/ef4557011ad3
https://hg.mozilla.org/mozilla-central/rev/ba7021170544
https://hg.mozilla.org/mozilla-central/rev/3f8e99e92344
https://hg.mozilla.org/mozilla-central/rev/6e8c5c011767
https://hg.mozilla.org/mozilla-central/rev/f7599b247eef
https://hg.mozilla.org/mozilla-central/rev/17cc480ae05d
https://hg.mozilla.org/mozilla-central/rev/b59fde84e97f
https://hg.mozilla.org/mozilla-central/rev/6234134d4430
https://hg.mozilla.org/mozilla-central/rev/cd6d52bdf2d8
https://hg.mozilla.org/mozilla-central/rev/a9ae09d4f1d8
Comment 258 Geoff Brown [:gbrown] 2012-07-03 09:19:00 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #253)
> Note that we are still expecting some Talos regressions with this, notably
> SVG and scrolling (on non-accelerated layer managers). 

Thanks for the warning. There were very significant Talos regressions for mobile associated with part 29:

Regression Robocop Checkerboarding Benchmark increase 4.07e+03% on Android 2.2 (Native) Mozilla-Inbound
----------------------------------------------------------------------------------------------------------
    Previous: avg 102.631 stddev 38.410 of 30 runs up to revision 0d9f7fb55226
    New     : avg 4276.834 stddev 275.135 of 5 runs since revision cd6d52bdf2d8
    Change  : +4174.203 (4.07e+03% / z=108.674)
    Graph   : http://mzl.la/MxoQlV


Regression Robocop Pan Benchmark increase 81.4% on Android 2.2 (Native) Mozilla-Inbound
------------------------------------------------------------------------------------------
    Previous: avg 42155.963 stddev 3641.365 of 30 runs up to revision 0d9f7fb55226
    New     : avg 76490.320 stddev 8384.926 of 5 runs since revision cd6d52bdf2d8
    Change  : +34334.357 (81.4% / z=9.429)
    Graph   : http://mzl.la/Mxm2oM
Comment 259 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-03 09:29:00 PDT
There are two interesting cases here we should handle (in FrameLayerBuilder?), not sure if we are
 - platform disabled component alpha
 - platform wants component alpha, but is drawing with a basic layer manager and compositing with a GPU layer manager

Fennec and b2g fall into the first bucket, "desktop" omtc will fall into the second.
Comment 260 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-03 18:03:09 PDT
Backed out because of the regressions:

https://hg.mozilla.org/mozilla-central/rev/f56dac9de37c
https://hg.mozilla.org/mozilla-central/rev/48755ba6bb62
https://hg.mozilla.org/mozilla-central/rev/5f34f67d178d
https://hg.mozilla.org/mozilla-central/rev/6f0754272ae0
https://hg.mozilla.org/mozilla-central/rev/6add891f904f
https://hg.mozilla.org/mozilla-central/rev/e596d4c9db2e
https://hg.mozilla.org/mozilla-central/rev/461b05b02842
https://hg.mozilla.org/mozilla-central/rev/6bfde067f700
https://hg.mozilla.org/mozilla-central/rev/6baef7642626
https://hg.mozilla.org/mozilla-central/rev/d6c612f34054
https://hg.mozilla.org/mozilla-central/rev/5bdd6a4fe0fd
https://hg.mozilla.org/mozilla-central/rev/45acaaed2f94
https://hg.mozilla.org/mozilla-central/rev/cf71b613d9e7
https://hg.mozilla.org/mozilla-central/rev/19dcfb96c0d3
https://hg.mozilla.org/mozilla-central/rev/c86e39ee8c55
https://hg.mozilla.org/mozilla-central/rev/cdbbbea09aeb
https://hg.mozilla.org/mozilla-central/rev/02ba5ab5a3f9
https://hg.mozilla.org/mozilla-central/rev/cde7abd8c9dd
https://hg.mozilla.org/mozilla-central/rev/dd21bea4fb77
https://hg.mozilla.org/mozilla-central/rev/3b932903b7b2
https://hg.mozilla.org/mozilla-central/rev/9af1b99d622b
https://hg.mozilla.org/mozilla-central/rev/2d5604335e15
https://hg.mozilla.org/mozilla-central/rev/c22083ebc853
https://hg.mozilla.org/mozilla-central/rev/32e8fc01c01f
https://hg.mozilla.org/mozilla-central/rev/a49eedd187dd
https://hg.mozilla.org/mozilla-central/rev/3c2ce59947ab
https://hg.mozilla.org/mozilla-central/rev/e9117b081b9f
https://hg.mozilla.org/mozilla-central/rev/032329e0f4cc
https://hg.mozilla.org/mozilla-central/rev/ea8c4c02abf9
https://hg.mozilla.org/mozilla-central/rev/37ace49edeb3
https://hg.mozilla.org/mozilla-central/rev/fe89d83d71be
https://hg.mozilla.org/mozilla-central/rev/a6ee1952a230
https://hg.mozilla.org/mozilla-central/rev/87db9617a885
Comment 263 :Ms2ger 2012-07-18 07:38:30 PDT
Pushed

https://hg.mozilla.org/integration/mozilla-inbound/rev/d440311c727c

to fix three unused variable warnings.
Comment 264 Ed Morley [:emorley] 2012-07-19 07:35:49 PDT
https://hg.mozilla.org/mozilla-central/rev/d440311c727c
Comment 267 Girish Sharma [:Optimizer] 2012-07-25 11:27:39 PDT
Due to this landing, this page is completely broken : 
https://dl.dropbox.com/u/28610/bookmarks/index.html

Correct me if I am wrong and this breakage is not due to this bug.
Comment 268 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-25 16:01:31 PDT
This was backed out, so it's probably not your bug.
Comment 269 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-07-25 16:30:11 PDT
I think it might be a regression from 741682, could you file a bug please.

A reduced test case would be really helpful too if possible.
Comment 270 Girish Sharma [:Optimizer] 2012-07-25 21:21:09 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #268)
> This was backed out, so it's probably not your bug.

I searched all the changesets after landing of this bug (https://hg.mozilla.org/mozilla-central/rev/7a376ff3ae84) and could not find a backout of this bug, can you pls provide a link of the backouts ?
Comment 271 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-07-25 21:30:57 PDT
Filed bug 777617 for the issue.
Comment 272 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-07-27 19:09:46 PDT
Created attachment 646797 [details] [diff] [review]
Part 30 - Make ShadowContainerLayerD3D10 hold references to child layers.

Probably more complexity than needed for ContainerLayerD3D10, but it makes it the same as all the other layer types which doesn't hurt.
Comment 273 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-07-27 19:10:31 PDT
Created attachment 646799 [details] [diff] [review]
Part 31 - Call Azure canvas DidTransactionCallback even when painting from DrawWindow().
Comment 274 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-07-27 19:11:23 PDT
Created attachment 646800 [details] [diff] [review]
Part 32 - Change some SVG deferred animation tests to not require calling GetBaseValue.

Carrying forward r=dholbert from irc
Comment 275 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-07-27 19:11:59 PDT
Created attachment 646802 [details] [diff] [review]
Part 33 - Change test-overflow/single-value reftest to use MozReftestInvalidate
Comment 276 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-07-27 21:19:01 PDT
Comment on attachment 646797 [details] [diff] [review]
Part 30 - Make ShadowContainerLayerD3D10 hold references to child layers.

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

::: gfx/layers/d3d10/ContainerLayerD3D10.cpp
@@ +50,5 @@
> +    for (Layer *child = aContainer->GetFirstChild(); 
> +        child; child = child->GetNextSibling()) {
> +            if (aAfter == child) {
> +                Layer *oldNextSibling = child->GetNextSibling();
> +                child->SetNextSibling(aChild);

Indenting here is messed up

@@ +88,5 @@
> +    Layer *lastChild = nsnull;
> +    for (Layer *child = aContainer->GetFirstChild(); child; 
> +        child = child->GetNextSibling()) {
> +            if (child == aChild) {
> +                // We're sure this is not our first child. So lastChild != NULL.

Here too
Comment 277 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-08-11 21:34:04 PDT
Created attachment 651165 [details] [diff] [review]
Make paint timing changes prefable
Comment 280 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-08-27 03:39:11 PDT
https://tbpl.mozilla.org/?tree=Try&rev=52a30e616f8b

Try builds with the remaining patches, fully green.

Anyone interesting in trying these out before I attempt to land again?

I think I've addressed the majority of the blocking bugs, haven't checked them all individually though.
Comment 281 Dennis Jakobsen 2012-08-27 05:48:38 PDT
I'm taking them one by one, started out with MazeSolver and noticed the perfect invalidation on the default 30x30 maze, but large areas are being invalidated on both the 20x20 and 40x40 sized mazes.
Comment 282 Dennis Jakobsen 2012-08-27 05:53:51 PDT
http://ie.microsoft.com/testdrive/Performance/Bubbles/ is doing full page invalidation.
Comment 283 Dennis Jakobsen 2012-08-27 06:00:24 PDT
Also different input events seem to treat the screen differently. If i drag the window scrollbar to the top or bottom of any page larger than it's window, it will only redraw new parts of the page showing up, but if i use my mouse wheel, it will redraw the entire page when i hit the top or bottom.
Comment 284 Dennis Jakobsen 2012-08-27 06:28:46 PDT
Full page invalidation is also happening in about:config, by hovering between different entries. Moving the selected row "cursor" with arrow keys does the same.
Comment 285 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-08-27 18:51:28 PDT
Thanks for taking the time to check these!

MazeSolver - is this different to what we get with trunk? I can definitely reproduce the issue, but it's because the nsTextFrame being added is really big, DLBI can't really fix that.

Maybe we can more intelligently detect which areas of the overflow actually get pixels drawn. That's a bug with nsTextFrame::GetBounds()/GetComponentAlphaBounds() though, and would fix the over-invalidation with DLBI in the process.

roc/bz - any ideas?


Scrolling - again, is this new? I think that's the expected behaviour when the page isn't an integer number of pixels long.

about:config doesn't surprise me, I didn't spend all that much time getting XUL invalidation minimized, will have another look at that code.
Comment 286 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-27 18:55:29 PDT
> but it's because the nsTextFrame being added is really big

There's a textframe being added?
Comment 287 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-08-27 19:10:21 PDT
Sorry, the textframe being added is the timer changing.

Over-invalidation in the maze area looks like a real bug, working on it.
Comment 288 Dennis Jakobsen 2012-08-28 01:02:53 PDT
Happy to help, i've got more!

Keyboard navigating the links on this page, invalidates the entire container. I havent looked at the code, but there seems to be a pattern of invalidating the changing elements parent or event parents parent in certain cases (links inside tables and lists atleast).

Otherwise, most of the form input issues seem to be resolved. Can't confirm if any of the crashes have gone away, i havent had any.

Not sure if this is really an issue, but having paint flashing on also flashes the entire line when i'm typing here :)
Comment 289 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-08-28 17:24:55 PDT
(In reply to Dennis Jakobsen from comment #282)
> http://ie.microsoft.com/testdrive/Performance/Bubbles/ is doing full page
> invalidation.

This is because the page is using top/left to animate the images instead of using transforms. This isn't recommended because it's a lot harder to optimize.

The whole page invalidation is because we try keep the invalidation areas relatively simple and extend them outwards if we get too many rects. 1 rect for each image definitely hits this.

A quick hack to mimic using transforms to animate the motion gives me no invalidation apart from the frame rate counter though.
Comment 290 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-08-28 17:27:13 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #287)
> Sorry, the textframe being added is the timer changing.
> 
> Over-invalidation in the maze area looks like a real bug, working on it.

It was, we were invalidating two rects, one with scaling applied and one without. And then extending the two rects out into a single rect that covered both. :(

Fixed now
Comment 291 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-08-28 18:30:55 PDT
(In reply to Dennis Jakobsen from comment #288)
> Keyboard navigating the links on this page, invalidates the entire
> container. I havent looked at the code, but there seems to be a pattern of
> invalidating the changing elements parent or event parents parent in certain
> cases (links inside tables and lists atleast).

How are you doing this exactly? Using tab only navigates between text boxes.

Latest patch queue is available at: http://hg.mozilla.org/users/mwoodrow_mozilla.com/dlbi

Based on m-c revision 103014:5650196a8c7d. Apply up to patch 28 - svg-observers
Comment 292 Dennis Jakobsen 2012-08-29 00:45:14 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #291)
> (In reply to Dennis Jakobsen from comment #288)
> > Keyboard navigating the links on this page, invalidates the entire
> > container. I havent looked at the code, but there seems to be a pattern of
> > invalidating the changing elements parent or event parents parent in certain
> > cases (links inside tables and lists atleast).
> 
> How are you doing this exactly? Using tab only navigates between text boxes.

If i click in the empty space between the links and start tabbing, i get 1px dashed borders around links.
 
> Latest patch queue is available at:
> http://hg.mozilla.org/users/mwoodrow_mozilla.com/dlbi
> 
> Based on m-c revision 103014:5650196a8c7d. Apply up to patch 28 -
> svg-observers

I'll give it a shot and report my findings.
Comment 293 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-08-29 01:15:52 PDT
On Mac (unlike Windows/Linux), tabbing by default goes to text boxes only.  

You can open System Preferences > Keyboard > Keyboard Shortcuts and change the radio button near the bottom to "All controls" on Mac to change the behavior.
Comment 294 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 16:52:16 PDT
Created attachment 664708 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v5

This has had fairly significant changes since it was last reviewed. In particular, handling multiple DisplayItemData's for a single display item when it has merged frames.
Comment 295 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 16:53:16 PDT
Created attachment 664709 [details] [diff] [review]
Add an option for frames to invalid just a rect instead of the frame bounds
Comment 296 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 16:54:38 PDT
Created attachment 664710 [details] [diff] [review]
Make the table code use rect invalidation to avoid over invalidation

This makes me all sorts of sad, but it's much less disruptive than trying to get each frame to paint it's own content. I'd love to see that done at some point though.
Comment 297 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 16:57:51 PDT
Created attachment 664711 [details] [diff] [review]
Correctly invalidate SVG observers
Comment 298 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 16:59:10 PDT
Created attachment 664712 [details] [diff] [review]
Use rect invalidation in XUL tree's to avoid over invalidation

Another example of us not using per-frame display items and it messing with the assumptions DLBI makes.
Comment 299 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 17:00:30 PDT
Created attachment 664714 [details] [diff] [review]
Handled scrolled inactive layers trees correctly
Comment 300 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 17:02:00 PDT
Created attachment 664715 [details] [diff] [review]
Avoid some causes of unnecessary painting
Comment 301 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-25 20:06:22 PDT
Comment on attachment 664708 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v5

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

::: layout/base/FrameLayerBuilder.cpp
@@ +613,5 @@
>  }
>  
>  } // anonymous namespace
>  
> +uint8_t gContainerLayerPresContext;

static?

@@ +763,5 @@
> +      ThebesDisplayItemLayerUserData* data =
> +          static_cast<ThebesDisplayItemLayerUserData*>(t->GetUserData(&gThebesDisplayItemLayerUserData));
> +      if (data) {
> +        nsRegion old = entry->mData[i]->mGeometry->ComputeInvalidationRegion();
> +        nsIntRegion rgn = old.ScaleToOutsidePixels(data->mXScale, data->mYScale, entry->mData[i]->mGeometry->mAppUnitsPerDevPixel);

do we have to have mAppUnitsPerDevPixel in mGeometry? Or can we have it in ThebesDisplayItemLayerUserData? The latter sounds cheaper.

@@ +764,5 @@
> +          static_cast<ThebesDisplayItemLayerUserData*>(t->GetUserData(&gThebesDisplayItemLayerUserData));
> +      if (data) {
> +        nsRegion old = entry->mData[i]->mGeometry->ComputeInvalidationRegion();
> +        nsIntRegion rgn = old.ScaleToOutsidePixels(data->mXScale, data->mYScale, entry->mData[i]->mGeometry->mAppUnitsPerDevPixel);
> +        rgn.MoveBy(-entry->mData[i]->mGeometry->mPaintOffset);

Ditto, can mPaintOffset be in ThebesDisplayItemLayerUserData?

@@ +848,5 @@
> +  FrameProperties props = f->Properties();
> +  DisplayItemDataEntry* newDisplayItems =
> +    builder ? builder->mNewDisplayItemData.GetEntry(f) : nullptr;
> +
> +  // 

remove comment

@@ +922,5 @@
> +  // When a frame has multiple layer managers (main, inactive, svg), we
> +  // only need to store the outermost one since that will be enough to
> +  // invalidate the entire region covered by all the children.
> +  props.Remove(LayerManagerDataProperty());
> +  props.Set(LayerManagerDataProperty(), data);

This Remove call should be redundant; remove it.

@@ +2249,5 @@
> +    BasicLayerManager* basic = static_cast<BasicLayerManager*>(mInactiveLayer.get());
> +    if (basic->InTransaction()) {
> +      basic->EndTransaction(nullptr, nullptr);
> +    }
> +    basic->SetUserData(&gLayerManagerLayerBuilder, nullptr);

Why is this here? Shouldn't it be in the DrawThebesLayer drawing loop, so we end the transaction on one inactive item before we start drawing the next one?

::: layout/base/FrameLayerBuilder.h
@@ +125,5 @@
>     * Call this just before we end a transaction on aManager. If aManager
>     * is not the retained layer manager then it must be a temporary layer
>     * manager that will not be used again.
>     */
> +  void WillEndTransaction();

Fix comment since aManager is gone. same below.

Do you have a comment update somewhere that says FrameLayerBuilder is one per layer manager now? If not, please add one.

::: layout/base/nsDisplayListInvalidation.h
@@ +50,5 @@
>     * this display item was drawn into.
>     */
>    nsIntPoint mPaintOffset;
> +
> +  nsPoint mActiveScrolledRootOffset;

Document this
Comment 302 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-25 20:12:23 PDT
Comment on attachment 664709 [details] [diff] [review]
Add an option for frames to invalid just a rect instead of the frame bounds

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

::: layout/base/nsDisplayList.h
@@ +2024,2 @@
>      }
> +    // XXX: Is ToReferenceFrame() the same for all frames here?

Yes.

::: layout/generic/nsIFrame.h
@@ +2187,5 @@
>    virtual void InvalidateFrame();
> +
> +  /**
> +   * Same as InvalidateFrame(), but only mark a fixed rect as needing
> +   * repainting.

Add that aRect is relative to the top-left of the frame's border-box.

@@ +2206,5 @@
>     * last paint.
> +   *
> +   * If true, then the invalid rect is returned in aRect, with an
> +   * empty rect meaning all pixels drawn by this frame should be
> +   * invalidated.

How about using GetMaxSizedIntRect to mean all pixels.
Comment 303 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 20:49:30 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #301)
> Comment on attachment 664708 [details] [diff] [review]
> Part 9e - FrameLayerBuilder changes for display list invalidation v5
> 
> Review of attachment 664708 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/FrameLayerBuilder.cpp
> @@ +613,5 @@
> >  }
> >  
> >  } // anonymous namespace
> >  
> > +uint8_t gContainerLayerPresContext;
> 
> static?

No idea what this is even doing here, removed.

> 
> @@ +763,5 @@
> > +      ThebesDisplayItemLayerUserData* data =
> > +          static_cast<ThebesDisplayItemLayerUserData*>(t->GetUserData(&gThebesDisplayItemLayerUserData));
> > +      if (data) {
> > +        nsRegion old = entry->mData[i]->mGeometry->ComputeInvalidationRegion();
> > +        nsIntRegion rgn = old.ScaleToOutsidePixels(data->mXScale, data->mYScale, entry->mData[i]->mGeometry->mAppUnitsPerDevPixel);
> 
> do we have to have mAppUnitsPerDevPixel in mGeometry? Or can we have it in
> ThebesDisplayItemLayerUserData? The latter sounds cheaper.
> 
> @@ +764,5 @@
> > +          static_cast<ThebesDisplayItemLayerUserData*>(t->GetUserData(&gThebesDisplayItemLayerUserData));
> > +      if (data) {
> > +        nsRegion old = entry->mData[i]->mGeometry->ComputeInvalidationRegion();
> > +        nsIntRegion rgn = old.ScaleToOutsidePixels(data->mXScale, data->mYScale, entry->mData[i]->mGeometry->mAppUnitsPerDevPixel);
> > +        rgn.MoveBy(-entry->mData[i]->mGeometry->mPaintOffset);
> 
> Ditto, can mPaintOffset be in ThebesDisplayItemLayerUserData?

Will have a look at these, we need to be sure that they won't have been updated to the new values.


> 
> @@ +922,5 @@
> > +  // When a frame has multiple layer managers (main, inactive, svg), we
> > +  // only need to store the outermost one since that will be enough to
> > +  // invalidate the entire region covered by all the children.
> > +  props.Remove(LayerManagerDataProperty());
> > +  props.Set(LayerManagerDataProperty(), data);
> 
> This Remove call should be redundant; remove it.

It isn't, comment above explains.

> 
> @@ +2249,5 @@
> > +    BasicLayerManager* basic = static_cast<BasicLayerManager*>(mInactiveLayer.get());
> > +    if (basic->InTransaction()) {
> > +      basic->EndTransaction(nullptr, nullptr);
> > +    }
> > +    basic->SetUserData(&gLayerManagerLayerBuilder, nullptr);
> 
> Why is this here? Shouldn't it be in the DrawThebesLayer drawing loop, so we
> end the transaction on one inactive item before we start drawing the next
> one?

Another thing that I probably should have written down at the time. We can't tell exactly which items will actually get processed (in DrawThebesLayer) when we create the ClippedDisplayItems (and start the transaction). If for some reason we didn't call EndTransaction on this item from DrawThebesLayer, then this just ends the transaction to avoid assertions.

I don't remember the exact circumstances that caused this, but I concluded that it was valid behaviour and not a bug.
Comment 304 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-25 21:17:36 PDT
Comment on attachment 664710 [details] [diff] [review]
Make the table code use rect invalidation to avoid over invalidation

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

::: layout/base/nsFrameManager.cpp
@@ +480,5 @@
> +  // This has to sure to invalidate the entire overflow rect; this
> +  // is important in the presence of absolute positioning
> +  if (aInvalidate) {
> +    aOldFrame->InvalidateFrameSubtree();
> +  }

I don't understand why we need this here. Normally DLBI will invalidate for the missing frame.

::: layout/base/nsFrameManager.h
@@ +95,5 @@
>                                      nsFrameList&    aFrameList);
>  
>    NS_HIDDEN_(nsresult) RemoveFrame(ChildListID     aListID,
> +                                   nsIFrame*       aOldFrame,
> +                                   bool            aInvalidate = true);

change this to a flags word and a FLAG_INVALIDATE flag

::: layout/generic/nsFrame.cpp
@@ +4818,5 @@
>  void
>  nsIFrame::InvalidateFrameWithRect(const nsRect& aRect)
>  {
>    if (!HasAnyStateBits(NS_FRAME_NEEDS_PAINT)) {
> +    InvalidateFrameInternal(this);

Can't we just return here? No point in setting the rect property if we've invalidated the entire frame.

::: layout/generic/nsPlaceholderFrame.cpp
@@ +134,5 @@
>  {
>    nsIPresShell* shell = PresContext()->GetPresShell();
>    nsIFrame* oof = mOutOfFlowFrame;
>    if (oof) {
> +    oof->InvalidateFrameSubtree();

Why is this necessary?

::: layout/tables/nsTableCellFrame.cpp
@@ +416,5 @@
> +  nsIFrame::InvalidateFrameWithRect(aRect);
> +  if (!FrameLayerBuilder::HasRetainedDataFor(
> +         this, 
> +         nsDisplayItem::TYPE_TABLE_CELL_BACKGROUND) &&
> +      !IsTransformed()) {

What's the purpose of these checks?

Wouldn't it be fine to just unconditionally invalidate the parent?

I especially don't know why you're checking for TYPE_TABLE_CELL_BACKGROUND here.

@@ +418,5 @@
> +         this, 
> +         nsDisplayItem::TYPE_TABLE_CELL_BACKGROUND) &&
> +      !IsTransformed()) {
> +    nsIFrame *parent = GetParent();
> +    if (parent) {

I think we can assert the parent of a cell is never null!

@@ +419,5 @@
> +         nsDisplayItem::TYPE_TABLE_CELL_BACKGROUND) &&
> +      !IsTransformed()) {
> +    nsIFrame *parent = GetParent();
> +    if (parent) {
> +      parent->InvalidateFrameWithRect(aRect + GetPosition());

When the cell has a filter, the damage rect might need to be increased. Except that I guess it doesn't matter because the filter induces its own inactive layer tree and DLBI handles the invalidation that way. But you might want to comment here (and elsewhere, e.g. nsTableRowFrame) that that's what happens.

@@ +977,5 @@
> +  // If our parent is in initial reflow, it'll handle invalidating our
> +  // entire overflow rect.
> +  if (!(GetParent()->GetStateBits() & NS_FRAME_FIRST_REFLOW) &&
> +      nsSize(aDesiredSize.width, aDesiredSize.height) != GetVisualOverflowRect().Size()) {
> +    InvalidateFrame();

This is a weird comparison, if the visual overflow rect top-left is not 0,0. Maybe you should compare against mRect.Size() instead?

::: layout/tables/nsTableColFrame.cpp
@@ +206,5 @@
> +  nsIFrame::InvalidateFrameWithRect(aRect);
> +  nsIFrame *parent = GetParent();
> +  if (parent) {
> +    parent->InvalidateFrameWithRect(aRect + GetPosition());
> +  }

As above

::: layout/tables/nsTableColGroupFrame.cpp
@@ +480,5 @@
> +  nsIFrame::InvalidateFrameWithRect(aRect);
> +  nsIFrame *parent = GetParent();
> +  if (parent) {
> +    parent->InvalidateFrameWithRect(aRect + GetPosition());
> +  }

As above.

::: layout/tables/nsTableFrame.cpp
@@ +1806,5 @@
>    }
>    aDesiredSize.mOverflowAreas.UnionAllWith(tableRect);
>  
> +  if (GetStateBits() & NS_FRAME_FIRST_REFLOW ||
> +      nsSize(aDesiredSize.width, aDesiredSize.height) != GetVisualOverflowRect().Size()) {

as above

@@ +7249,5 @@
> +  }
> +
> +  // The part that looks at both the rect and the overflow rect is a
> +  // bit of a hack.  See nsBlockFrame::ReflowLine for an eloquent
> +  // description of its hackishness.

Why do we still need to do this?

I would have thought that we wouldn't need to do this anymore.

Can we just check for the border-box top-left change and not check for visual overflow top-left changes?

@@ +7261,5 @@
> +    // aFrame's parent, and reposition that overflow rect to the right
> +    // place.
> +    // XXXbz this doesn't handle outlines, does it?
> +    aFrame->InvalidateFrame();
> +    parent->InvalidateFrameWithRect(aOrigVisualOverflow + aOrigRect.TopLeft());

I wonder if we can avoid passing aOrigVisualOverflow in here. It seems to me that any changes in the visual overflow will have been separately invalidated. So we can just invalidate the new overflow at the old position and new position.

@@ +7265,5 @@
> +    parent->InvalidateFrameWithRect(aOrigVisualOverflow + aOrigRect.TopLeft());
> +  } else {
> +    aFrame->InvalidateFrameWithRect(aOrigVisualOverflow);;
> +    aFrame->InvalidateFrame();
> +    parent->InvalidateFrameWithRect(aOrigRect);;

Remove extra ;

Why are we invalidating in this case? For a potential size change? Due to box-shadow that might affect things in our visual overflow area so we should invalidate our visual overflow area.

::: layout/tables/nsTableFrame.h
@@ +467,5 @@
> +  /**
> +   * To be called on a frame by its parent after setting its size/position and
> +   * calling DidReflow (possibly via FinishReflowChild()).  This can also be
> +   * used for child frames which are not being reflowed but did have their size
> +   * or position changed.

I'd like a better understanding of exactly what this function invalidates.

I think maybe it should simply check whether aOrigRect is equal to the new rect. If it is, and aIsFirstReflow is false, then do nothing, otherwise invalidate the new visual overflow area at the old and new positions.

::: layout/tables/nsTableRowFrame.cpp
@@ +1028,5 @@
>  
> +  // If our parent is in initial reflow, it'll handle invalidating our
> +  // entire overflow rect.
> +  if (!(GetParent()->GetStateBits() & NS_FRAME_FIRST_REFLOW) &&
> +      nsSize(aDesiredSize.width, aDesiredSize.height) != GetVisualOverflowRect().Size()) {

same comment as above

@@ +1391,5 @@
> +         nsDisplayItem::TYPE_TABLE_ROW_BACKGROUND) &&
> +      !IsTransformed()) {
> +    nsIFrame *parent = GetParent();
> +    if (parent) {
> +      parent->InvalidateFrame();

Why not call parent->InvalidateFrameWithRect?

::: layout/tables/nsTableRowGroupFrame.cpp
@@ +1313,5 @@
>  
> +  // If our parent is in initial reflow, it'll handle invalidating our
> +  // entire overflow rect.
> +  if (!(GetParent()->GetStateBits() & NS_FRAME_FIRST_REFLOW) &&
> +      nsSize(aDesiredSize.width, aDesiredSize.height) != GetVisualOverflowRect().Size()) {

as above

@@ +1881,5 @@
> +         nsDisplayItem::TYPE_TABLE_ROW_GROUP_BACKGROUND) &&
> +      !IsTransformed()) {
> +    nsIFrame *parent = GetParent();
> +    if (parent) {
> +      parent->InvalidateFrame();

why not call parent->InvalidateFrameWithRect()?
Comment 305 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-25 21:20:13 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #303)
> > @@ +922,5 @@
> > > +  // When a frame has multiple layer managers (main, inactive, svg), we
> > > +  // only need to store the outermost one since that will be enough to
> > > +  // invalidate the entire region covered by all the children.
> > > +  props.Remove(LayerManagerDataProperty());
> > > +  props.Set(LayerManagerDataProperty(), data);
> > 
> > This Remove call should be redundant; remove it.
> 
> It isn't, comment above explains.

I still don't get it. props.Remove(X); props.Set(X,Y) should be equivalent to props.Set(X,Y), always.

> Another thing that I probably should have written down at the time. We can't
> tell exactly which items will actually get processed (in DrawThebesLayer)
> when we create the ClippedDisplayItems (and start the transaction). If for
> some reason we didn't call EndTransaction on this item from DrawThebesLayer,
> then this just ends the transaction to avoid assertions.

OK, put in a comment about that.
Comment 306 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-25 21:28:16 PDT
Comment on attachment 664715 [details] [diff] [review]
Avoid some causes of unnecessary painting

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

::: layout/generic/nsFrame.cpp
@@ +4651,5 @@
> +
> +  if (aObject->mChangeHint & nsChangeHint_UpdateTransformLayer &&
> +      FrameLayerBuilder::GetDedicatedLayer(f, nsDisplayItem::TYPE_TRANSFORM)) {
> +    f->InvalidateFrameSubtree();
> +  }

Put () around & subexpression and use } else if { (or make a giant boolean expression)

Would it be better to call HasRetainedDataFor? Slightly simpler...

Although, isn't this going to break cases where we have a rendering observer ancestor?

::: layout/generic/nsImageFrame.cpp
@@ +593,5 @@
>           aRect->x, aRect->y, aRect->width, aRect->height);
>  #endif
> +  if (!FrameLayerBuilder::HasRetainedDataFor(this, nsDisplayItem::TYPE_IMAGE) &&
> +      !FrameLayerBuilder::HasRetainedDataFor(this, nsDisplayItem::TYPE_ALT_FEEDBACK)) {
> +    return NS_OK;

Isn't this going to break cases where we have a rendering observer ancestor?
Comment 307 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-25 21:47:04 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #306)
> Isn't this going to break cases where we have a rendering observer ancestor?

How about we add a flag to InvalidateFrame that is set when we have retained data for the frame that needs to be invalidated? When that flag is not set, InvalidateInternal would still walk up through the frame's ancestors and call nsSVGEffects::InvalidateDirectRenderingObservers on them. We could still stop when we reach NS_FRAME_DESCENDANT_NEEDS_PAINT. But we wouldn't add NS_FRAME_NEEDS_PAINT or NS_FRAME_DESCENDANT_NEEDS_PAINT and we wouldn't call SchedulePaint etc.

I wonder if we could pass in the display item type(s) to InvalidateFrame and have it call HasRetainedDataFor and do the right things.
Comment 308 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-25 21:48:44 PDT
or we could spray ANCESTOR_HAS_RENDERING_OBSERVER frame state bits everywhere.
Comment 309 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 21:50:48 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #305)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #303)
> > > @@ +922,5 @@
> > > > +  // When a frame has multiple layer managers (main, inactive, svg), we
> > > > +  // only need to store the outermost one since that will be enough to
> > > > +  // invalidate the entire region covered by all the children.
> > > > +  props.Remove(LayerManagerDataProperty());
> > > > +  props.Set(LayerManagerDataProperty(), data);
> > > 
> > > This Remove call should be redundant; remove it.
> > 
> > It isn't, comment above explains.
> 
> I still don't get it. props.Remove(X); props.Set(X,Y) should be equivalent
> to props.Set(X,Y), always.

Props.Delete(X); props.Set(X,Y); would be equivalent. props.Remove(X) returns the pointer without invoking the destructor, which is what we want here.

> 
> > Another thing that I probably should have written down at the time. We can't
> > tell exactly which items will actually get processed (in DrawThebesLayer)
> > when we create the ClippedDisplayItems (and start the transaction). If for
> > some reason we didn't call EndTransaction on this item from DrawThebesLayer,
> > then this just ends the transaction to avoid assertions.
> 
> OK, put in a comment about that.

Sure.
Comment 310 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-25 21:52:55 PDT
Comment on attachment 664711 [details] [diff] [review]
Correctly invalidate SVG observers

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

I think you need to InvalidateDirectRenderingObservers in nsFrame::DidReflow as well.
Comment 311 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-25 22:10:18 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #304)
> Comment on attachment 664710 [details] [diff] [review]
> Make the table code use rect invalidation to avoid over invalidation
> 
> Review of attachment 664710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/nsFrameManager.cpp
> @@ +480,5 @@
> > +  // This has to sure to invalidate the entire overflow rect; this
> > +  // is important in the presence of absolute positioning
> > +  if (aInvalidate) {
> > +    aOldFrame->InvalidateFrameSubtree();
> > +  }
> 
> I don't understand why we need this here. Normally DLBI will invalidate for
> the missing frame.
> 
This, and the other related frame manager etc changes are just reverting changes from earlier in the patch queue.

This covers the case where we remove a child frame from a table. Since the child might not have any display items, DLBI wouldn't detect it and nothing would be invalidated. But we do need to invalidate the table background display item created by the outer table frame.
Comment 312 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-25 22:23:02 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #311)
> This covers the case where we remove a child frame from a table. Since the
> child might not have any display items, DLBI wouldn't detect it and nothing
> would be invalidated. But we do need to invalidate the table background
> display item created by the outer table frame.

OK then, can we have an nsIFrame::InvalidateFrameForRemoval() method that does nothing for most frames but calls InvalidateFrameSubtree for table parts?
Comment 313 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-26 03:08:31 PDT
Comment on attachment 664710 [details] [diff] [review]
Make the table code use rect invalidation to avoid over invalidation

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

::: layout/tables/nsTableCellFrame.cpp
@@ +416,5 @@
> +  nsIFrame::InvalidateFrameWithRect(aRect);
> +  if (!FrameLayerBuilder::HasRetainedDataFor(
> +         this, 
> +         nsDisplayItem::TYPE_TABLE_CELL_BACKGROUND) &&
> +      !IsTransformed()) {

I only wanted to walk up to the frame that renders the background for this frame. As soon as we hit a transformed frame (guaranteed to be a stacking context, and thus renders the background for all descendants), or a frame that drew a background, we know we're done.

This might just be a waste of time and it would be easier to just walk to the root of the table regardless.
Comment 314 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-26 03:38:32 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #313)
> This might just be a waste of time and it would be easier to just walk to
> the root of the table regardless.

Yes, I think so.
Comment 315 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-26 17:54:24 PDT
Created attachment 665217 [details] [diff] [review]
Fix selectAtPoint.html test

This test needs to wait until paint unsuppression has happened. As written, it can run its tests during the onload event handler, when the content might not be visible so calls to selectAtPoint won't find anything to select.

Alternatively, we could make selectAtPoint ignore paint suppression, but I don't think that's the right thing to do. We won't want users touching the screen and selecting invisible content during paint suppression.
Comment 316 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-26 23:43:15 PDT
Created attachment 665272 [details] [diff] [review]
Fix mActiveScrolledRootOffset to be offset to the reference frame

This fixes the issues with pastebin I was seeing.

I also included some comment fixes and removed some dead code the compiler was complaining about.
Comment 317 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 02:31:46 PDT
Created attachment 665308 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v6
Comment 318 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 02:32:46 PDT
Created attachment 665309 [details] [diff] [review]
Add an option for frames to invalid just a rect instead of the frame bounds v2
Comment 319 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 02:33:55 PDT
Created attachment 665310 [details] [diff] [review]
Make the table code use rect invalidation to avoid over invalidation v2
Comment 320 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 02:35:12 PDT
Created attachment 665312 [details] [diff] [review]
Add HasRetainedDataFor
Comment 321 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 02:36:58 PDT
Created attachment 665313 [details] [diff] [review]
Avoid some causes of unnecessary painting v2
Comment 322 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 02:37:59 PDT
Created attachment 665315 [details] [diff] [review]
Fix mActiveScrolledRootOffset to be offset to the reference frame

Rebased against my other changes. r=me
Comment 323 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-27 03:03:03 PDT
Comment on attachment 665308 [details] [diff] [review]
Part 9e - FrameLayerBuilder changes for display list invalidation v6

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

::: layout/base/FrameLayerBuilder.cpp
@@ +498,5 @@
>     */
>    float mXScale, mYScale;
>  
>    /**
> +   * The appunits per dev pixel for the item in this layer.

for the items in this layer

@@ +525,5 @@
>  
> +  nsIntRegion mRegionToInvalidate;
> +
> +  // The result of GetPosition() for the active scrolled root of this layer
> +  // for the previous and current paints respectively.

This comment will need to be updated in my patch to make these relative to the reference frame.
Comment 324 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-27 03:22:34 PDT
Comment on attachment 665310 [details] [diff] [review]
Make the table code use rect invalidation to avoid over invalidation v2

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

::: layout/base/nsFrameManager.cpp
@@ +479,5 @@
> +  // that doesn't change the size of the parent.)
> +  // This has to sure to invalidate the entire overflow rect; this
> +  // is important in the presence of absolute positioning
> +  if (!(aFlags & DONT_INVALIDATE)) {
> +    aOldFrame->InvalidateFrameSubtree();

Instead of using a flag I thought you'd just call InvalidateFrameForRemoval() here... You don't seem to have added any callers for InvalidateFrameForRemoval().

::: layout/generic/nsFrame.cpp
@@ +4823,1 @@
>    } 

Can't we just return here? No point in setting the rect property if we've invalidated the entire frame.

::: layout/generic/nsPlaceholderFrame.cpp
@@ +134,5 @@
>  {
>    nsIPresShell* shell = PresContext()->GetPresShell();
>    nsIFrame* oof = mOutOfFlowFrame;
>    if (oof) {
> +    oof->InvalidateFrameSubtree();

Why is this necessary?

::: layout/tables/nsTableFrame.cpp
@@ +1805,5 @@
>      tableRect.Inflate(bcMargin);
>    }
>    aDesiredSize.mOverflowAreas.UnionAllWith(tableRect);
>  
> +  if (GetStateBits() & NS_FRAME_FIRST_REFLOW ||

() around & subexpression

@@ +7249,5 @@
> +  }
> +
> +  // The part that looks at both the rect and the overflow rect is a
> +  // bit of a hack.  See nsBlockFrame::ReflowLine for an eloquent
> +  // description of its hackishness.

Need a note somewhere that this still doesn't make much sense and needs to be cleaned up later.
Comment 325 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-27 03:30:30 PDT
Comment on attachment 665312 [details] [diff] [review]
Add HasRetainedDataFor

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

::: layout/base/FrameLayerBuilder.cpp
@@ +1121,5 @@
> +  }
> +  data = GetSecondaryLayerManagerDataForFrame(aFrame);
> +  if (data && GetDisplayItemDataForManager(aFrame, aDisplayItemKey, data)) {
> +    return true;
> +  }

We have to do something for non-Mac platforms to ensure that we don't have overhead for the secondary layer manager. But it can be deferred.

::: layout/base/nsPresShell.cpp
@@ +3969,5 @@
>  
>    if (aStateMask.HasState(NS_DOCUMENT_STATE_WINDOW_INACTIVE)) {
>      nsIFrame* root = mFrameConstructor->GetRootFrame();
>      if (root) {
> +      FrameLayerBuilder::InvalidateAllLayersForFrame(root);

Why have you made this change?
Comment 326 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-27 03:39:01 PDT
Comment on attachment 665313 [details] [diff] [review]
Avoid some causes of unnecessary painting v2

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

::: layout/generic/nsFrame.cpp
@@ +4643,5 @@
> +
> +  // if there are hints other than transform/opacity, invalidate, since we don't know what else to do.
> +  if (aObject->mChangeHint & ~(nsChangeHint_UpdateOpacityLayer|nsChangeHint_UpdateTransformLayer)) {
> +    f->InvalidateFrameSubtree();
> +  }

The remaining cases can be in an else clause, since there's no need to do any more work once we've invalidated the entire subtree.

::: layout/generic/nsIFrame.h
@@ +2216,5 @@
>     * Normally does nothing since DLBI handles removed frames.
> +   * 
> +   * @param aDisplayItemKey If specified, only issues an invalidate
> +   * if this frame painted a display item of that type during the 
> +   * previous paint. SVG rendering observers are always notified.

You didn't actually add the parameter. I think you should not be adding this comment.

::: layout/generic/nsImageFrame.cpp
@@ +599,3 @@
>    } else {
> +    InvalidateFrameWithRect(SourceRectToDest(*aRect), nsDisplayItem::TYPE_IMAGE);
> +    InvalidateFrameWithRect(SourceRectToDest(*aRect), nsDisplayItem::TYPE_ALT_FEEDBACK);

Factor out SourceRectToDest into a local variable.
Comment 327 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-27 03:40:46 PDT
For stuff that's changing offscreen, having to scan frame ancestors all the time just in case there are SVG rendering observers sucks. We should fix that in followup.
Comment 328 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 03:50:33 PDT
Created attachment 665338 [details] [diff] [review]
Correctly invalidate SVG observers v2
Comment 329 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-27 03:57:03 PDT
Comment on attachment 665338 [details] [diff] [review]
Correctly invalidate SVG observers v2

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

::: layout/svg/base/src/nsSVGEffects.h
@@ +77,5 @@
>    nsIFrame* GetReferencedFrame(nsIAtom* aFrameType, bool* aOK);
>  
>    Element* GetReferencedElement();
>  
> +  virtual bool ShouldObserveReflow() { return true; }

Call it ObservesReflow()
Comment 330 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 04:34:19 PDT
> 
> ::: layout/generic/nsFrame.cpp
> @@ +4823,1 @@
> >    } 
> 
> Can't we just return here? No point in setting the rect property if we've
> invalidated the entire frame.
> 

We didn't invalidate the entire frame really. The rect is an additional property on top of the state bit, so that just makes sure the bit is set, and paints are scheduled. Then the rest of the function sets the actual rect.
Comment 331 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 04:35:31 PDT
> ::: layout/base/nsPresShell.cpp
> @@ +3969,5 @@
> >  
> >    if (aStateMask.HasState(NS_DOCUMENT_STATE_WINDOW_INACTIVE)) {
> >      nsIFrame* root = mFrameConstructor->GetRootFrame();
> >      if (root) {
> > +      FrameLayerBuilder::InvalidateAllLayersForFrame(root);
> 
> Why have you made this change?

I decided that it made debugging easier at the time since you don't get a massive influx of InvalidateFrameSubtree() calls when switching to gdb.
Comment 332 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 04:53:48 PDT
Created attachment 665354 [details] [diff] [review]
Make the table code use rect invalidation to avoid over invalidation v3
Comment 333 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-27 05:28:42 PDT
Created attachment 665368 [details] [diff] [review]
Fix PruneDisplayListForExtraPage
Comment 334 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-27 05:37:28 PDT
Created attachment 665372 [details] [diff] [review]
Add an option for frames to invalid just a rect instead of the frame bounds v3

Fixed a bug where we could overwrite an already invalid frame with a partial rect.
Comment 335 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-27 05:45:40 PDT
Comment on attachment 665372 [details] [diff] [review]
Add an option for frames to invalid just a rect instead of the frame bounds v3

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

More followup: fix IsInvalid to return something other than an empty rect to mean "everything"!
Comment 336 :Ehsan Akhgari (busy, don't ask for review please) 2012-09-27 07:06:16 PDT
Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2633180110 to fix the reftest manifest on top of this landing, but there's still reftest leaks on OS X:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef035122864a
https://hg.mozilla.org/integration/mozilla-inbound/rev/074075d10fd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/76bc2ff685f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/932a03fc9d44
https://hg.mozilla.org/integration/mozilla-inbound/rev/64cd1b640565
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c5620eaac6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/16856e7778fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/53823eee494f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e004a9acf00d
https://hg.mozilla.org/integration/mozilla-inbound/rev/39e70f91d826
https://hg.mozilla.org/integration/mozilla-inbound/rev/625343f34ca2
https://hg.mozilla.org/integration/mozilla-inbound/rev/226afc8b39b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b06b79fe138
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3c797462d2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7676af66ca43
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cc58b98cbf9
https://hg.mozilla.org/integration/mozilla-inbound/rev/01095a6c8926
https://hg.mozilla.org/integration/mozilla-inbound/rev/eeada32fd169
https://hg.mozilla.org/integration/mozilla-inbound/rev/daa0868b05bf
https://hg.mozilla.org/integration/mozilla-inbound/rev/a55f82f0feca
https://hg.mozilla.org/integration/mozilla-inbound/rev/bcd22436baf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/20ffee0f2e7d
https://hg.mozilla.org/integration/mozilla-inbound/rev/33be0c4f310d
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c1da25b8452
https://hg.mozilla.org/integration/mozilla-inbound/rev/b12bf692dc2d
https://hg.mozilla.org/integration/mozilla-inbound/rev/213b15f0c3c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbcc52822df2
https://hg.mozilla.org/integration/mozilla-inbound/rev/66f6deffdc70
https://hg.mozilla.org/integration/mozilla-inbound/rev/920cf04e0fe0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e70defa60099
Comment 337 Ed Morley [:emorley] 2012-09-27 08:55:59 PDT
Sorry, we had to backout due to crashes on Windows debug mochitests:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e70defa60099

There were also seemingly frequent failures in browser/devtools/highlighter/test/browser_inspector_invalidate.js.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f86e3a3240
Comment 338 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-09-27 22:33:48 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #336)
> Landed https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2633180110
> to fix the reftest manifest on top of this landing, but there's still
> reftest leaks on OS X:

I don't see any reftest leaks on pushes with the reftest manifest fixed, so hopefully the leaks were related to the reftest manifest error.
Comment 339 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-28 04:15:42 PDT
Created attachment 665853 [details] [diff] [review]
Disable test_text on android since it causes crashes
Comment 340 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-09-28 04:17:33 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #339)
> Created attachment 665853 [details] [diff] [review]
> Disable test_text on android since it causes crashes

More specifically, it somehow triggers JM on arv6 to generate code that's complete broken.
Comment 341 Geoff Brown [:gbrown] 2012-09-28 13:34:50 PDT
As we saw on the earlier landing (comment 258) there was a significant Android Talos regression:

Regression Robocop Checkerboarding Benchmark increase 3.03e+03% on Android 2.2 (Native) Mozilla-Inbound
----------------------------------------------------------------------------------------------------------
    Previous: avg 112.814 stddev 44.673 of 30 runs up to revision 9f476b4ac1e1
    New     : avg 3533.110 stddev 246.564 of 5 runs since revision 6b58397ad735
    Change  : +3420.296 (3.03e+03% / z=76.562)
    Graph   : http://mzl.la/QKmFd5

This time, trobopan does not seem to be affected.
Comment 342 Ed Morley [:emorley] 2012-09-28 16:18:10 PDT
https://hg.mozilla.org/mozilla-central/rev/9d205a349c6a
https://hg.mozilla.org/mozilla-central/rev/1556064f1c20
https://hg.mozilla.org/mozilla-central/rev/aa4652f24df9
https://hg.mozilla.org/mozilla-central/rev/e5651c513f3f
https://hg.mozilla.org/mozilla-central/rev/9c8f66d8eee4
https://hg.mozilla.org/mozilla-central/rev/04df652e5847
https://hg.mozilla.org/mozilla-central/rev/6cabf68e297b
https://hg.mozilla.org/mozilla-central/rev/93686f8add79
https://hg.mozilla.org/mozilla-central/rev/0d3c93bc5716
https://hg.mozilla.org/mozilla-central/rev/673eac4d9c55
https://hg.mozilla.org/mozilla-central/rev/cf82755175d0
https://hg.mozilla.org/mozilla-central/rev/25af0febf329
https://hg.mozilla.org/mozilla-central/rev/4372c4914958
https://hg.mozilla.org/mozilla-central/rev/b5559af3cf94
https://hg.mozilla.org/mozilla-central/rev/8d12f7ceea13
https://hg.mozilla.org/mozilla-central/rev/61672a211aac
https://hg.mozilla.org/mozilla-central/rev/ec48fbf9dd69
https://hg.mozilla.org/mozilla-central/rev/1c3434a2ef97
https://hg.mozilla.org/mozilla-central/rev/967b1ef7afdc
https://hg.mozilla.org/mozilla-central/rev/ba21437b7113
https://hg.mozilla.org/mozilla-central/rev/8fa21e95b22d
https://hg.mozilla.org/mozilla-central/rev/b55adbbe23bc
https://hg.mozilla.org/mozilla-central/rev/5947680feefb
https://hg.mozilla.org/mozilla-central/rev/b3c0862a729a
https://hg.mozilla.org/mozilla-central/rev/937aaddb295c
https://hg.mozilla.org/mozilla-central/rev/035ed3e2d9d4
https://hg.mozilla.org/mozilla-central/rev/e98e2d0aef4d
https://hg.mozilla.org/mozilla-central/rev/96752e66aaab
https://hg.mozilla.org/mozilla-central/rev/23f1b8c7d17e
https://hg.mozilla.org/mozilla-central/rev/07914b108f91
https://hg.mozilla.org/mozilla-central/rev/385d57149939
Comment 343 Ed Morley [:emorley] 2012-09-28 16:27:02 PDT
Believe the [leave open] was not intended; closing.
Comment 344 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-09-28 17:18:25 PDT
(In reply to Geoff Brown [:gbrown] from comment #341)
> As we saw on the earlier landing (comment 258) there was a significant
> Android Talos regression:
> 
> Regression Robocop Checkerboarding Benchmark increase 3.03e+03% on Android
> 2.2 (Native) Mozilla-Inbound
> -----------------------------------------------------------------------------
> -----------------------------
>     Previous: avg 112.814 stddev 44.673 of 30 runs up to revision
> 9f476b4ac1e1
>     New     : avg 3533.110 stddev 246.564 of 5 runs since revision
> 6b58397ad735
>     Change  : +3420.296 (3.03e+03% / z=76.562)
>     Graph   : http://mzl.la/QKmFd5
> 
> This time, trobopan does not seem to be affected.

Right, so we decided to land anyway because I couldn't reproduce this regression in browser.

When scrolling timecube.com (with paint flashing), I only get invalidations for the scrolled in area and there is little to no checkerboarding (even in a debug build).

After the last landing (with a similar regression), there was obvious invalidation of the entire page when scrolling.

My conclusion was that the test suite is doing something slightly different to regular browsing (I tried various zoom levels) and is doing similar excessive invalidation.

If anyone on the mobile team has time to look into this, I'd really appreciate it. I expect there will be a fairly large to-do list after a landing of this magnitude, so I'm going to be fairly busy.
Comment 345 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-09-29 14:59:24 PDT
Nightly is totally unusable atm (Bug 795591). Could you please backout.
Comment 346 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-09-30 04:11:16 PDT
*** Bug 119311 has been marked as a duplicate of this bug. ***
Comment 347 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-09-30 04:17:31 PDT
I had to give up on the blocked bugs because I'm too tired.  We should re-evaluate them tomorrow.
Comment 348 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-09-30 04:53:57 PDT
Nightly updates were stopped for a while.  Make sure yours was built from https://hg.mozilla.org/mozilla-central/rev/a680fd777c3b or later.
Comment 349 Tom Levels 2012-10-03 00:41:42 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #290)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #287)
> > Sorry, the textframe being added is the timer changing.
> > 
> > Over-invalidation in the maze area looks like a real bug, working on it.
> 
> It was, we were invalidating two rects, one with scaling applied and one
> without. And then extending the two rects out into a single rect that
> covered both. :(
> 
> Fixed now
You say the over-invalidation should be fixed, but I still see it in the latest nightly? Seems the 20x20 is worst, the 40x40 is already much better.
Comment 350 Dennis Jakobsen 2012-10-09 06:51:41 PDT
First off, Great work Matt!

Still alot points to parents or parents parents being invalidated.

1. Selecting few chars in a text invalidates the entire container (also on typing)
2. Opening a new tab invalidates the entire strip on each frame of the animation.
3. Firefox UI, mouse in/out focus on firefox menus invalidates the whole dropdown.

And confirming maze solver behaviour Tom mentioned in comment 349
Comment 351 Ryan VanderMeulen [:RyanVM] 2012-10-09 07:10:58 PDT
Thanks for the report, Dennis. However, this bug being fixed and having 350 comments is not really the place to be posting follow-up information or bugs with what's landed. It muddies the water quickly. Assuming the issues you pointed out are not already filed (check the dependencies at the top here), please file them individually as new bugs and mark them as blocking this bug so that they can get the proper focus and attention they need. Thanks!
Comment 352 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-10-25 18:34:28 PDT
Are we making sure we get all the regression fixes to Aurora too?
Comment 353 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-10-27 04:57:54 PDT
Adding tracking-firefox18 so that we don't forget to check all the regressions.
Comment 354 Alex Keybl [:akeybl] 2012-11-02 15:45:14 PDT
(In reply to Olli Pettay [:smaug] from comment #353)
> Adding tracking-firefox18 so that we don't forget to check all the
> regressions.

Let's schedule a meeting to go through the regressions and track those that are critical, rather than tracking this meta bug. Please let bajaj know if you'd like to be included. The default meeting list will be matt, roc, me, and bajaj, and will happen before the next merge.
Comment 355 Andrei Boros 2013-12-08 02:42:59 PST
Is there a possible relation with this?
https://bugzilla.mozilla.org/show_bug.cgi?id=834629#c4

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