Closed Bug 526394 Opened 15 years ago Closed 14 years ago

Move scrolling out of the view system into layout

Categories

(Core :: Web Painting, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: roc, Assigned: roc)

References

(Depends on 2 open bugs)

Details

Attachments

(40 files, 4 obsolete files)

5.93 KB, patch
Details | Diff | Splinter Review
1.70 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
30.60 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
9.58 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
34.10 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
4.99 KB, patch
jst
: review+
Details | Diff | Splinter Review
16.92 KB, patch
jst
: review+
Details | Diff | Splinter Review
13.77 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.13 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
13.46 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
16.85 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
4.61 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
1.60 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
11.62 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
6.81 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
26.92 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
23.95 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
15.90 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
16.76 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
33.65 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
16.05 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
1.12 KB, patch
MatsPalmgren_bugz
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
4.00 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
15.34 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
38.86 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
1.85 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
2.72 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
5.40 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
5.42 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
53.42 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
26.86 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.65 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
14.88 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
129.47 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
1.23 KB, text/html
Details
637 bytes, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
546.02 KB, patch
Details | Diff | Splinter Review
4.28 KB, patch
sicking
: review+
Details | Diff | Splinter Review
4.57 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
3.77 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
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.
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!
Attachment #410127 - Flags: superreview?(dbaron)
Attachment #410127 - Flags: review?(matspal)
This obsoletes nsIScrollableViewProvider (which I'll remove later in the patch series).
Attachment #410128 - Flags: superreview?(dbaron)
Attachment #410128 - Flags: review?(matspal)
Switch to new APIs in PresShell, nsLayoutUtils, and nsSelection (but not completely, there's more to do in later patches)
Attachment #410131 - Flags: review?(matspal)
-- Use nsIScrollableFrame::ScrollUnit instead of nsEventStateManager::ScrollQuantity
-- Remove #if NON_KEYBINDING code
-- Update to new APIs
Attachment #410132 - Flags: review?(Olli.Pettay)
I need this to fix some bogus "am I the root element or BODY" code in nsNSElementTearoff in the next patch.
Attachment #410134 - Flags: review?(jst)
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".
Attachment #410136 - Flags: review?(jst)
Asking Mats for review since this is all about layout code really.
Attachment #410138 - Flags: review?(matspal)
Again, requesting review from Mats because this is basically simple layout-related changes.
Attachment #410139 - Flags: review?(matspal)
Attachment #410140 - Attachment description: Part 1: Fix XUL layout code: scrollboxes and trees → Part 12: Fix XUL layout code: scrollboxes and trees
A lot more to come in nsSelection later in the patch series --- it's a real view-fest.
Attachment #410145 - Flags: review?(matspal)
Now we can finally do something fun. We've removed all users of nsIScrollableViewProvider, so now we can kill it.
Attachment #410146 - Flags: review?(matspal)
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...
Attachment #410151 - Flags: review?(matspal)
GetCaretCoordinates is grotesque and tied to views.
Attachment #410152 - Flags: review?(matspal)
We need this to reimplement scrolling of selections into view, without using scrollviews.
Attachment #410153 - Flags: review?(matspal)
Attachment #410154 - Flags: review? → review?(matspal)
At this point no-one outside of nsGfxScrollFrame uses nsIScrollableView anymore, so we can remove access to it.
Attachment #410171 - Flags: review?(matspal)
Attachment #410137 - Flags: review?(Olli.Pettay) → review+
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?
Creates nsLayoutUtils::IsPopup and nsLayoutUtils::GetDisplayRootFrame.
Attachment #410186 - Flags: review?(matspal)
The height and width components of the rect are never used.
Attachment #410188 - Flags: review?(matspal)
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.
Attachment #410191 - Flags: review?(matspal)
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.
Attachment #410192 - Flags: review?(matspal)
(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.
Attached patch Part 6 v2Splinter Review
Attachment #410200 - Flags: review?(Olli.Pettay)
Comment on attachment 410200 [details] [diff] [review]
Part 6 v2

In my mind the CanScroll clearly belongs to nsIScrollableFrame.
But ok.
Attachment #410200 - Flags: review?(Olli.Pettay) → review+
Attachment #410128 - Attachment is patch: true
Attachment #410128 - Attachment mime type: application/octet-stream → text/plain
Attachment #410136 - Attachment is patch: true
Attachment #410136 - Attachment mime type: application/octet-stream → text/plain
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).
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.)
Attachment #410435 - Flags: review?(matspal)
Attached patch Part 32 v2Splinter Review
Update part 32 so we don't create views for scrolled frames, as Boris mentioned.
Attachment #410132 - Attachment is obsolete: true
Attachment #410192 - Attachment is obsolete: true
Attachment #410436 - Flags: review?(matspal)
Attachment #410192 - Flags: review?(matspal)
Attachment #410132 - Flags: review?(Olli.Pettay)
Frames with transforms don't need views either anymore?
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.
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
(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.
Attached patch Part 31 v2Splinter Review
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.
Attachment #410191 - Attachment is obsolete: true
Attachment #414183 - Flags: review?(matspal)
Attachment #410191 - Flags: review?(matspal)
Blocks: 481131
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
Attachment #410127 - Flags: superreview?(dbaron) → superreview+
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
Attachment #410128 - Flags: superreview?(dbaron) → superreview+
(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?
(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.
Attached file Testcase #1
This testcase doesn't work in my tree with the patch set applied.
(my review is almost complete, comments in bit...)
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
Attachment #410151 - Flags: superreview?(dbaron)
Attachment #410151 - Flags: review?(matspal)
Attachment #410151 - Flags: review+
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.
Attachment #410151 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 410151 [details] [diff] [review]
Part 23: fix nsRect::IntersectRect

sr=dbaron
(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?
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.
> >> > > 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.
(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.
(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?
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.
I think we should try disabling Composite() and see what happens. If the results aren't too bad, that will do.
Attachment #416251 - Flags: review?(matspal) → review+
Comment on attachment 416251 [details] [diff] [review]
Part 0: Remove Composite() call

r=mats
Your updates in attachment 416252 [details] [diff] [review] looks good, r=mats on the remaining parts
where my review was requested.
Attachment #410124 - Flags: review?(matspal) → review+
Attachment #410127 - Flags: review?(matspal) → review+
Attachment #410131 - Flags: review?(matspal) → review+
Attachment #410138 - Flags: review?(matspal) → review+
Attachment #410139 - Flags: review?(matspal) → review+
Attachment #410140 - Flags: review?(matspal) → review+
Attachment #410141 - Flags: review?(matspal) → review+
Attachment #410142 - Flags: review?(matspal) → review+
Attachment #410143 - Flags: review?(matspal) → review+
Attachment #410144 - Flags: review?(matspal) → review+
Attachment #410145 - Flags: review?(matspal) → review+
Attachment #410146 - Flags: review?(matspal) → review+
Attachment #410147 - Flags: review?(matspal) → review+
Attachment #410148 - Flags: review?(matspal) → review+
Attachment #410149 - Flags: review?(matspal) → review+
Attachment #410150 - Flags: review?(matspal) → review+
Attachment #410128 - Flags: review?(matspal) → review+
Attachment #410152 - Flags: review?(matspal) → review+
Attachment #410153 - Flags: review?(matspal) → review+
Attachment #410154 - Flags: review?(matspal) → review+
Attachment #410155 - Flags: review?(matspal) → review+
Attachment #410171 - Flags: review?(matspal) → review+
Attachment #410186 - Flags: review?(matspal) → review+
Attachment #410188 - Flags: review?(matspal) → review+
Attachment #410197 - Flags: review?(matspal) → review+
Attachment #410435 - Flags: review?(matspal) → review+
Attachment #410436 - Flags: review?(matspal) → review+
Attachment #414183 - Flags: review?(matspal) → review+
Attachment #410134 - Flags: review?(jst) → review+
Attachment #410136 - Flags: review?(jst) → review+
Whiteboard: [needs landing]
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.
Part 0 caused regression bug 536065. It seems we're going to have to back it out and just audit a bazillion code paths :-(.
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.
Could you use script runner and not fully asynchronous thread dispatching?
That's a good idea, I'll look into it. Thanks!
Blocks: 535994
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.
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.
Attachment #418773 - Flags: review?(jonas)
Attachment #418773 - Attachment description: Create AddScriptBlockerAndPreventAddingRunners → Part 34: Create AddScriptBlockerAndPreventAddingRunners
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.
Attachment #418774 - Flags: review?(matspal)
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?
Attachment #418773 - Flags: review?(jonas) → review+
(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.
Attached patch Part 35 v2Splinter Review
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.
Attachment #418774 - Attachment is obsolete: true
Attachment #418808 - Flags: review?(matspal)
Attachment #418774 - Flags: review?(matspal)
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.
Attachment #419076 - Flags: review?(matspal)
Whiteboard: [needs landing] → [needs review]
The nsFrame::FinishAndStoreOverflow changes are actually to avoid assertions about the scrolled area ending up smaller than the scrollport.
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
Attachment #418808 - Flags: review?(matspal) → review+
Attachment #419076 - Flags: review?(matspal) → review+
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!
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review]
Depends on: 539342
Depends on: 539348
Depends on: 539332
No longer depends on: 539332
Depends on: 540161
Depends on: 540183
Depends on: 540247
Depends on: 540294
Depends on: 541144
Depends on: 539344
Depends on: 542115
Depends on: 542677
Depends on: 542900
Depends on: 543681
Depends on: 544085
Target Milestone: --- → mozilla1.9.3a1
Depends on: 545049
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.
Yeah, it probably was this bug. Can you file it as a new bug and mark this bug as blocking it? Thanks.
Blocks: 546033
No longer blocks: 546033
Depends on: 546033
Depends on: 551463
Depends on: 552436
Blocks: 524436
Depends on: 559725
Depends on: 548792
Depends on: 559993
Depends on: 539949
Depends on: 590082
No longer blocks: 595060
Depends on: 595060
Depends on: 607464
Blocks: 447749
Depends on: 629587
Depends on: 718353
Depends on: 729968
Depends on: 752608
Depends on: 754520
Depends on: 795358
Depends on: 1260016
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.