Last Comment Bug 526394 - Move scrolling out of the view system into layout
: Move scrolling out of the view system into layout
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla1.9.3a1
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
:
Mentors:
: 550703 (view as bug list)
Depends on: 718353 752608 795358 539226 539342 539344 539348 539949 540161 540183 540247 540294 541144 542115 542200 542677 542900 543681 544085 545049 546033 548792 551463 552436 559725 559993 590082 595060 607464 629587 729968 754520 1092626 1260016
Blocks: 481131 447749 524436 530387 535994
  Show dependency treegraph
 
Reported: 2009-11-03 20:29 PST by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2016-06-06 03:16 PDT (History)
24 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: make a couple of tests more descriptive when they fail (5.93 KB, patch)
2009-11-03 20:38 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 2: remove unnecessary code from nsTextControlFrame (1.70 KB, patch)
2009-11-03 20:40 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 3: Add all necessary APIs to nsIScrollableFrame (30.60 KB, patch)
2009-11-03 20:44 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
dbaron: superreview+
Details | Diff | Splinter Review
Part 4: add nsIFrame::GetScrollTargetFrame (9.58 KB, patch)
2009-11-03 20:46 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
dbaron: superreview+
Details | Diff | Splinter Review
Part 5: Convert some scroll API users to the new API (34.10 KB, patch)
2009-11-03 20:59 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 6: Convert nsEventStateManager to new APIs (27.57 KB, patch)
2009-11-03 21:01 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 7: Hoist GetHtmlContent/GetBodyContent to nsIDocument (4.99 KB, patch)
2009-11-03 21:03 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
jst: review+
Details | Diff | Splinter Review
Part 8: Fix nsNSElementTearoff to use new APIs (16.92 KB, patch)
2009-11-03 21:09 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
jst: review+
Details | Diff | Splinter Review
Part 9: More conversion on nsEventStateManager and nsImageDocument (13.77 KB, patch)
2009-11-03 21:11 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
bugs: review+
Details | Diff | Splinter Review
Part 10: Convert accessibility code (6.13 KB, patch)
2009-11-03 21:13 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 11: Convert nsGlobalWindow and nsDOMWindowUtils (13.46 KB, patch)
2009-11-03 21:15 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 12: Fix XUL layout code: scrollboxes and trees (16.85 KB, patch)
2009-11-03 21:16 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 13: Convert nsCanvasFrame (4.61 KB, patch)
2009-11-03 21:17 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 14: Convert nsListBoxBodyFrame::GetAvailableHeight (1.60 KB, patch)
2009-11-03 21:18 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 15: Convert various chunks of layout code (11.62 KB, patch)
2009-11-03 21:19 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 16: Convert nsListControlFrame (6.81 KB, patch)
2009-11-03 21:20 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 17: Fix selection code in nsTextControlFrame and some parts of nsSelection (26.92 KB, patch)
2009-11-03 21:22 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 18: remove nsIScrollableViewProvider (23.95 KB, patch)
2009-11-03 21:24 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 19: Remove view parameters from nsIScrollPositionListener (15.90 KB, patch)
2009-11-03 21:25 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 20: Convert code in caret and docshell (16.76 KB, patch)
2009-11-03 21:26 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 21: Remove nsIViewManager RootScrollableView APIs, and move GetRectVisibility to nsIPresShell (33.65 KB, patch)
2009-11-03 21:29 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 22: Remove more view stuff from PresShell (16.05 KB, patch)
2009-11-03 21:31 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 23: fix nsRect::IntersectRect (1.12 KB, patch)
2009-11-03 21:33 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
dbaron: superreview+
Details | Diff | Splinter Review
Part 24: Create nsCaret::GetGeometry to obsolete nsCaret::GetCaretCoordinates (4.00 KB, patch)
2009-11-03 21:40 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 25: Create nsIPresShell::ScrollFrameRectIntoView (15.34 KB, patch)
2009-11-03 21:41 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 26: Rework nsSelection to use frames only (38.86 KB, patch)
2009-11-03 21:44 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 27: Rewrite nsMenuPopupFrame::EnsureMenuItemIsVisible to use ScrollFrameRectIntoView (1.85 KB, patch)
2009-11-03 21:46 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 28: Remove nsIScrollableFrame::GetScrollableView (2.72 KB, patch)
2009-11-04 01:00 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 29: Create new nsLayoutUtils methods and remove useless code from nsListControlFrame::CaptureMouseEvents (5.40 KB, patch)
2009-11-04 02:47 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 30: make scroll state in nsPresState a point (5.42 KB, patch)
2009-11-04 02:49 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 31: Move scroll implementation into nsGfxScrollFrame (126.55 KB, patch)
2009-11-04 02:53 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 32: Remove code that supports scrollframes having views (12.79 KB, patch)
2009-11-04 02:56 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 33: Remove nsScrollPortView and associated code (53.42 KB, patch)
2009-11-04 03:01 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 6 v2 (26.86 KB, patch)
2009-11-04 03:10 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
bugs: review+
Details | Diff | Splinter Review
Part 31.5: Cleanup ScrolledAreaEvent stuff (11.65 KB, patch)
2009-11-04 20:20 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 32 v2 (14.88 KB, patch)
2009-11-04 20:21 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 31 v2 (129.47 KB, patch)
2009-11-23 20:00 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Testcase #1 (1.23 KB, text/html)
2009-11-30 20:56 PST, Mats Palmgren (:mats)
no flags Details
Part 0: Remove Composite() call (637 bytes, patch)
2009-12-05 02:18 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
diff -r qparent (546.02 KB, patch)
2009-12-05 02:22 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 34: Create AddScriptBlockerAndPreventAddingRunners (4.28 KB, patch)
2009-12-21 19:49 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
jonas: review+
Details | Diff | Splinter Review
Part 35: Block script execution and event dispatch during UpdateViewAfterScroll (6.36 KB, patch)
2009-12-21 19:55 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 35 v2 (4.57 KB, patch)
2009-12-22 01:48 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review
Part 36: Fix some test failures (3.77 KB, patch)
2009-12-23 18:01 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 20:29:34 PST
I have a very large pile of patches that do this. They pass tests on the tryservers and seem to work well. The basic idea is that all the scrolling API moves out of nsIScrollableView into nsIScrollableFrame. All the functionality of nsScrollPortView moves into nsGfxScrollFrame.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 20:38:49 PST
Created attachment 410123 [details] [diff] [review]
Part 1: make a couple of tests more descriptive when they fail

I don't think this needs review.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 20:40:21 PST
Created attachment 410124 [details] [diff] [review]
Part 2: remove unnecessary code from nsTextControlFrame
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 20:44:56 PST
Created attachment 410127 [details] [diff] [review]
Part 3: Add all necessary APIs to nsIScrollableFrame

This adds every API that we need from nsIScrollableView to nsIScrollableFrame, and implements it in the existing infrastructure. This needs careful review, I hope you like the API!
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 20:46:46 PST
Created attachment 410128 [details] [diff] [review]
Part 4: add nsIFrame::GetScrollTargetFrame

This obsoletes nsIScrollableViewProvider (which I'll remove later in the patch series).
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 20:59:39 PST
Created attachment 410131 [details] [diff] [review]
Part 5: Convert some scroll API users to the new API

Switch to new APIs in PresShell, nsLayoutUtils, and nsSelection (but not completely, there's more to do in later patches)
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:01:02 PST
Created attachment 410132 [details] [diff] [review]
Part 6: Convert nsEventStateManager to new APIs

-- Use nsIScrollableFrame::ScrollUnit instead of nsEventStateManager::ScrollQuantity
-- Remove #if NON_KEYBINDING code
-- Update to new APIs
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:03:31 PST
Created attachment 410134 [details] [diff] [review]
Part 7: Hoist GetHtmlContent/GetBodyContent to nsIDocument

I need this to fix some bogus "am I the root element or BODY" code in nsNSElementTearoff in the next patch.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:09:22 PST
Created attachment 410136 [details] [diff] [review]
Part 8: Fix nsNSElementTearoff to use new APIs

Also fixes things so that we check specifically for "the root element" and "the body element" instead of doing something weird for any element that happens to have tag name "html" or "body".
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:11:01 PST
Created attachment 410137 [details] [diff] [review]
Part 9: More conversion on nsEventStateManager and nsImageDocument
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:13:11 PST
Created attachment 410138 [details] [diff] [review]
Part 10: Convert accessibility code

Asking Mats for review since this is all about layout code really.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:15:03 PST
Created attachment 410139 [details] [diff] [review]
Part 11: Convert nsGlobalWindow and nsDOMWindowUtils

Again, requesting review from Mats because this is basically simple layout-related changes.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:16:19 PST
Created attachment 410140 [details] [diff] [review]
Part 12: Fix XUL layout code: scrollboxes and trees
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:17:25 PST
Created attachment 410141 [details] [diff] [review]
Part 13: Convert nsCanvasFrame
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:18:20 PST
Created attachment 410142 [details] [diff] [review]
Part 14: Convert nsListBoxBodyFrame::GetAvailableHeight
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:19:25 PST
Created attachment 410143 [details] [diff] [review]
Part 15: Convert various chunks of layout code
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:20:41 PST
Created attachment 410144 [details] [diff] [review]
Part 16: Convert nsListControlFrame
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:22:52 PST
Created attachment 410145 [details] [diff] [review]
Part 17: Fix selection code in nsTextControlFrame and some parts of nsSelection

A lot more to come in nsSelection later in the patch series --- it's a real view-fest.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:24:21 PST
Created attachment 410146 [details] [diff] [review]
Part 18: remove nsIScrollableViewProvider

Now we can finally do something fun. We've removed all users of nsIScrollableViewProvider, so now we can kill it.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:25:47 PST
Created attachment 410147 [details] [diff] [review]
Part 19: Remove view parameters from nsIScrollPositionListener
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:26:46 PST
Created attachment 410148 [details] [diff] [review]
Part 20: Convert code in caret and docshell
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:29:35 PST
Created attachment 410149 [details] [diff] [review]
Part 21: Remove nsIViewManager RootScrollableView APIs, and move GetRectVisibility to nsIPresShell

No-one calls nsIViewManager::GetRootScrollableView anymore.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:31:25 PST
Created attachment 410150 [details] [diff] [review]
Part 22: Remove more view stuff from PresShell
Comment 23 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:33:27 PST
Created attachment 410151 [details] [diff] [review]
Part 23: fix nsRect::IntersectRect

Make nsRect::IntersectRect preserve height and width if possible even when the intersection of two rectangles is empty. We need this for some later patches that scroll selection into view and need to do meaningful things with zero-width rects.

Asking Mats for review since nsRect is really layout...
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:40:09 PST
Created attachment 410152 [details] [diff] [review]
Part 24: Create nsCaret::GetGeometry to obsolete nsCaret::GetCaretCoordinates

GetCaretCoordinates is grotesque and tied to views.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:41:24 PST
Created attachment 410153 [details] [diff] [review]
Part 25: Create nsIPresShell::ScrollFrameRectIntoView

We need this to reimplement scrolling of selections into view, without using scrollviews.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:44:16 PST
Created attachment 410154 [details] [diff] [review]
Part 26: Rework nsSelection to use frames only
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-03 21:46:35 PST
Created attachment 410155 [details] [diff] [review]
Part 27: Rewrite nsMenuPopupFrame::EnsureMenuItemIsVisible to use ScrollFrameRectIntoView
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 01:00:45 PST
Created attachment 410171 [details] [diff] [review]
Part 28: Remove nsIScrollableFrame::GetScrollableView

At this point no-one outside of nsGfxScrollFrame uses nsIScrollableView anymore, so we can remove access to it.
Comment 29 Olli Pettay [:smaug] (reviewing overload) 2009-11-04 02:01:51 PST
Comment on attachment 410132 [details] [diff] [review]
Part 6: Convert nsEventStateManager to new APIs

> static PRBool
>-CanScrollOn(nsIScrollableView* aScrollView, PRInt32 aNumLines,
>+CanScrollInRange(nscoord aMin, nscoord aValue, nscoord aMax, PRInt32 aDirection)
>+{
>+  return aDirection > 0 ? aValue < aMax : aMin < aValue;
>+}
>+
>+static PRBool
>+CanScrollOn(nsIScrollableFrame* aScrollFrame, PRInt32 aNumLines,
>             PRBool aScrollHorizontal)
> {
>-  NS_PRECONDITION(aScrollView, "aScrollView is null");
>+  NS_PRECONDITION(aScrollFrame, "aScrollFrame is null");
>   NS_PRECONDITION(aNumLines, "aNumLines must be non-zero");
>-  PRBool canScroll;
>-  nsresult rv =
>-    aScrollView->CanScroll(aScrollHorizontal, aNumLines > 0, canScroll);
>-  return NS_SUCCEEDED(rv) && canScroll;
>+  nsPoint scrollPt = aScrollFrame->GetScrollPosition();
>+  nsRect scrollRange = aScrollFrame->GetScrollRange();
>+
>+  return aScrollHorizontal
>+    ? CanScrollInRange(scrollRange.x, scrollPt.x, scrollRange.XMost(), aNumLines)
>+    : CanScrollInRange(scrollRange.y, scrollPt.y, scrollRange.YMost(), aNumLines);
> }

Would it make sense to move this to nsIScrollableFrame.
I could have method
PRBool CanScroll(PRBool aHorizontal, PRBool aForward)
So, basically the same what nsIScrollableView has now.


>-  if (!passToParent && scrollView) {
>-    if (aScrollQuantity == eScrollByLine) {
>+  if (!passToParent && frameToScroll) {
>+    if (aScrollQuantity == nsIScrollableFrame::LINES) {
>       numLines =
>         nsMouseWheelTransaction::AccelerateWheelDelta(numLines, isHorizontal,
>                                                       aAllowScrollSpeedOverride,
>                                                       &aScrollQuantity);
>+      // Limit scrolling to be at most one page, but if possible, try to
>+      // just adjust the number of scrolled lines.
>+      nscoord lineHeight = frameToScroll->GetLineScrollAmount().height;
>+      if (lineHeight) {
>+        nsSize pageScrollAmount = frameToScroll->GetPageScrollAmount();
>+        nscoord pageScroll = isHorizontal ?
>+          pageScrollAmount.width : pageScrollAmount.height;
>+
>+        if (PR_ABS(numLines) * lineHeight > pageScroll) {
>+          nscoord maxLines = (pageScroll / lineHeight);
>+          if (maxLines >= 1) {
>+            numLines = ((numLines < 0) ? -1 : 1) * maxLines;
>+          } else {
>+            aScrollQuantity = nsIScrollableFrame::PAGES;
>+          }
>+        }
>+      }
I don't understand this change. AccelerateWheelDelta calls LimitToOnePageScroll. Why do
we still need to limit scrolling to one page here?
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 02:47:40 PST
Created attachment 410186 [details] [diff] [review]
Part 29: Create new nsLayoutUtils methods and remove useless code from nsListControlFrame::CaptureMouseEvents

Creates nsLayoutUtils::IsPopup and nsLayoutUtils::GetDisplayRootFrame.
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 02:49:04 PST
Created attachment 410188 [details] [diff] [review]
Part 30: make scroll state in nsPresState a point

The height and width components of the rect are never used.
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 02:53:41 PST
Created attachment 410191 [details] [diff] [review]
Part 31: Move scroll implementation into nsGfxScrollFrame

This is the big patch. Basically, most of the code from nsScrollPortView is migrated to nsGfxScrollFrameInner. Since we don't have an anonymous inner scrollport view anymore, the scrollport bounds are stored in mScrollPort. We reimplement the nsIScrollableFrame methods directly. The merge simplifies many things; for example, nsGfxScrollFrameInner no longer needs to be an nsIScrollPositionListener.
Comment 33 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 02:56:27 PST
Created attachment 410192 [details] [diff] [review]
Part 32: Remove code that supports scrollframes having views

We no longer need nsIView::SetInvalidateFrameOnScroll etc, scrollframes are checking for transformed ancestors directly.

nsCSSFrameConstructor::FinishBuildingScrollFrame no longer needs to construct a view for the scrolled frame.

nsIFrame::GetParentViewForChildFrame is no longer needed.

View construction is simplified because there is no longer any need to call SetScrolledView.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 03:01:24 PST
Created attachment 410197 [details] [diff] [review]
Part 33: Remove nsScrollPortView and associated code
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 03:08:06 PST
(In reply to comment #29)
> Would it make sense to move this to nsIScrollableFrame.
> I could have method
> PRBool CanScroll(PRBool aHorizontal, PRBool aForward)
> So, basically the same what nsIScrollableView has now.

We only need it here. I'd rather keep the interface minimal.

> I don't understand this change. AccelerateWheelDelta calls
> LimitToOnePageScroll. Why do
> we still need to limit scrolling to one page here?

That's basically just a mismerge. I'll delete this code.
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 03:10:15 PST
Created attachment 410200 [details] [diff] [review]
Part 6 v2
Comment 37 Olli Pettay [:smaug] (reviewing overload) 2009-11-04 03:12:38 PST
Comment on attachment 410200 [details] [diff] [review]
Part 6 v2

In my mind the CanScroll clearly belongs to nsIScrollableFrame.
But ok.
Comment 38 Boris Zbarsky [:bz] (still a bit busy) 2009-11-04 16:50:16 PST
Two issues I found so far:

1)  nsContainerFrame::FrameNeedsView forces views for scrolledframes.  We should
    nix that.
2)  A crash on restoring from bfcache due to this code in
    nsHTMLScrollFrame::PostScrolledAreaEventForCurrentArea

880       nsRect currentScrolledArea = mInner.mScrolledFrame->GetView()->GetBounds();

once issue #1 is fixed (because there is no view).
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 20:20:22 PST
Created attachment 410435 [details] [diff] [review]
Part 31.5: Cleanup ScrolledAreaEvent stuff

We can use a simpler event object. We don't need to store the scrolled area, since we can just compute it when we fire the event. Computing it without using the view avoids the crash Boris mentioned. (That code was added after I first converted nsGfxScrollFrame.)
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 20:21:34 PST
Created attachment 410436 [details] [diff] [review]
Part 32 v2

Update part 32 so we don't create views for scrolled frames, as Boris mentioned.
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2009-11-04 20:45:40 PST
Frames with transforms don't need views either anymore?
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-04 23:03:19 PST
Correct, as far as I know. They used to have views so we could set the "scrolling descendants can't bitblit" bit, but that's not needed anymore --- nsGfxScrollFrame just checks the ancestor frames explicitly.
Comment 43 Mats Palmgren (:mats) 2009-11-17 18:02:03 PST
Will have some review comments soon; meanwhile you might want to have a look
at these (local Linux x86-64 debug build, in case it matters):

Mochitest content/html/document/test/test_bug512367.html triggers:
###!!! ASSERTION: curPos.x not a multiple of device pixels: 'curPosDevPx.x*appUnitsPerDevPixel == curPos.x', nsGfxScrollFrame.cpp, line 1717
###!!! ASSERTION: curPos.y not a multiple of device pixels: 'curPosDevPx.y*appUnitsPerDevPixel == curPos.y', nsGfxScrollFrame.cpp, line 1719

Mochitest content/base/test/test_bug340571.html (and many others) triggers:
###!!! ASSERTION: Scrolled rect smaller than scrollport?: 'result.height >= mScrollPort.height', nsGfxScrollFrame.cpp, line 2989
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-17 20:04:22 PST
(In reply to comment #43)
> Mochitest content/html/document/test/test_bug512367.html triggers:
> ###!!! ASSERTION: curPos.x not a multiple of device pixels:
> 'curPosDevPx.x*appUnitsPerDevPixel == curPos.x', nsGfxScrollFrame.cpp, line
> 1717
> ###!!! ASSERTION: curPos.y not a multiple of device pixels:
> 'curPosDevPx.y*appUnitsPerDevPixel == curPos.y', nsGfxScrollFrame.cpp, line
> 1719

Normally we maintain the invariant that the scroll position is a multiple of device pixels. Here that invariant's breaking because we've scrolled a bit, then we change the zoom factor so the current scroll position is no longer a multiple of device pixels.

What we probably need to do is to fix the scroll position during the reflow triggered by zooming.
Comment 45 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-23 20:00:05 PST
Created attachment 414183 [details] [diff] [review]
Part 31 v2

Fix those two assertions:
-- Make nsHTMLScrollFrame::Reflow snap the current scroll position to device pixels in case the reflow is due to a change in zoom factor
-- GetScrolledRect gets called in some situations where the overflow area of the scrolled frame is not set correctly. In those situations, call GetScrolledRectInternal passing in what the overflow area is going to be.
Comment 46 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-11-29 17:34:16 PST
Comment on attachment 410127 [details] [diff] [review]
Part 3: Add all necessary APIs to nsIScrollableFrame

>-/*
>- * interface for rendering objects that wrap rendering objects that should
>- * be scrollable
>- */

Those are useful; they show up in LXR.  Could you leave it, or fix it
appropriately?  See
http://mxr.mozilla.org/mozilla-central/source/layout/generic/

>+   * @param aOverflow if non-null, returns the amount that scrolling
>+   * was clamped by in each direction. This is never negative. This
>+   * is only supported for LINES and DEVICE_PIXELS. The values are in
>+   * device pixels.

Could you define "clamped" better?  And what units is aOverflow in?
aUnit or always device pixels?  (And why doesn't it work with PAGES?)

>+   * XXX should we atke an aMode parameter here? Currently it's instant.

take

I don't quite get the pattern of when you're putting a blank line before
new doc comments in nsIScrollableFrame.h and when you're not, though
perhaps there's a reason.  If not, maybe a blank line before each?

sr=dbaron
Comment 47 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-11-29 17:38:07 PST
Comment on attachment 410128 [details] [diff] [review]
Part 4: add nsIFrame::GetScrollTargetFrame

>+// XXXroc this is megalame. Fossicking around for a frame of the right

Choosing words that are in the dictionary might make things a little more 
approachable to non-native speakers. :-)

rev nsIFrame's IID, and sr=dbaron
Comment 48 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-11-29 17:39:16 PST
(In reply to comment #47)
> (From update of attachment 410128 [details] [diff] [review])
> >+// XXXroc this is megalame. Fossicking around for a frame of the right
> 
> Choosing words that are in the dictionary might make things a little more 
> approachable to non-native speakers. :-)

But I guess that comment was copied from existing code, too.

I'm assuming that existing code gets removed in one of the other 30 patches?
Comment 49 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-11-29 18:10:13 PST
(In reply to comment #48)
> I'm assuming that existing code gets removed in one of the other 30 patches?

Yes, part 18.

(In reply to comment #47)
> rev nsIFrame's IID

Will do.

NS_IFRAME_IID is actually unused now, I guess since the switch to nsQueryFrame. So I'll just remove it.

(In reply to comment #46)
> (From update of attachment 410127 [details] [diff] [review])
> >-/*
> >- * interface for rendering objects that wrap rendering objects that should
> >- * be scrollable
> >- */
> 
> Those are useful; they show up in LXR.  Could you leave it, or fix it
> appropriately?

Fascinating. Will fix.
 
> >+   * @param aOverflow if non-null, returns the amount that scrolling
> >+   * was clamped by in each direction. This is never negative. This
> >+   * is only supported for LINES and DEVICE_PIXELS. The values are in
> >+   * device pixels.
> 
> Could you define "clamped" better?

Yes.

> And what units is aOverflow in?

Device pixels, as it says.

> (And why doesn't it work with PAGES?)

Because nsScrollPortView::ScrollByPages doesn't support it. Part 31 removes this limitation.

> I don't quite get the pattern of when you're putting a blank line before
> new doc comments in nsIScrollableFrame.h and when you're not, though
> perhaps there's a reason.

I was trying to group together related members.
Comment 50 Mats Palmgren (:mats) 2009-11-30 20:56:43 PST
Created attachment 415332 [details]
Testcase #1

This testcase doesn't work in my tree with the patch set applied.
(my review is almost complete, comments in bit...)
Comment 51 Mats Palmgren (:mats) 2009-11-30 23:01:05 PST
Comment on attachment 410151 [details] [diff] [review]
Part 23: fix nsRect::IntersectRect

part 23:
This patch looks a bit scary.  David, can you have a look too please?

This comment about width/height is incorrect:
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.cpp#1067
is the code still correct?

Maybe early return here if dirty.IsEmpty():
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#2020
Comment 52 Mats Palmgren (:mats) 2009-11-30 23:03:57 PST
part 1: ok
part 2: ok
part 3: 
> nsIScrollableFrame.h:
+   * XXX should we atke an aMode parameter here? Currently it's instant.

typo

part 4:
> nsComboboxControlFrame.h:
+  virtual nsIScrollableFrame* GetScrollTargetFrame() {
+    nsIScrollableFrame* scrollable = do_QueryFrame(mDropdownFrame);
+    return scrollable;
+  }

"return do_QueryFrame(mDropdownFrame);" is shorter.

part 5:
> nsLayoutUtils.cpp:
+      // If scrolling in a specific direction, require visible scrollbars or
+      // something to scroll to in that direction.

Suggest "Require visible scrollbars or something to scroll to in
the given direction." now that "eEither" is handled separately.

> nsLayoutUtils.h:
+   * GetNearestScrollableFrame locates the first ancestor of aFrame
+   * (or aFrame itself) that is scrollable and overflow:scroll or

s/and/with/

> nsPresContext.h:
+    PRBool operator==(const ScrollbarStyles& aStyles) {
+    PRBool operator!=(const ScrollbarStyles& aStyles) {

Can be const methods?

> nsPresShell.cpp
   // Utility to find which view to scroll.
-  nsIScrollableView* GetViewToScroll(nsLayoutUtils::Direction aDirection);
+  nsIScrollableFrame* GetFrameToScroll(nsLayoutUtils::Direction aDirection);

Fix/remove the comment.

+PresShell::GetFrameToScroll

There's a subtle change here compared to the old GetViewToScroll.
In the "focusedContent case", the old code looks for a scrollable view
on the primary frame and calls GetNearestScrollingView(... aDirection),
which only searches in aDirection.  The new code calls
GetScrollTargetFrame() (which is not limited to aDirection) and returns
that if non-null.  Is this change intentional?

+          nscoord scrollViewLineHeight =

s/scrollViewLineHeight/scrollFrameLineHeight/

part 10: ok
part 11: ok

part 12:
> nsTreeBodyFrame.cpp
nsGfxScrollFrameInner::ScrollToImpl does PostScrollEvent(), so I don't
think we need that in nsTreeBodyFrame::ScrollHorzInternal.

part 13: ok
part 14: ok
part 15: ok
part 16: ok
part 17:
> nsTextControlFrame.cpp
In nsTextInputSelectionImpl::nsTextInputSelectionImpl():
+    mScrollFrame = nsnull;

Please change this to an (unconditional) member init.

part 18: ok
part 19: ok
part 20: ok
Part 21: 
> nsAccessible.cpp
+    if (isEmpty && frame && !(frame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)) {

No need to null-check 'frame' there.

+PresShell::GetRectVisibility

Why change '<' to '<=' for the "kAboveViewport" test (etc)?

Part 22:
> nsAccessible.cpp
-  PRBool isVisible = CheckVisibilityInParentChain(doc, frame->GetClosestView());
+  nsIView* view = frame->GetAncestorWithViewExternal()->GetViewExternal();

GetClosestView() checks 'this', GetAncestorWithViewExternal() does not.
Is 'frame' guarranteed to not be the root frame here?

> nsPresShell.cpp
In AccumulateFrameBounds():
   // If this is an inline frame and either the bounds height is 0 (quirks
   // layout model) or aVPercent is not NS_PRESSHELL_SCROLL_ANYWHERE, we need to
   // change the top of the bounds to include the whole line.
-  if (frameBounds.height == 0 || aVPercent != NS_PRESSHELL_SCROLL_ANYWHERE) {
+  if (frameBounds.height == 0 || aUseWholeLineHeightForInlines) {

The comment refers to the removed 'aVPercent'.

part 23: see separate comment above.
part 24: ok
part 25: ok
part 26:
> nsSelection.cpp
In nsAutoScrollTimer::Notify()
-      if (!frame.IsAlive())
-        return NS_OK;

We still need this check - 'frame' is used on the next line.
(with that we can have NS_PRECONDITION(aFrame, "Need a frame")
in DoAutoScroll too)

part 27: ok
part 28: ok
part 29:
> nsLayoutUtils.h

+   * A frame is a popup if it has its own floating window. Menus, panels
+   * and combobox dropdowns are popups.

The comment mentions panels, the code doesn't seem to do that.

part 30: ok
part 31 v2:
> nsGfxScrollFrame.cpp
In nsGfxScrollFrameInner::ScrollBy():
+  ScrollTo(newPos, aMode);
+
   if (aOverflow) {
-    *aOverflow = overflow;
+    nsPoint clampAmount = mDestination - newPos;

ScrollTo may have destroyed 'this' if 'aMode' is INSTANT.

We should update aOverflow also for WHOLE, or say in the header that
it isn't supported for WHOLE (and in that case assert that it's null).

In nsGfxScrollFrameInner::GetPageScrollAmount():
+    mScrollPort.width - PR_MIN(mScrollPort.width/10, ...
+    mScrollPort.height - PR_MIN(mScrollPort.height/10, ...

NS_MIN is better.

In nsGfxScrollFrameInner::ScrollToRestoredPosition()():
+      ScrollTo(mRestorePos, nsIScrollableFrame::INSTANT);
+      // Re-get the scroll position, it might not be exactly equal to
+      // mRestorePos due to rounding and clamping.
+      mLastPos = GetScrollPosition();

ScrollTo may have destroyed 'this'.

> nsGfxScrollFrame.h
+  // Update scrollbar curpos attributes to reflect current scroll
+  // position

This comment fits on one line.

part 32: ok
part 33: ok

No specific part:
nsIPresShell.h  needs to bump NS_IPRESSHELL_IID
nsFrameSelection.h needs to bump NS_FRAME_SELECTION_IID
nsIScrollPositionListener.h needs to bump NS_ISCROLLPOSITIONLISTENER_IID
Maybe nsView.cpp needs to bump VIEW_WRAPPER_IID since we removed
nsIScrollableView in ViewWrapper::GetInterface()?
Maybe nsDocAccessible.h needs to bump NS_DOCACCESSIBLE_IMPL_CID?

Finally, I want to take another pass looking at frame liveness specifically.
Should be done by tomorrow with that.
Comment 53 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2009-12-01 13:30:16 PST
Comment on attachment 410151 [details] [diff] [review]
Part 23: fix nsRect::IntersectRect

sr=dbaron
Comment 54 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-03 01:39:36 PST
(In reply to comment #51)
> This comment about width/height is incorrect:
> http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsMenuPopupFrame.cpp#1067
> is the code still correct?

Perhaps not. I'll fix it to set the width and height to zero there.

> Maybe early return here if dirty.IsEmpty():
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSRendering.cpp#2020

Sure.

(In reply to comment #52)
> typo

Fixed

> "return do_QueryFrame(mDropdownFrame);" is shorter.

OK

> Suggest "Require visible scrollbars or something to scroll to in
> the given direction." now that "eEither" is handled separately.

Good, fixed.

> s/and/with/

Fixed

> Can be const methods?

Yes, fixed

> Fix/remove the comment.

Removed

> +PresShell::GetFrameToScroll
> 
> There's a subtle change here compared to the old GetViewToScroll.
> In the "focusedContent case", the old code looks for a scrollable view
> on the primary frame and calls GetNearestScrollingView(... aDirection),
> which only searches in aDirection.  The new code calls
> GetScrollTargetFrame() (which is not limited to aDirection) and returns
> that if non-null.  Is this change intentional?

No. That's a very good catch. We should start our search at the result of GetScrollTargetFrame (if it's non-null) but always call GetNearestScrollableFrameForDirection whether GetScrollTargetFrame returned null or not.

> s/scrollViewLineHeight/scrollFrameLineHeight/

Fixed

> part 12:
> > nsTreeBodyFrame.cpp
> nsGfxScrollFrameInner::ScrollToImpl does PostScrollEvent(), so I don't
> think we need that in nsTreeBodyFrame::ScrollHorzInternal.

You mean nsGfxScrollFrameInner::ScrollToImpl in aParts.mColumnsScrollFrame? The event doesn't bubble so I don't think that will reach the tree. Anyway, I wouldn't want to rely on that.

> Please change this to an (unconditional) member init.

Fixed

> Part 21: 
> 
> +PresShell::GetRectVisibility
> 
> Why change '<' to '<=' for the "kAboveViewport" test (etc)?

If aMinTwips is 0 and r.YMost() == insetRect.y (which == scrollPortRect.y), then the frame is not visible. Right?

> Part 22:
> > nsAccessible.cpp
> -  PRBool isVisible = CheckVisibilityInParentChain(doc,
> frame->GetClosestView());
> +  nsIView* view = frame->GetAncestorWithViewExternal()->GetViewExternal();
> 
> GetClosestView() checks 'this', GetAncestorWithViewExternal() does not.
> Is 'frame' guarranteed to not be the root frame here?

Good catch, I'll just restore the old behavior of return frame's view if frame->HasView().

> The comment refers to the removed 'aVPercent'.

Fixed.

> We still need this check - 'frame' is used on the next line.
> (with that we can have NS_PRECONDITION(aFrame, "Need a frame")
> in DoAutoScroll too)

Great catch, fixed.

> part 29:
> > nsLayoutUtils.h
> 
> +   * A frame is a popup if it has its own floating window. Menus, panels
> +   * and combobox dropdowns are popups.
> 
> The comment mentions panels, the code doesn't seem to do that.

Panels are just nsMenuPopupFrames.

> part 31 v2:
> > nsGfxScrollFrame.cpp
> In nsGfxScrollFrameInner::ScrollBy():
> +  ScrollTo(newPos, aMode);
> +
>    if (aOverflow) {
> -    *aOverflow = overflow;
> +    nsPoint clampAmount = mDestination - newPos;
> 
> ScrollTo may have destroyed 'this' if 'aMode' is INSTANT.

How would ScrollTo destroy 'this'?

> We should update aOverflow also for WHOLE, or say in the header that
> it isn't supported for WHOLE (and in that case assert that it's null).

I'll just return 0,0 in *aOverflow for WHOLE.

> NS_MIN is better.

Fixed

> In nsGfxScrollFrameInner::ScrollToRestoredPosition()():
> +      ScrollTo(mRestorePos, nsIScrollableFrame::INSTANT);
> +      // Re-get the scroll position, it might not be exactly equal to
> +      // mRestorePos due to rounding and clamping.
> +      mLastPos = GetScrollPosition();
> 
> ScrollTo may have destroyed 'this'.

As above, I don't think ScrollTo can destroy the frame.

> This comment fits on one line.

Fixed

> nsIPresShell.h  needs to bump NS_IPRESSHELL_IID

Part 21 bumps it. But Part 25 should too, so I've done it there as well.

> nsFrameSelection.h needs to bump NS_FRAME_SELECTION_IID

Does it? We don't change any virtual methods.

> nsIScrollPositionListener.h needs to bump NS_ISCROLLPOSITIONLISTENER_IID

Done.

> Maybe nsView.cpp needs to bump VIEW_WRAPPER_IID since we removed
> nsIScrollableView in ViewWrapper::GetInterface()?

Sure, why not :-).

> Maybe nsDocAccessible.h needs to bump NS_DOCACCESSIBLE_IMPL_CID?

I don't think so.

Which parts do you want me to repost?
Comment 55 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-03 02:28:35 PST
The testcase in comment #50 fails because scroll positions are now forced to be multiples of device pixels, but GetScrollRange returns values which aren't rounded to device pixels. So when an element whose height is not an integer number of device pixels is scrolled as far down as possible, the scroll position y is still less than GetScrollRange().YMost() so CanScrollOn returns true.
Comment 56 Mats Palmgren (:mats) 2009-12-03 11:28:39 PST
> >> > > nsTreeBodyFrame.cpp
> > > nsGfxScrollFrameInner::ScrollToImpl does PostScrollEvent(), so I don't
> > > think we need that in nsTreeBodyFrame::ScrollHorzInternal.
> You mean nsGfxScrollFrameInner::ScrollToImpl in aParts.mColumnsScrollFrame?

Yes.

> The event doesn't bubble so I don't think that will reach the tree.

Ok, but I thought 'aParts.mColumnsScrollFrame' and 'this' in 
nsTreeBodyFrame::ScrollHorzInternal() had the same content?
If not, then disregard my comment.

> How would ScrollTo destroy 'this'?

We already had this discussion in bug 435422 ;-)
I still believe ScrollToImpl can destroy the world.
(related: bug 421839, bug 402505, bug 269832)

I think the call to ScrollTo() in ScrollToRestoredPosition() and
ScrollBy() should have a nsWeakFrame check around it.

> > Why change '<' to '<=' for the "kAboveViewport" test (etc)?
> If aMinTwips is 0 and r.YMost() == insetRect.y (which == scrollPortRect.y),
> then the frame is not visible. Right?

Ah, right, so the old code has a bug?

> > nsFrameSelection.h needs to bump NS_FRAME_SELECTION_IID
> Does it? We don't change any virtual methods.

No, you're right.

> Which parts do you want me to repost?

Please attach "hg diff -r qparent" instead and I'll diff that to
the current one.
Comment 57 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-03 13:24:19 PST
(In reply to comment #56)
> > The event doesn't bubble so I don't think that will reach the tree.
> 
> Ok, but I thought 'aParts.mColumnsScrollFrame' and 'this' in 
> nsTreeBodyFrame::ScrollHorzInternal() had the same content?
> If not, then disregard my comment.

Hmm ... I'm not sure without checking, and anyway, we shouldn't rely on such a subtle thing.

> > How would ScrollTo destroy 'this'?
> 
> We already had this discussion in bug 435422 ;-)
> I still believe ScrollToImpl can destroy the world.
> (related: bug 421839, bug 402505, bug 269832)

The summary of bug 435422 is that it happens via "mWidget->Invalidate(PR_TRUE);" in nsPluginInstanceOwner::ScrollPositionDidChange, which is only on Mac right? I think we should just remove that Invalidate call completely. I don't see why it's necessary.

Bug 421839 doesn't show ScrollTo/ScrollToImpl destroying anything. Nor does bug 402505 or bug 269832, as far as I can tell.

I suppose it is possible that on Mac, doing a SetWindow call to the plugin in Will/DidChangeScrollPosition would cause the plugin to do something nasty like trigger JS. But we can't really protect ourselves from that.

> I think the call to ScrollTo() in ScrollToRestoredPosition() and
> ScrollBy() should have a nsWeakFrame check around it.

The problem is that then all the callers to ScrollBy and ScrollToRestoredPosition need to be audited to use weak frames as well. It gets really messy.

> > > Why change '<' to '<=' for the "kAboveViewport" test (etc)?
> > If aMinTwips is 0 and r.YMost() == insetRect.y (which == scrollPortRect.y),
> > then the frame is not visible. Right?
> 
> Ah, right, so the old code has a bug?

I think so.

> > Which parts do you want me to repost?
> 
> Please attach "hg diff -r qparent" instead and I'll diff that to
> the current one.

I'll do that once I've integrated the fix for your failing testcase.
Comment 58 Mats Palmgren (:mats) 2009-12-03 21:18:36 PST
(In reply to comment #57)
> > > How would ScrollTo destroy 'this'?

The stack in bug 490461 shows that there's a path from ScrollToImpl()
to nsFocusManager::SetFocusInner() which calls CheckIfFocusable()
which does "doc->FlushPendingNotifications(Flush_Frames)" which can do
nasty things, right?
Comment 59 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-04 11:56:08 PST
In that stack, Windows is dispatching WM_MOUSEACTIVATE while we call ::UpdateWindow(). That is evil (and undocumented) :-(. Maybe something to do with the Acrobat plugin running out-of-process? Maybe the solution is to rip out the Composite() call and not bother.

I really don't want to carefully make lots of code resistant to flushes during scrolling and then go and make the visual part of scrolling asynchronous so we don't need it anymore.
Comment 60 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-04 13:47:35 PST
I think we should try disabling Composite() and see what happens. If the results aren't too bad, that will do.
Comment 61 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-05 02:18:44 PST
Created attachment 416251 [details] [diff] [review]
Part 0: Remove Composite() call
Comment 62 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-05 02:22:20 PST
Created attachment 416252 [details] [diff] [review]
diff -r qparent

As requested
Comment 63 Mats Palmgren (:mats) 2009-12-07 22:25:52 PST
Comment on attachment 416251 [details] [diff] [review]
Part 0: Remove Composite() call

r=mats
Comment 64 Mats Palmgren (:mats) 2009-12-07 22:28:33 PST
Your updates in attachment 416252 [details] [diff] [review] looks good, r=mats on the remaining parts
where my review was requested.
Comment 65 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-19 01:24:46 PST
I checked in part 0:
http://hg.mozilla.org/mozilla-central/rev/56a6449e4833
Comment 66 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-19 15:01:42 PST
I'm not really comfortable with part 0. But I don't see any other way to avoid the reentrancy issues if UpdateWindow can post arbitrary non-Paint messages to the window, as bug 490461 indicates.
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-20 20:53:36 PST
Part 0 caused regression bug 536065. It seems we're going to have to back it out and just audit a bazillion code paths :-(.
Comment 68 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-21 02:49:24 PST
One problematic path is where nsScrollbarFrame::AttributeChanged calls nsGfxScrollFrameInner::CurPosAttributeChanged which calls nsGfxScrollFrameInner::ScrollTo(INSTANT), which can destroy the world. We can't make AttributeChanged safe for arbitrary script execution, obviously. I suppose we can just assume that the scrollbar mode is always "smooth" so that scrolling happens asynchronously, but that might feel laggy. But I don't see we have much choice.
Comment 69 Olli Pettay [:smaug] (reviewing overload) 2009-12-21 02:59:27 PST
Could you use script runner and not fully asynchronous thread dispatching?
Comment 70 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-21 12:52:29 PST
That's a good idea, I'll look into it. Thanks!
Comment 71 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-21 19:17:14 PST
Making nsIScrollableFrame::ScrollTo/ScrollBy possibly destroy the world is incredibly painful. The amount of code that would have to be modified to handle potential frame destruction is really large and testing it all to make sure it handles frame destruction would be really, really hard. We've got to just not do this.

Here is Mats' problematic stack:
https://bug490461.bugzilla.mozilla.org/attachment.cgi?id=416005

I think it's significant that the only stack we have of this _NtUserCallHwndLock involves a plugin (Adobe Acrobat Reader) which uses HWNDs bound to a non-main thread. Also, the event being dispatched during _NtUserCallHwndLock is WM_MOUSEACTIVATE, which has the behaviour that DefWindowProc dispatches it to the parent window before processing in the child. So I suspect that what happens there is that WM_MOUSEACTIVATE is sent to a non-main-thread window of Acrobat, which sends WM_MOUSEACTIVATE to our plugin window, and our call to UpdateWindow decides to process that event. So I think it should be enough to add a script blocker around the call to UpdateViewAfterScroll and have our PluginWndProc WM_MOUSEACTIVE handler ignore the event if it's not safe to run scripts.
Comment 72 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-21 19:49:51 PST
Created attachment 418773 [details] [diff] [review]
Part 34: Create AddScriptBlockerAndPreventAddingRunners

I want to be able to add a script blocker to signal that it's not safe to run scripts, in a situation where code might test whether it's safe to run scripts but will not (or at least, should not) add any actual script runners. I don't want to just add and remove a normal script blocker because I want to a) assert immediately if someone tries to add a script runner when they shouldn't and b) if we accidentally screw up and add a script runner, I want that script runner to be dropped on the floor rather than executed at a time I know is unsafe. (Point b) is negotiable, you might argue that it's safer to execute the runner than drop it, but I definitely want the instant assertion.)

This patch adds nsContentUtils::AddScriptBlockerAndPreventAddingRunners, which prevents adding any runners until that script blocker is removed.
Comment 73 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-21 19:55:13 PST
Created attachment 418774 [details] [diff] [review]
Part 35: Block script execution and event dispatch during UpdateViewAfterScroll

Adds a script blocker while we call UpdateViewAfterScroll. Makes PluginWndProc WM_MOUSEACTIVATE bail out instead of dispatching an event if it's not safe to run script. Also suppresses FlushDelayedResize in nsViewManager::DispatchEvent NS_PAINT, if we're in the middle of scrolling --- this looks like another potential reentrancy hazard.
Comment 74 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-12-21 22:01:56 PST
Comment on attachment 418773 [details] [diff] [review]
Part 34: Create AddScriptBlockerAndPreventAddingRunners

Wouldn't mind using a guard object here, though maybe that's overkill unless we end up using this in more places.


Though sort of wondering if there's any place up the callstack where it would make sense to add a scriptblocker such that we could run script when we leave that scope?
Comment 75 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-21 22:33:05 PST
(In reply to comment #74)
> Though sort of wondering if there's any place up the callstack where it would
> make sense to add a scriptblocker such that we could run script when we leave
> that scope?

The problem is that there are many different callstacks leading to nsIScrollableFrame::ScrollTo/ScrollBy, along which we hold frame pointers, and they would all need to be independently guarded with script blockers.
Comment 76 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-22 01:48:22 PST
Created attachment 418808 [details] [diff] [review]
Part 35 v2

The first version of this patch didn't build on Windows, because nsPluginNativeWindowWin can't use nsContentUtils. But it turns out we actually don't need to change nsPluginNativeWindowWin. We don't do anything dangerous with WM_MOUSEACTIVATE except here in PluginWndProc where we call DispatchEvent with NS_PLUGIN_ACTIVATE. And that doesn't do anything dangerous before we get to PresShell::HandleEvent, which already bails out early if !IsSafeToRunScript.
Comment 77 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-23 18:01:27 PST
Created attachment 419076 [details] [diff] [review]
Part 36: Fix some test failures

I noticed a couple of test failures when pushing to the try server. We need to fix test_offsets.js to compute the scrolled area using floor, not round, and we also need to make nsFrame::FinishAndStoreOverflow handle empty overflow areas more precisely.
Comment 78 Robert O'Callahan (:roc) (email my personal email if necessary) 2009-12-23 18:02:47 PST
The nsFrame::FinishAndStoreOverflow changes are actually to avoid assertions about the scrolled area ending up smaller than the scrollport.
Comment 79 Mats Palmgren (:mats) 2010-01-10 19:05:56 PST
Comment on attachment 418808 [details] [diff] [review]
Part 35 v2

>+      // Block script execution. This is basically a signal that flushing
>+      // and handling of plugin events on Windows (WM_MOUSEACTIVATE) ...

Fix comment now that you don't change the WM_MOUSEACTIVATE code?

r=mats
Comment 80 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-11 18:44:46 PST
http://hg.mozilla.org/mozilla-central/rev/5546b129745e
http://hg.mozilla.org/mozilla-central/rev/db4c8c32177e
http://hg.mozilla.org/mozilla-central/rev/eb1ae9aabecc
http://hg.mozilla.org/mozilla-central/rev/8136d8a8d52f
http://hg.mozilla.org/mozilla-central/rev/176699b95417
http://hg.mozilla.org/mozilla-central/rev/3d8d388c968b
http://hg.mozilla.org/mozilla-central/rev/7448e55631cf
http://hg.mozilla.org/mozilla-central/rev/8a224eb3aa04
http://hg.mozilla.org/mozilla-central/rev/a66a31bd68a9
http://hg.mozilla.org/mozilla-central/rev/78109c8760ca
http://hg.mozilla.org/mozilla-central/rev/bbb0cb6172e3
http://hg.mozilla.org/mozilla-central/rev/c367c90a3853
http://hg.mozilla.org/mozilla-central/rev/bb8115d4556f
http://hg.mozilla.org/mozilla-central/rev/a83834428689
http://hg.mozilla.org/mozilla-central/rev/927cce54b446
http://hg.mozilla.org/mozilla-central/rev/f1394b021aa0
http://hg.mozilla.org/mozilla-central/rev/7e30f976a6a3
http://hg.mozilla.org/mozilla-central/rev/8c1587eac4b2
http://hg.mozilla.org/mozilla-central/rev/2156590f92b2
http://hg.mozilla.org/mozilla-central/rev/2ec64a146a7f
http://hg.mozilla.org/mozilla-central/rev/822e303a19a0
http://hg.mozilla.org/mozilla-central/rev/fa0d667648ce
http://hg.mozilla.org/mozilla-central/rev/4fb067ad0ae4
http://hg.mozilla.org/mozilla-central/rev/b53b126bd64c
http://hg.mozilla.org/mozilla-central/rev/49d5090912a4
http://hg.mozilla.org/mozilla-central/rev/5d8894904869
http://hg.mozilla.org/mozilla-central/rev/a423f5c3057a
http://hg.mozilla.org/mozilla-central/rev/208d262ef05b
http://hg.mozilla.org/mozilla-central/rev/d00058e431d3
http://hg.mozilla.org/mozilla-central/rev/fada8c5cef07
http://hg.mozilla.org/mozilla-central/rev/4ccff5df452c
http://hg.mozilla.org/mozilla-central/rev/17131f693e28
http://hg.mozilla.org/mozilla-central/rev/4384150589e0
http://hg.mozilla.org/mozilla-central/rev/2f7d76044ee8
http://hg.mozilla.org/mozilla-central/rev/9584d6bce9c1
http://hg.mozilla.org/mozilla-central/rev/8544017aa583
http://hg.mozilla.org/mozilla-central/rev/47cd92d616d7

Thanks Mats!
Comment 81 Ori Avtalion (salty-horse) 2010-02-13 12:25:17 PST
Could this have caused the following bug?

Alt-tabbing until the entire viewport is in "focus" and have a rectangle around it.
Then scroll up/down.
The focus rect's dotted line will smear.

I found this happening beginning with the 2010-01-12 nightly builds.
Previously, the focus rect would immediately disappear after scrolling, so no smearing is possible.
Comment 82 Timothy Nikkel (:tnikkel) 2010-02-13 12:30:34 PST
Yeah, it probably was this bug. Can you file it as a new bug and mark this bug as blocking it? Thanks.
Comment 83 Boris Zbarsky [:bz] (still a bit busy) 2010-03-17 14:29:57 PDT
*** Bug 550703 has been marked as a duplicate of this bug. ***

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