Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Description • 11 years ago

The way scrollbar interactions trigger scrolling is currently a mess.

An nsHTMLScrollFrame generally has two scrollbars. Each one is a XUL <scrollbar> element. This element has an XBL binding here:
http://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/scrollbar.xml
It has up to two button elements at each end, each one getting an nsScrollbarButtonFrame, plus a slider element in between, which gets an nsSliderFrame.

When you click on a scrollbar button, nsScrollbarButtonFrame::HandleButtonPress gets called to handle the event. Depending on which mouse button was used, it computes mIncrement, the amount to move (using the 'curpos', 'minpos', 'maxpos', 'increment' and 'pageincrement' attributes of the scrollbar element). Then it calls nsScrollbarButtonFrame::DoButtonAction, which basically adds the increment to the value of the 'curpos' attribute and updates that attribute.

That updates some DOM stuff and eventually calls nsScrollbarFrame::AttributeChanged, which calls nsHTMLScrollFrame::CurPosAttributeChanged. That method grabs the current value of 'curpos' from the scrollbar, and then if this is a smooth scroll resets 'curpos' to its old value! Then we trigger the actual scroll operation, which updates 'curpos' to the value we're actually going to use (which may be different due to clamping).

Clicking on the slider or dragging the thumb works similarly. nsSliderFrame updates the scrollbar 'curpos' with the new values and nsHTMLScrollFrame::CurPosAttributeChanged responds.

One problem with this setup is that the 'curpos' attribute of the scrollbar is used both to control how the scrollbar is rendered (e.g. where the thumb is) *and* as an interface for the scrollbar content to tell nsHTMLScrollFrame where to scroll. This means that nsHTMLScrollFrame (via ScrollFrameHelper) has to be careful to ignore CurPosAttributeChanged calls that occur while it's setting 'curpos' itself. Worse, setting 'curpos' is a very limited interface. To communicate whether the operation should be smooth or not, scrollbar content has to set the 'smooth' attribute on the scrollbar temporarily so ScrollFrameHelper::CurPosAttributeChanged can read it. When we introduce scroll snapping, we need to communicate additional information about the scroll gesture.

I think it would make a lot more sense for scrollbar content to make direct calls back to the nsHTML/XULScrollFrame to trigger scrolling actions, and not set 'curpos'.

There is one weird situation we have to deal with: standalone XUL <scrollbar> elements. It's possible (in Firefox addons and other XUL apps) to create a freestanding <scrollbar> element. Events on this scrollbar will update its 'curpos' and this can be used by applications as a sort of slider control. (They'd be better off using <input type="range"> nowadays.) So for compatibility we might need to leave the current setup in place for non-scrollframe scrollbars.

One way to do this would be to build on nsIScrollbarMediator. We already have this as a way for certain elements (XUL trees (nsTreeBodyFrame), XUL listboxes (nsListBoxBodyFrame)) to listen for scrollbar events. We could make nsHTML/XULScrollFrame implement nsIScrollbarMediator, and make nsScrollbarFrame::GetScrollbarMediator return the scrollframe if the scrolled frame is not itself a mediator. We would modify the callers of the nsIScrollbarMediator methods so that they don't set curpos themselves, and make the implementors of those methods always set the scroll position. And we would extend the nsIScrollbarMediator methods to take more information (smooth scroll, type of scroll gesture) as needed.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 3 • 11 years ago

Comment on attachment 8369281 [details] [diff] [review]
Part 1 Scrolling moved from nsScrollbarButtonFrame and nsSlider Frame into nsIScrollbarMediator

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

::: layout/xul/base/public/nsIScrollbarMediator.h
@@ +27,1 @@
>    NS_IMETHOD VisibilityChanged(bool aVisible) = 0;

While we're here, let's make all these methods "virtual void" instead of NS_IMETHOD. Returning result codes from them is unnecessary.

All these methods need to be documented --- document when they get called, what sequences of calls are valid, what the parameters mean. For aDirection specify which values are valid. For SliderScroll (which I think should be called ThumbMoved), document the units of aOldPos and aNewPos. Instead of aSmoothScroll being a boolean, which makes callers hard to read (e.g. SliderScroll(..., false) --- what does that mean?), pass an enum. Although I'm not actually sure why we'd ever want to pass true here!

If we can make aOldPos and aNewPos be nscoord without too much trouble, let's do that too. We prefer not to pass around CSS pixel values for lengths, and these values might theoretically not be integer multiples of CSS pixels.

::: layout/xul/base/src/nsListBoxBodyFrame.cpp
@@ +325,5 @@
> +nsListBoxBodyFrame::ScrollByPage(nsScrollbarFrame* aScrollbar, int32_t aDirection)
> +{
> +  if (aScrollbar == nullptr) {
> +    return NS_OK;
> +  }

These should never be null. Let's make that clear in nsIScrollbarMediator and assert here that aScrollbar is non-null.

@@ +363,5 @@
> +  int32_t increment = aScrollbar->GetIncrement();
> +  if (increment < 0)
> +    UpdateIndex(-1);
> +  else if (increment > 0)
> +    UpdateIndex(1);

{} around these statements

::: layout/xul/base/src/nsScrollbarButtonFrame.cpp
@@ +42,5 @@
> +nsScrollbarButtonFrame::HandleEvent(nsPresContext* aPresContext,
> +                                    WidgetGUIEvent* aEvent,
> +                                    nsEventStatus* aEventStatus)
> +{  
> +  NS_ENSURE_ARG_POINTER(aEventStatus);

I'm not sure why these lines changed. I think you might have introduced rogue CR line endings into this file. Please check and fix.

::: layout/xul/base/src/nsScrollbarFrame.cpp
@@ +159,5 @@
> +  if (!sbm) {
> +    f = GetParent();
> +    scrollFrame = do_QueryFrame(f);
> +    sbm = do_QueryFrame(scrollFrame);
> +  }

Do these changes belong in this patch? I think they belong in the next patch.

@@ +197,5 @@
>    return nsBox::GetMargin(aMargin);
>  }
> +
> +void
> +nsScrollbarFrame::SetIncrementToLine(int32_t aDirection) {

{ on its own line

::: layout/xul/base/src/nsScrollbarFrame.h
@@ +86,5 @@
> +  void SetIncrementToLine(int32_t aDirection);
> +  void SetIncrementToPage(int32_t aDirection);
> +  void SetIncrementToWhole(int32_t aDirection);
> +  int32_t MoveToNewPosition();
> +  int32_t GetIncrement() { return mIncrement; }

Document all these!

@@ +90,5 @@
> +  int32_t GetIncrement() { return mIncrement; }
> +
> +protected:
> +  int32_t mIncrement;
> +  bool mSmoothScroll;

Do we actually need these? Why not just set the attributes directly instead of deferring that to MoveToNewPosition()?

::: layout/xul/base/src/nsSliderFrame.cpp
@@ +1047,5 @@
>    mDestinationPoint = eventPoint;
>    StartRepeat();
> +  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::dir,
> +                            nsGkAtoms::reverse, eCaseMatters))
> +    change = -change;

{}

@@ +1049,5 @@
> +  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::dir,
> +                            nsGkAtoms::reverse, eCaseMatters))
> +    change = -change;
> +  nsIFrame* scrollbar = GetScrollbar();
> +  nsScrollbarFrame* sb = do_QueryFrame(scrollbar); 

Remove trailing space

@@ +1161,5 @@
>      } else {
> +      nscoord change = mChange;
> +      if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::dir,
> +                            nsGkAtoms::reverse, eCaseMatters))
> +        change = -change;

{}

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +4165,5 @@
> +nsTreeBodyFrame::ScrollByPage(nsScrollbarFrame* aScrollbar, int32_t aDirection)
> +{
> +  if (aScrollbar == nullptr) {
> +    return NS_OK;
> +  }

Just assert here

@@ +4196,5 @@
> +  int32_t increment = aScrollbar->GetIncrement();
> +  if (increment < 0)
> +    ScrollInternal(aScrollbar, -1);
> +  else if (increment > 0)
> +    ScrollInternal(aScrollbar, 1);

{}

@@ +4236,5 @@
> +  bool isHorizontal = aScrollbar->IsHorizontal();
> +  int32_t increment = 0;
> +  nsIContent* content = aScrollbar->GetContent();
> +
> +  if (!isHorizontal) {

Swap the if/else clauses so you don't need to use ! here

@@ +4240,5 @@
> +  if (!isHorizontal) {
> +    if (aDirection > 0)
> +      ScrollToRowInternal(parts, mTopRowIndex+1);
> +    else if (aDirection < 0)
> +      ScrollToRowInternal(parts, mTopRowIndex-1);

Why not just "ScrollToRowInternal(parts, mTopRowIndex + aDirection)"?

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 4 • 11 years ago

Comment on attachment 8369282 [details] [diff] [review]
Part 2 nsHTML/XULScrollFrame implement nsIScrollbarMediator

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

I think we should be able to remove some code too. Basically I think ScrollFrameHelper::CurPosAttributeChanged can go away.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1050,5 @@
> +ScrollFrameHelper::ScrollByLine(nsScrollbarFrame* aScrollbar, int32_t aDirection)
> +{
> +  bool smoothScroll = true;
> +  nsIScrollableFrame::ScrollUnit unit = nsIScrollableFrame::LINES;
> +  return ScrollByUnit(aScrollbar, smoothScroll, aDirection, unit);

Are we actually scrolling by the same amount before and after this patch? I have a feeling we won't be. We need to fix that.

@@ +1067,5 @@
> +                                int32_t aNewPos,
> +                                bool aSmoothScroll)
> +{
> +  if (aScrollbar == nullptr)
> +    return NS_OK;

just assert

@@ +1075,5 @@
> +  nsPoint dest = current;
> +  if (isHorizontal)
> +    dest.x = FindFinalScrollingDestination(aNewPos, &allowedRange.x, &allowedRange.width);
> +  else 
> +    dest.y = FindFinalScrollingDestination(aNewPos, &allowedRange.y, &allowedRange.height);

{}

also, remove trailing space after else

@@ +1093,5 @@
> +}
> +
> +NS_IMETHODIMP
> +ScrollFrameHelper::ScrollByUnit(nsScrollbarFrame* aScrollbar,
> +                                bool aSmoothScroll,

Just pass nsIScrollableFrame::ScrollMode instead of using a bool

@@ +1098,5 @@
> +                                int32_t aDirection,
> +                                nsIScrollableFrame::ScrollUnit aUnit)
> +{
> +  if (aScrollbar == nullptr)
> +    return NS_OK;

assert here

::: layout/generic/nsGfxScrollFrame.h
@@ +324,5 @@
> +                          bool aSmoothScroll);
> +  NS_IMETHOD ScrollByUnit(nsScrollbarFrame* aScrollbar,
> +                          bool aSmoothScroll,
> +                          int32_t aDirection,
> +                          nsIScrollableFrame::ScrollUnit aUnit);

These should not be virtual

::: layout/generic/nsIScrollableFrame.h
@@ +276,5 @@
> +  NS_IMETHOD SliderScroll(nsScrollbarFrame* aScrollbar,
> +                          int32_t aOldPos,
> +                          int32_t aNewPos,
> +                          bool aSmoothScroll) = 0;
> +  NS_IMETHOD VisibilityChanged(bool aVisible) = 0;

You don't need to repeat these declarations here.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 5 • 11 years ago

In addition to these patches, I think we should have a third patch that merges nsIScrollbarOwner with nsIScrollbarMediator, now that every frame which implements nsIScrollbarOwner is also an nsIScrollbarMediator.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 8 • 11 years ago

Comment on attachment 8369822 [details] [diff] [review]
Part 1 Scrolling moved from nsScrollbarButtonFrame and nsSlider Frame into nsIScrollbarMediator

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

::: layout/xul/base/public/nsIScrollbarMediator.h
@@ +29,5 @@
> +  // increment to the current position.
> +  virtual void RepeatButtonScroll(nsScrollbarFrame* aScrollbar) = 0;
> +  virtual void ThumbMoved(nsScrollbarFrame* aScrollbar,
> +                          nscoord aOldPos,
> +                          nscoord aNewPos) = 0;

Document that aOldPos and aNewPos are scroll positions, not the position of the thumb itself.

::: layout/xul/base/src/nsListBoxBodyFrame.cpp
@@ +323,5 @@
>  
> +void
> +nsListBoxBodyFrame::ScrollByPage(nsScrollbarFrame* aScrollbar, int32_t aDirection)
> +{
> +  MOZ_ASSERT(aScrollbar != nullptr); 

Trailing spaces!

@@ +411,4 @@
>    }
> +  nsAutoString curpos;
> +  curpos.AppendInt(aNewPos);
> +  mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::curpos, curpos, true);

This should be setting it on the scrollbar right?

::: layout/xul/base/src/nsScrollbarButtonFrame.cpp
@@ +110,5 @@
>      return false;
>  
>    // get the scrollbars content node
>    nsIContent* content = scrollbar->GetContent();
> + 

trailing space!

@@ +125,5 @@
>      direction = -1;
>    else
>      return false;
>  
> +  bool repeat = pressedButtonAction == 2 ? false : true;

!= 2

@@ +141,5 @@
> +      m = sb->GetScrollbarMediator();
> +      switch (pressedButtonAction) {
> +      case 0:
> +        if (m)
> +          m->ScrollByLine(sb, direction);

{}

@@ +192,5 @@
> +    nsScrollbarFrame* sb = do_QueryFrame(scrollbar);
> +    sb->MoveToNewPosition();
> +    if (sb) {
> +      nsIScrollbarMediator* m = sb->GetScrollbarMediator();
> +      if (m) 

trailing space

@@ +195,5 @@
> +      nsIScrollbarMediator* m = sb->GetScrollbarMediator();
> +      if (m) 
> +        m->RepeatButtonScroll(sb);
> +      else
> +       sb->MoveToNewPosition();

{} and fix indent

::: layout/xul/base/src/nsScrollbarFrame.cpp
@@ +218,5 @@
> +  if (aDirection == -1)
> +    mIncrement = -nsSliderFrame::GetCurrentPosition(content);
> +  else
> +    mIncrement = nsSliderFrame::GetMaxPosition(content) - 
> +                 nsSliderFrame::GetCurrentPosition(content);

trailing space

@@ +239,5 @@
> +  int32_t maxpos = nsSliderFrame::GetMaxPosition(content);
> +
> +  // increment the given amount
> +  if (mIncrement)
> +    curpos += mIncrement;

{}

@@ +245,5 @@
> +  // make sure the current position is between the current and max positions
> +  if (curpos < 0)
> +    curpos = 0;
> +  else if (curpos > maxpos)
> +    curpos = maxpos;

{}

::: layout/xul/base/src/nsSliderFrame.cpp
@@ +766,2 @@
>        nsCOMPtr<nsIContent> content = GetContent();
> +      mediator->ThumbMoved(scrollbarFrame, GetCurrentPosition(scrollbar), aNewPos);

convert to appunits

@@ +1165,5 @@
> +                            nsGkAtoms::reverse, eCaseMatters)) {
> +        change = -change;
> +      }
> +      nsIFrame* scrollbar = GetScrollbar();
> +      nsScrollbarFrame* sb = do_QueryFrame(scrollbar); 

trailing space

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +4163,5 @@
>  
> +void
> +nsTreeBodyFrame::ScrollByPage(nsScrollbarFrame* aScrollbar, int32_t aDirection)
> +{
> +  MOZ_ASSERT(aScrollbar != nullptr); 

trailing spaces!

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 9 • 11 years ago

Comment on attachment 8369823 [details] [diff] [review]
Part 2 nsHTML/XULScrollFrame implement nsIScrollbarMediator

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1024,5 @@
> +  nscoord halfPixel = nsPresContext::CSSPixelsToAppUnits(0.5f);
> +  if (aIsHorizontal) {
> +    allowedRange.x = aPoint.x - halfPixel;
> +    allowedRange.width = halfPixel*2 - 1;
> +  } else { 

trailing space

@@ +1036,5 @@
> +ScrollFrameHelper::ScrollByPage(nsScrollbarFrame* aScrollbar, int32_t aDirection)
> +{
> +  nsIScrollableFrame::ScrollMode mode = nsIScrollableFrame::SMOOTH;
> +  nsIScrollableFrame::ScrollUnit unit = nsIScrollableFrame::PAGES;
> +  ScrollByUnit(aScrollbar, mode, aDirection, unit);

Just fold those variables into the call

@@ +1069,5 @@
> +    nscoord increment = lineScrollAmount.height * kScrollMultiplier;
> +    dest.y += increment*aDirection;
> +  }
> +  nsRect allowedRange = GetAllowedRange(dest, isHorizontal);
> +  ScrollTo(dest, mode, &allowedRange);

Try to convert this into a call to ScrollBy instead of ScrollTo.

@@ +1083,5 @@
> +ScrollFrameHelper::ThumbMoved(nsScrollbarFrame* aScrollbar,
> +                              nscoord aOldPos,
> +                              nscoord aNewPos)
> +{
> +  MOZ_ASSERT(aScrollbar != nullptr); 

trailing space

@@ +1090,5 @@
> +  nsPoint dest = current;
> +  if (isHorizontal) {
> +    dest.x = nsPresContext::CSSPixelsToAppUnits(aNewPos);
> +  } else {
> +    dest.y = nsPresContext::CSSPixelsToAppUnits(aNewPos);

no need for these conversions now!

::: layout/xul/base/src/nsScrollbarFrame.cpp
@@ +159,5 @@
> +  if (!sbm) {
> +    f = GetParent();
> +    scrollFrame = do_QueryFrame(f);
> +    sbm = do_QueryFrame(scrollFrame);
> +  }

This is a little scary since we just blindly look at the parent frame, which could be for a completely different element.

I think:
-- if the primary frame is a scrollframe, try the scrolled frame and return that if it's a mediator.
-- if the primary frame is a mediator, return that.
-- call f->PresContext()->PresShell()->GetRootScrollFrame(). If that's non-null and its GetContent() == mScrollbarMediator, return it.

::: layout/xul/base/src/nsSliderFrame.cpp
@@ +765,5 @@
>      if (mediator) {
>        nsCOMPtr<nsIContent> content = GetContent();
> +      nscoord oldPos = nsPresContext::CSSPixelsToAppUnits(GetCurrentPosition(scrollbar));
> +      nscoord newPos = nsPresContext::CSSPixelsToAppUnits(aNewPos);
> +      mediator->ThumbMoved(scrollbarFrame, oldPos, newPos);

This should be in the other patch.

	David Baron :dbaron:
	Comment 10 • 11 years ago

Will this make it so that the chain of logic that leads to scrolling operations doesn't go through the scrollbar anymore, except when the scrollbar was actually used to trigger those operations?

	miranda.j.emery
	Comment 11 • 11 years ago

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #10)
> Will this make it so that the chain of logic that leads to scrolling
> operations doesn't go through the scrollbar anymore, except when the
> scrollbar was actually used to trigger those operations?

Before it was going through the scrollbar buttons or the slider. Now it goes through the mediator if there is one, otherwise through the scrollbar.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 13 • 11 years ago

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #10)
> Will this make it so that the chain of logic that leads to scrolling
> operations doesn't go through the scrollbar anymore, except when the
> scrollbar was actually used to trigger those operations?

I think that's already the case.

Currently when we do a scripted scroll, we set curpos on the scrollbars which triggers a callback into ScrollFrameHelper::CurPosAttributeChange which we have to carefully ignore using the mFrameIsUpdatingScrollbar reentrancy flag. These patches will fix that ugliness because we won't have to be notified of 'curpos' attribute changes anymore.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 15 • 11 years ago

Comment on attachment 8369863 [details] [diff] [review]
Part 1 Scrolling moved from nsScrollbarButtonFrame and nsSlider Frame into nsIScrollbarMediator

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

::: layout/xul/base/src/nsListBoxBodyFrame.h
@@ +63,5 @@
> +  virtual void ThumbMoved(nsScrollbarFrame* aScrollbar,
> +                          int32_t aOldPos,
> +                          int32_t aNewPos) MOZ_OVERRIDE;
> +  virtual void VisibilityChanged(bool aVisible) MOZ_OVERRIDE;
> +  

trailing space

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +4170,5 @@
> +
> +void
> +nsTreeBodyFrame::ScrollByWhole(nsScrollbarFrame* aScrollbar, int32_t aDirection)
> +{
> +  MOZ_ASSERT(aScrollbar != nullptr); 

trailing space

@@ +4178,5 @@
> +
> +void
> +nsTreeBodyFrame::ScrollByLine(nsScrollbarFrame* aScrollbar, int32_t aDirection)
> +{
> +  MOZ_ASSERT(aScrollbar != nullptr); 

trailing space

::: layout/xul/tree/nsTreeBodyFrame.h
@@ +141,5 @@
> +  virtual void ThumbMoved(nsScrollbarFrame* aScrollbar,
> +                          nscoord aOldPos,
> +                          nscoord aNewPos) MOZ_OVERRIDE;
> +  virtual void VisibilityChanged(bool aVisible) MOZ_OVERRIDE { Invalidate(); }
> +  

trailing space

	Mats Palmgren (inactive)
	Comment 16 • 11 years ago

Comment on attachment 8369863 [details] [diff] [review]
Part 1 Scrolling moved from nsScrollbarButtonFrame and nsSlider Frame into nsIScrollbarMediator

SetAttr(..., true) may destroy the world as we know it.
You need to take that into account, usually with a "weakFrame.IsAlive()"
check, and holding on to ref-counted things with nsCOMPtr/nsRefPtr as needed.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 17 • 11 years ago

Comment on attachment 8370314 [details] [diff] [review]
Part 2 nsHTML/XULScrollFrame implement nsIScrollbarMediator

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1017,5 @@
>      mOuter->PresContext()->ThemeChanged();
>    }
>  }
>  
> +nsRect GetAllowedRange(nsPoint aPoint, bool aIsHorizontal)

make this 'static' and put the function name at the beginning of a new line.

@@ +1104,5 @@
> +                                nsIScrollableFrame::ScrollMode aMode,
> +                                int32_t aDirection,
> +                                nsIScrollableFrame::ScrollUnit aUnit)
> +{
> +  MOZ_ASSERT(aScrollbar != nullptr); 

trailing space

	miranda.j.emery
	Comment 18 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #16)
> Comment on attachment 8369863 [details] [diff] [review]
> Part 1 Scrolling moved from nsScrollbarButtonFrame and nsSlider Frame into
> nsIScrollbarMediator
> 
> SetAttr(..., true) may destroy the world as we know it.
> You need to take that into account, usually with a "weakFrame.IsAlive()"
> check, and holding on to ref-counted things with nsCOMPtr/nsRefPtr as needed.

All the calls to SetAttr were already there, they've just been moved around.

	Mats Palmgren (inactive)
	Comment 19 • 11 years ago

(In reply to miranda.j.emery from comment #18)
> All the calls to SetAttr were already there, they've just been moved around.

The call you add in ThumbMoved wasn't there in the old method
(PositionChanged), as far as I can tell.  And you're calling
InternalPositionChanged on the next line where the 'this' frame
might be destroyed.  In HandleButtonPress you add some code between
a SetAttr(..., true) call and a weakFrame.IsAlive() check, code
that depends on the frame being alive.

Besides, "just been moved around" doesn't necessarily mean they will
be safe in the place you moved it to.

I'd like to see a safety analysis on each and everyone of the ones you
moved and the ones you add, please.  And that's not just for the method
the SetAttr call is made, but every caller of that method, and callers
of those etc, until a point where the method is already known to be
destructive or has proper checks in place.

	miranda.j.emery
	Comment 21 • 11 years ago

(In reply to Mats Palmgren (:mats) from comment #19)
> (In reply to miranda.j.emery from comment #18)
> > All the calls to SetAttr were already there, they've just been moved around.
> 
> The call you add in ThumbMoved wasn't there in the old method
> (PositionChanged), as far as I can tell.  And you're calling
> InternalPositionChanged on the next line where the 'this' frame
> might be destroyed.  In HandleButtonPress you add some code between
> a SetAttr(..., true) call and a weakFrame.IsAlive() check, code
> that depends on the frame being alive.
> 
> Besides, "just been moved around" doesn't necessarily mean they will
> be safe in the place you moved it to.
> 
> I'd like to see a safety analysis on each and everyone of the ones you
> moved and the ones you add, please.  And that's not just for the method
> the SetAttr call is made, but every caller of that method, and callers
> of those etc, until a point where the method is already known to be
> destructive or has proper checks in place.

Currently there are calls in MoveToNewPosition and HandleButtonPress, so I've made some changes that should deal with those. The call in ThumbMoved is unnecessary actually so I've removed it altogether. Let me know if there's anything else I should do.

	Mats Palmgren (inactive)
	Comment 23 • 11 years ago

Comment on attachment 8371927 [details] [diff] [review]
Part 1 Scrolling moved from nsScrollbarButtonFrame and nsSlider Frame into nsIScrollbarMediator

>+++ b/layout/xul/nsIScrollbarMediator.h
>+  // aScrollbar is never null.
>+  // aDirection is either -1, 0, or 1.

I see that the current code use a // comment, but please make all
these comments proper doc comments: /** ... */

>+  virtual void ThumbMoved(nsScrollbarFrame* aScrollbar,
>+                          nscoord aOldPos,
>+                          nscoord aNewPos) = 0;

You should include nsCoord.h for the 'nscoord' type.


>+++ b/layout/xul/nsScrollbarButtonFrame.cpp
> nsScrollbarButtonFrame::HandleButtonPress(nsPresContext* aPresContext,
>+      case 0:
>+        if (m) {
>+          m->ScrollByLine(sb, direction);
>+        }
>+        sb->SetIncrementToLine(direction);
>+        break;
>+      case 1:
>+        if (m) {
>+          m->ScrollByPage(sb, direction);
>+        }
>+        sb->SetIncrementToPage(direction);
>+        break;
>+      case 2:
>+        if (m) {
>+          m->ScrollByWhole(sb, direction);
>+        }
>+        sb->SetIncrementToWhole(direction);
>+        break;
>+      case 3:
>+      default:
>+        // We were told to ignore this click, or someone assigned a non-standard
>+        // value to the button's action.
>+        return NS_OK;
>+      }
>+      if (!m) sb->MoveToNewPosition();
>+    }
>   }
>   if (repeat)
>     StartRepeat();
>   return true;
> }

All the ScrollBy* methods call MoveToNewPosition, which is destructive,
so this code needs to deal with 'this', 'sb' etc. being destroyed by
those calls.  I don't think it matters if you use nsWeakFrame(sb) or
nsWeakFrame(this), since if either one is destroyed so is the other
one too.  So you can just check the existing 'weakFrame'.
(And since this method already has an existing nsWeakFrame we don't
need to analyze its callers.  We can assume that's been done.)

"return NS_OK" in one place and "return true" in another doesn't
seem right.


> void nsScrollbarButtonFrame::Notify()
>+  nsWeakFrame weakFrame(this);
>+  if (weakFrame.IsAlive() && (mCursorOnThis ||

This doesn't make sense.  If 'this' isn't alive you shouldn't
create a nsWeakFrame instance on it.  nsWeakFrame should be
used like this:
    nsWeakFrame weakThis(this);
    ... do something that may be destroy the frame tree ...
    if (!weakThis.IsAlive()) {
       return;
    }
    ... OK to use 'this' here, it's still alive ...

So you should just remove the nsWeakFrame you added in this
method.  You can assume that 'this' is alive on entry on all
methods, that's the caller's responsibility.

Please verify that this Notify() thing is cancelled when the
frame is destroyed though, and explain how that is done.


>+++ b/layout/xul/nsScrollbarFrame.cpp
>+nsScrollbarFrame::MoveToNewPosition()
> ...
>+  if (mSmoothScroll)
>+    content->SetAttr(kNameSpaceID_None, nsGkAtoms::smooth, NS_LITERAL_STRING("true"), false);
>+  content->SetAttr(kNameSpaceID_None, nsGkAtoms::curpos, curposStr, true);
>+  if (mSmoothScroll)
>+    content->UnsetAttr(kNameSpaceID_None, nsGkAtoms::smooth, false);
>+  return curpos;
>+}

The SetAttr(..., true) makes the use of mSmoothScroll that follows unsafe.
Is it necessary to do them in this order?  If so, then you need a
"nsWeakFrame weakThis(this)" before it an IsAlive() check after it,
otherwise, I'd suggest setting 'curpos' last.


>+++ b/layout/xul/nsScrollbarFrame.h
>@@ -78,13 +78,27 @@ public:
>    * frame. This means that when the scroll code decides to hide a
>    * scrollframe by setting its height or width to zero, that will
>    * hide the children too.
>    */
>   virtual bool DoesClipChildren() MOZ_OVERRIDE { return true; }
> 
>   NS_IMETHOD GetMargin(nsMargin& aMargin) MOZ_OVERRIDE;
> 
>+  // The following three methods set the value of mIncrement when a
>+  // scrollbar button is pressed.
>+  void SetIncrementToLine(int32_t aDirection);
>+  void SetIncrementToPage(int32_t aDirection);
>+  void SetIncrementToWhole(int32_t aDirection);

Please use /** */ comments.  The comment is slightly misleading.
It should be "... is pressed and then calls MoveToNewPosition.".

>+  // MoveToNewPosition() adds mIncrement to the current position and
>+  // updates the curpos attribute.
>+  int32_t MoveToNewPosition();

For all the four methods above, you should add a @note that clearly
documents that they are destructive.  A phrase I've used in the past
for that is:
  @note This method might destroy the frame, pres shell and other objects.

You should add that in nsIScrollbarMediator.h too.


>+++ b/layout/xul/nsSliderFrame.cpp
>@@ -244,29 +244,33 @@ nsSliderFrame::AttributeChanged(int32_t 
>+      int32_t direction = 0;
>       if (current < min || current > max)

Move that declaration inside the 'if'.

>@@ -758,21 +758,21 @@ nsSliderFrame::SetCurrentPositionInterna
>-      mediator->PositionChanged(scrollbarFrame, GetCurrentPosition(scrollbar), aNewPos);
>+      nscoord oldPos = nsPresContext::CSSPixelsToAppUnits(GetCurrentPosition(scrollbar));
>+      nscoord newPos = nsPresContext::CSSPixelsToAppUnits(aNewPos);
>+      mediator->ThumbMoved(scrollbarFrame, oldPos, newPos);
>       // 'mediator' might be dangling now...
>-      UpdateAttribute(scrollbar, aNewPos, false, aIsSmooth);
>       nsIFrame* frame = content->GetPrimaryFrame();
>       if (frame && frame->GetType() == nsGkAtoms::sliderFrame) {
>         static_cast<nsSliderFrame*>(frame)->CurrentPositionChanged();
>       }
>       mUserChanged = false;
>       return;
>     }
>   }

The current code, and you're new code, is unsafe because 
PositionChanged/ThumbMoved might have destroyed 'this' and
assigning 'mUserChanged' is not protected against that.

Getting the primary frame again just to call CurrentPositionChanged()
on it seems a bit odd.  Can we change it like this:
  nsWeakFrame weakThis(this);
  ... ThumbMoved ...
  if (!weakThis->IsAlive()) {
    return;
  }
  CurrentPositionChanged();
  mUserChanged = false;


>@@ -1043,16 +1043,29 @@ nsSliderFrame::HandlePress(nsPresContext
>+  if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::dir,
>+                            nsGkAtoms::reverse, eCaseMatters)) {
>+    change = -change;
>+  }
>+  nsIFrame* scrollbar = GetScrollbar();
>+  nsScrollbarFrame* sb = do_QueryFrame(scrollbar);
>+  if (sb) {
>+    nsIScrollbarMediator* m = sb->GetScrollbarMediator();
>+    if (m) {
>+      m->ScrollByPage(sb, change);
>+      return NS_OK;
>+    }
>+  }
>   PageUpDown(change);
>   return NS_OK;
> }
> [...]
>@@ -1144,14 +1157,28 @@ void nsSliderFrame::Notify(void)
>                 stop = true;
>         }
>     }
> 
> 
>     if (stop) {
>       StopRepeat();
>     } else {
>-      PageUpDown(mChange);
>+      nscoord change = mChange;
>+      if (mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::dir,
>+                            nsGkAtoms::reverse, eCaseMatters)) {
>+        change = -change;
>+      }
>+      nsIFrame* scrollbar = GetScrollbar();
>+      nsScrollbarFrame* sb = do_QueryFrame(scrollbar);
>+      if (sb) {
>+        nsIScrollbarMediator* m = sb->GetScrollbarMediator();
>+        if (m) {
>+          m->ScrollByPage(sb, change);
>+          return;
>+        }
>+      }
>+      PageUpDown(change);
>     }
> }

These code blocks looks the same to me - can they share the code somehow?


>+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
>+void
>+nsTreeBodyFrame::ScrollInternal(nsScrollbarFrame* aScrollbar, int32_t aDirection)
>+{
>+  ScrollParts parts = GetScrollParts();
>+  bool isHorizontal = aScrollbar->IsHorizontal();
>+  int32_t increment = 0;
>+  nsIContent* content = aScrollbar->GetContent();
>+
>+  nsWeakFrame weakFrame(this);
>+  if (isHorizontal) {
>+    int32_t curpos = aScrollbar->MoveToNewPosition();
>+    if (weakFrame.isAlive()) {
>+      ScrollHorzInternal(parts, curpos);
>+    }
>+  } else {
>+    ScrollToRowInternal(parts, mTopRowIndex+aDirection);
>+  }
>+
>+  if (weakFrame.IsAlive() && mScrollbarActivity) {
>+    mScrollbarActivity->ActivityOccurred();
>+  }
> }

These weakFrame checks looks correct, but you need to make sure
that the caller(s) also deals with it somehow.  There's at least
one (RepeatButtonScroll) that is unsafe.

Also, it's confusing to me to have two methods:

  ScrollInternal(nsScrollbarFrame* aScrollbar, int32_t aDirection)
  ScrollInternal(const ScrollParts& aParts, int32_t aRow)

where one is destructive and the other one is not.
They seem to be quite unrelated actually, so I think you're new
method should use a different name.

	Mats Palmgren (inactive)
	Comment 24 • 11 years ago

... and as I said earlier, I'd like to see a list of all the
possible callers of the methods that are destructive, and the
callers of those etc, and how they deal with it.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 25 • 11 years ago

I'm not convinced it's worth doing all this work and making our code more complex to handle a case that I think can't really happen in Web content. Web content can't create a standalone XUL scrollbar, and can't access the anonymous scrollbars of a scrollframe. So it can't attach mutation listeners or CSS rules or XBL bindings or the other usual things that would trigger frame reconstruction due to these attribute changes. Am I wrong?

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 26 • 11 years ago

Comment on attachment 8370314 [details] [diff] [review]
Part 2 nsHTML/XULScrollFrame implement nsIScrollbarMediator

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +1017,5 @@
>      mOuter->PresContext()->ThemeChanged();
>    }
>  }
>  
> +nsRect GetAllowedRange(nsPoint aPoint, bool aIsHorizontal)

Let's call this "GetOnePixelRangeAroundPoint" to make it more clear.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 27 • 11 years ago

See comment #25

	Mats Palmgren (inactive)
	Comment 28 • 11 years ago

There's a long list of crash bugs that were triggered by the [Un]SetAttr
calls in the code you're changing here.  It's not a mutation listener on
the scrollbar itself that I'm worried about, but any script runners etc
that may run.  We've discussed this problem already in bug 842166 and
as far as I know nothing has changed since then.  Also, some of these
checks were added to protect against the synchronous scrolling path
flushing style changes and whatnot (e.g. bug 490461) and it looks like
that sync path still exists.

Granted, changes to XUL-only code (nsListBoxBodyFrame, nsTreeBodyFrame)
are less worrisome, but the other changes also affects HTML.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 29 • 11 years ago

I think we can solve this by making the setting of the 'curpos' attribute non-notifying, and having the setter code manually perform the specific notifications we need. We don't need to call CurPosAttributeChanged, but we do need to call nsScrollbarFrame::AttributeChanged and some widget stuff and possibly other things.

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 30 • 11 years ago

We need to make sure these notifications get called:
-- nsScrollbarFrame::AttributeChanged
-- nsSliderFrame::AttributeChanged
-- nsITheme::WidgetStateChanged, called from here: http://dxr.mozilla.org/mozilla-central/source/layout/base/RestyleManager.cpp#1000

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 10 years ago

I see various r- from Mats and no recent comments that the final patches are good with him? Is this for sure good to land?

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 35 • 10 years ago

I'll need to update these patches and get re-review from Mats.

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 36 • 10 years ago

https://tbpl.mozilla.org/?tree=Try&rev=9352b8413552

	Mats Palmgren (inactive)
	Comment 37 • 10 years ago

Comment on attachment 8382574 [details] [diff] [review]
Part 1 Scrolling moved from nsScrollbarButtonFrame and nsSlider Frame into nsIScrollbarMediatormediator1.patch

Looks good overall but I have a few nits and corrections:


>layout/xul/nsListBoxBodyFrame.h
>   // nsIScrollbarMediator
>+  ...
>+
>+  virtual void UpdateIndex(int32_t aDirection);

UpdateIndex isn't part of nsIScrollbarMediator or any other
interface AFAICT, so I think we should remove "virtual" and
move the declaration down to the internal methods section.
Perhaps under "// scrolling" ?


>layout/xul/nsScrollbarButtonFrame.cpp
>   nsWeakFrame weakFrame(this);
>   mContent->SetAttr(kNameSpaceID_None, nsGkAtoms::active, NS_LITERAL_STRING("true"), true);
> 
>   nsIPresShell::SetCapturingContent(mContent, CAPTURE_IGNOREALLOWED);
> 
>   if (weakFrame.IsAlive()) {
>-    DoButtonAction(smoothScroll);
>+    nsScrollbarFrame* sb = do_QueryFrame(scrollbar);
> ...

Not your fault, but I think we should change the above weakFrame check
to an early "return false", because we shouldn't call StartRepeat()
at the end with a destroyed 'this'.

>+    nsIScrollbarMediator* m;
>+    if (sb) {
>+      m = sb->GetScrollbarMediator();

Move the 'm' declaration inside the 'if' block, and make the
assignment the initializer.

>+      switch (pressedButtonAction) {
>+      case 0:
>+        if (m) {
>+          m->ScrollByLine(sb, direction);
>+          if (!weakFrame.IsAlive()) {
>+            return false;
>+          }
>+        }
>+        sb->SetIncrementToLine(direction);
>+        break;

Can we call SetIncrementTo* before each 'if (m)' in this switch?
It would simplify it by just making one weakFrame check after 
the switch instead.

>+      if (!m) sb->MoveToNewPosition();
>+      if (!weakFrame.IsAlive()) {
>+        return false;
>+      }

Add {}, and move the weakFrame check inside that block.

>layout/xul/nsScrollbarFrame.cpp
>+nsScrollbarFrame::SetIncrementToLine(int32_t aDirection)
>+{
>+  // get the scrollbar's content node
>+  nsCOMPtr<nsIContent> content = GetContent();
>+  mSmoothScroll = true;
>+  mIncrement = aDirection * nsSliderFrame::GetIncrement(content);
>+}

Using a strong ref in the SetIncrementTo* methods seems unnecessary.
I think it can be just nsIContent*.

>+nsScrollbarFrame::MoveToNewPosition()
>+  if (mSmoothScroll)
>+    content->SetAttr(kNameSpaceID_None, nsGkAtoms::smooth, NS_LITERAL_STRING("true"), false);

Add {}

>+  AttributeChanged(kNameSpaceID_None, nsGkAtoms::curpos, nsIDOMMutationEvent::MODIFICATION);

nsScrollbarFrame::AttributeChanged (for "curpos") calls to
ScrollFrameHelper::CurPosAttributeChanged which is destructive.
We should return early if !weakFrame.IsAlive() here.

>+        sliderFrame->AttributeChanged(... nsGkAtoms::curpos ...)

Add a weakFrame.IsAlive() check here too, just in case.
(I don't think we need a separate nsWeakFrame on 'sliderFrame' because
I don't think it's possible to destroy it separately from its scrollbar.)


>layout/xul/nsScrollbarFrame.h
>+  int32_t GetIncrement() { return mIncrement; }
>+
>+protected:
>+  int32_t mIncrement;

We should probably document what unit this integer has.


>layout/xul/nsSliderFrame.cpp
>@@ -246,27 +250,31 @@ nsSliderFrame::AttributeChanged(int32_t 
>         if (scrollbarFrame) {
>           nsIScrollbarMediator* mediator = scrollbarFrame->GetScrollbarMediator();
>           if (mediator) {
>-            mediator->PositionChanged(scrollbarFrame, GetCurrentPosition(scrollbar), current);
>+            mediator->ScrollByWhole(scrollbarFrame, direction);
>           }
>         }

Shouldn't we call "scrollbarFrame->SetIncrementToWhole()" here?
(like in nsScrollbarButtonFrame::HandleButtonPress)

It might be worth adding a comment after the "if (scrollbarFrame)"
block that says 'this' might be destroyed there.

>@@ -758,25 +762,26 @@ nsSliderFrame::SetCurrentPositionInterna
>+      mediator->ThumbMoved(scrollbarFrame, oldPos, newPos);
>+      if (!weakFrame.IsAlive()) {
>+        return;
>+      }
>       // 'mediator' might be dangling now...

The comment is obsolete since the weakFrame check deals with that.

>   UpdateAttribute(scrollbar, aNewPos, true, aIsSmooth);
>   mUserChanged = false;

Not your fault, but we might as well fix it while we're here -
the 'true' there means the attribute change will be notified
so we should treat it as destructive.  I think you can move
the 'weakFrame' declaration to an outer scope and add an IsAlive()
check here before assigning 'mUserChanged'.

>@@ -1144,14 +1149,35 @@ void nsSliderFrame::Notify(void)
>-      PageUpDown(mChange);
>+      nscoord change = mChange;
>+      PageScroll(change);

The local variable seems unnecessary.

>+NS_IMETHODIMP
>+nsSliderFrame::PageScroll(nscoord aChange)
>+  nsScrollbarFrame* sb = do_QueryFrame(scrollbar);
>+  if (sb) {
>+    nsIScrollbarMediator* m = sb->GetScrollbarMediator();
>+    if (m) {
>+      m->ScrollByPage(sb, aChange);
>+      return NS_OK;
>+    }
>+  }

Same pattern -- shouldn't we call "sb->SetIncrementToPage" ?

This method always returns NS_OK, can we make it return void instead?


>layout/xul/tree/nsTreeBodyFrame.cpp
>+void
>+nsTreeBodyFrame::RepeatButtonScroll(nsScrollbarFrame* aScrollbar)
> ...
>+      ScrollHorzInternal(parts, curpos);
> ...
>+  if (weakFrame.IsAlive() && mScrollbarActivity) {
>+    mScrollbarActivity->ActivityOccurred();
>+  }
>   UpdateScrollbars(parts);

ScrollHorzInternal is destructive so "UpdateScrollbars(parts)"
needs to be inside a "if (weakFrame.IsAlive())" check.

>+nsTreeBodyFrame::ThumbMoved(nsScrollbarFrame* aScrollbar,
>   UpdateScrollbars(parts);

Same thing here. (the "if (NS_FAILED(rv))" used to cover that)

>+void
>+nsTreeBodyFrame::ScrollInternal(nsScrollbarFrame* aScrollbar, int32_t aDirection)
>+{
> }

This method is never called.

	Mats Palmgren (inactive)
	Comment 38 • 10 years ago

Comment on attachment 8382576 [details] [diff] [review]
Part 2 nsHTML/XULScrollFrame implement nsIScrollbarMediator

r=mats, with a few nits:

>layout/generic/nsGfxScrollFrame.cpp
>+ScrollFrameHelper::ScrollByLine(nsScrollbarFrame* aScrollbar, int32_t aDirection)
>+  nsIntPoint delta = nsIntPoint(0,0);

The initializer is redundant, the default ctor makes it 0,0.

>+  nsIntPoint overflow = nsIntPoint(0,0);

Same.

>+ScrollFrameHelper::ScrollByUnit(nsScrollbarFrame* aScrollbar,
>+  nsIntPoint delta = nsIntPoint(0,0);

Same.

>+  nsIntPoint overflow = nsIntPoint(0,0);

Same.


>layout/generic/nsGfxScrollFrame.h
>+  virtual void ScrollByPage(nsScrollbarFrame* aScrollbar, int32_t aDirection) MOZ_OVERRIDE {
>+    mHelper.ScrollByPage(aScrollbar, aDirection);
>+  }
>+  virtual void ScrollByWhole(nsScrollbarFrame* aScrollbar, int32_t aDirection) MOZ_OVERRIDE {
>+    mHelper.ScrollByWhole(aScrollbar, aDirection);
>+  }
>+  virtual void ScrollByLine(nsScrollbarFrame* aScrollbar, int32_t aDirection) MOZ_OVERRIDE {
>+    mHelper.ScrollByLine(aScrollbar, aDirection);
>+  }
>+  virtual void RepeatButtonScroll(nsScrollbarFrame* aScrollbar) MOZ_OVERRIDE {
>+    mHelper.RepeatButtonScroll(aScrollbar);
>+  }
>+  virtual void ThumbMoved(nsScrollbarFrame* aScrollbar,
>+                          nscoord aOldPos,
>+                          nscoord aNewPos) MOZ_OVERRIDE {
>+    mHelper.ThumbMoved(aScrollbar, aOldPos, aNewPos);
>+  }
>+  virtual void VisibilityChanged(bool aVisible) {}

Please add "// nsIScrollbarMediator" before this block of declarations.
Same in nsXULScrollFrame a bit further down.


>layout/xul/nsScrollbarFrame.cpp
>nsScrollbarFrame::GetScrollbarMediator()
>+  nsIScrollableFrame* scrollFrame = do_QueryFrame(f);
>+  nsIFrame* scrolledFrame;
>+  nsIScrollbarMediator* sbm;
> 
>-  // check if the frame is a scroll frame. If so, get the scrollable frame
>-  // inside it.
>-  nsIScrollableFrame* scrollFrame = do_QueryFrame(f);
>   if (scrollFrame) {
>-    f = scrollFrame->GetScrolledFrame();
>+    scrolledFrame = scrollFrame->GetScrolledFrame();

Move the declaration of 'scrolledFrame' here,
it's not use outside this if-block.

>+    sbm = do_QueryFrame(scrolledFrame);
>+    if (sbm) return sbm;

add {}

>+  sbm = do_QueryFrame(f);
>+  if (f && !sbm) {
>+    f = f->PresContext()->PresShell()->GetRootScrollFrame();
>+    if (f && f->GetContent() == mScrollbarMediator) {
>+      return do_QueryFrame(f);
>+    }
>+  }

Why is this block is needed?

	Mats Palmgren (inactive)
	Comment 39 • 10 years ago

Comment on attachment 8382577 [details] [diff] [review]
Part 3 combines nsIScrollbarOwner with nsIScrollbarMediator

r=mats, with a few nits and one bug fix:

>layout/generic/nsGfxScrollFrame.h
>+
>+  virtual void ScrollbarActivityStarted() const MOZ_OVERRIDE;
>+  virtual void ScrollbarActivityStopped() const MOZ_OVERRIDE;

Remove the empty line to follow the general formatting of the
declarations above it.  Same for nsXULScrollFrame further down.

>layout/xul/nsIScrollbarMediator.h
>+class nsIScrollbarMediator : public nsQueryFrame {

Move the { to a separate line.

>layout/xul/nsListBoxBodyFrame.cpp
>+nsIFrame*
>+nsListBoxBodyFrame::GetScrollbarBox(bool aVertical)
>+{
>+  nsIScrollableFrame* scrollFrame = nsLayoutUtils::GetScrollableFrameFor(this);
>+  nsIFrame* scrollbarBox;
>+  if (scrollFrame) {
>+    scrollbarBox = scrollFrame->GetScrollbarBox(true);
>+  }
>+  return scrollbarBox;

'scrollbarBox' may be uninitialized on return.
Perhaps it's simpler to write this as:
  return scrollFrame ? scrollFrame->GetScrollbarBox(true) : nullptr;


>layout/xul/tree/nsTreeBodyFrame.h
>   virtual void RepeatButtonScroll(nsScrollbarFrame* aScrollbar) MOZ_OVERRIDE;
>   virtual void ThumbMoved(nsScrollbarFrame* aScrollbar,
>                           nscoord aOldPos,
>                           nscoord aNewPos) MOZ_OVERRIDE;
>   virtual void VisibilityChanged(bool aVisible) MOZ_OVERRIDE { Invalidate(); }
> 
>   virtual void ScrollInternal(nsScrollbarFrame* aScrollbar, int32_t aDirection);
> 
>-  // nsIScrollbarOwner
>   virtual nsIFrame* GetScrollbarBox(bool aVertical) MOZ_OVERRIDE {

Please remove the empty line above GetScrollbarBox, and move the
ScrollbarActivityStarted/Stopped() declarations below it.

	Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)
	Comment 40 • 10 years ago

I don't have time to work on this. Does anyone want to take over landing this bug?

	Jet Villegas (inactive)
	Comment 41 • 10 years ago

Kip: Is this something you can push through while in the neighborhood?

	:kip (Kearwood Gilbert) Assignee
	Comment 42 • 10 years ago

(In reply to Jet Villegas (:jet) from comment #41)
> Kip: Is this something you can push through while in the neighborhood?

I'll be glad to get this landed.

	:kip (Kearwood Gilbert) Assignee
	Comment 43 • 10 years ago

(In reply to Mats Palmgren (:mats) from comment #38)
> Comment on attachment 8382576 [details] [diff] [review]
> Part 2 nsHTML/XULScrollFrame implement nsIScrollbarMediator
> 
> r=mats, with a few nits:
> 
> >layout/generic/nsGfxScrollFrame.cpp
> >+ScrollFrameHelper::ScrollByLine(nsScrollbarFrame* aScrollbar, int32_t aDirection)
> >+  nsIntPoint delta = nsIntPoint(0,0);
> 
> The initializer is redundant, the default ctor makes it 0,0.
> 
> >+  nsIntPoint overflow = nsIntPoint(0,0);
> 
> Same.
> 
> >+ScrollFrameHelper::ScrollByUnit(nsScrollbarFrame* aScrollbar,
> >+  nsIntPoint delta = nsIntPoint(0,0);
> 
> Same.
> 
> >+  nsIntPoint overflow = nsIntPoint(0,0);
> 
> Same.
> 
> 
> >layout/generic/nsGfxScrollFrame.h
> >+  virtual void ScrollByPage(nsScrollbarFrame* aScrollbar, int32_t aDirection) MOZ_OVERRIDE {
> >+    mHelper.ScrollByPage(aScrollbar, aDirection);
> >+  }
> >+  virtual void ScrollByWhole(nsScrollbarFrame* aScrollbar, int32_t aDirection) MOZ_OVERRIDE {
> >+    mHelper.ScrollByWhole(aScrollbar, aDirection);
> >+  }
> >+  virtual void ScrollByLine(nsScrollbarFrame* aScrollbar, int32_t aDirection) MOZ_OVERRIDE {
> >+    mHelper.ScrollByLine(aScrollbar, aDirection);
> >+  }
> >+  virtual void RepeatButtonScroll(nsScrollbarFrame* aScrollbar) MOZ_OVERRIDE {
> >+    mHelper.RepeatButtonScroll(aScrollbar);
> >+  }
> >+  virtual void ThumbMoved(nsScrollbarFrame* aScrollbar,
> >+                          nscoord aOldPos,
> >+                          nscoord aNewPos) MOZ_OVERRIDE {
> >+    mHelper.ThumbMoved(aScrollbar, aOldPos, aNewPos);
> >+  }
> >+  virtual void VisibilityChanged(bool aVisible) {}
> 
> Please add "// nsIScrollbarMediator" before this block of declarations.
> Same in nsXULScrollFrame a bit further down.
> 
> 
> >layout/xul/nsScrollbarFrame.cpp
> >nsScrollbarFrame::GetScrollbarMediator()
> >+  nsIScrollableFrame* scrollFrame = do_QueryFrame(f);
> >+  nsIFrame* scrolledFrame;
> >+  nsIScrollbarMediator* sbm;
> > 
> >-  // check if the frame is a scroll frame. If so, get the scrollable frame
> >-  // inside it.
> >-  nsIScrollableFrame* scrollFrame = do_QueryFrame(f);
> >   if (scrollFrame) {
> >-    f = scrollFrame->GetScrolledFrame();
> >+    scrolledFrame = scrollFrame->GetScrolledFrame();
> 
> Move the declaration of 'scrolledFrame' here,
> it's not use outside this if-block.
> 
> >+    sbm = do_QueryFrame(scrolledFrame);
> >+    if (sbm) return sbm;
> 
> add {}
> 
> >+  sbm = do_QueryFrame(f);
> >+  if (f && !sbm) {
> >+    f = f->PresContext()->PresShell()->GetRootScrollFrame();
> >+    if (f && f->GetContent() == mScrollbarMediator) {
> >+      return do_QueryFrame(f);
> >+    }
> >+  }
> 
> Why is this block is needed?
I have stepped into this branch and it indeed is being hit while reflowing the Viewport frame and the two earlier conditions could not resolve the nsIScrollbarMediator.

	:kip (Kearwood Gilbert) Assignee
	Comment 44 • 10 years ago

Un-bitrotted and made adjustments from review in Comment 37

	:kip (Kearwood Gilbert) Assignee
	Comment 45 • 10 years ago

Un-bitrotted and made adjustments from review in Comment 38

	:kip (Kearwood Gilbert) Assignee
	Comment 46 • 10 years ago

Un-bitrotted and made adjustments from review in Comment 39

	:kip (Kearwood Gilbert) Assignee
	Comment 47 • 10 years ago

Pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=2439f8e226e6

	Mats Palmgren (inactive)
	Comment 48 • 10 years ago

Comment on attachment 8475509 [details] [diff] [review]
Part 1 Scrolling moved from nsScrollbarButtonFrame and nsSlider Frame into nsIScrollbarMediator

Please put the bug number in the commit message before landing.
(applies to all patches)

r=mats

	Mats Palmgren (inactive)
	Comment 49 • 10 years ago

Comment on attachment 8475510 [details] [diff] [review]
Part 2 nsHTML/XULScrollFrame implement nsIScrollbarMediator

It looks like you attached the wrong file? (it's exactly the same
as part 1)

	Mats Palmgren (inactive)
	Comment 50 • 10 years ago

r=mats on Part 2 in the Try push: https://hg.mozilla.org/try/rev/46268ff286c1
with one nit: please add "// nsIScrollbarMediator" before the ScrollByPage
declaration in nsXULScrollFrame too in nsGfxScrollFrame.h, it looks like it's
missing.

Thanks for finishing this!

	:kip (Kearwood Gilbert) Assignee
	Comment 51 • 10 years ago

This is the correct patch.  (Previously had uploaded Part 1 patch twice)

	:kip (Kearwood Gilbert) Assignee
	Comment 52 • 10 years ago

Added bug number to commit message

	:kip (Kearwood Gilbert) Assignee
	Comment 53 • 10 years ago

Added bug number to commit message

	Mats Palmgren (inactive)
	Comment 54 • 10 years ago

Comment on attachment 8477513 [details] [diff] [review]
Bug 957445: Part 2 - nsHTML/XULScrollFrame implement nsIScrollbarMediator, r=mats

r=mats with the nit in comment 50.

	:kip (Kearwood Gilbert) Assignee
	Comment 55 • 10 years ago

Added "// nsIScrollbarMediator" comment to nsGfxScrollFrame.h as per Comment 50

	:kip (Kearwood Gilbert) Assignee
	Comment 56 • 10 years ago

Added bug number to commit message

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 57 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9eabf947efc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/249dae95ea83
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef03db1385b

	Robert O'Callahan (:roc) (email my personal email if necessary) Reporter
	Comment 58 • 10 years ago

Thanks Kip!!!

	Ryan VanderMeulen [:RyanVM]
	Comment 59 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/9eabf947efc3
https://hg.mozilla.org/mozilla-central/rev/249dae95ea83
https://hg.mozilla.org/mozilla-central/rev/4ef03db1385b

	Yonggang Luo
	Comment 60 • 10 years ago

It's seems after this patch, we can not scroll tree with 1000000 elements and the row height is 112,
the maximal position that can be scrolled to by thumb is about 157000

	Yonggang Luo
	Comment 61 • 10 years ago

Fixup the tree scroll issue when have lots of tree elements.

	Yonggang Luo
	Comment 62 • 10 years ago

Handling the status that newPos would overflow int32_t


 layout/xul/nsSliderFrame.cpp | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/layout/xul/nsSliderFrame.cpp b/layout/xul/nsSliderFrame.cpp
index 3d3d734..91aed75 100644
--- a/layout/xul/nsSliderFrame.cpp
+++ b/layout/xul/nsSliderFrame.cpp
@@ -734,17 +734,22 @@ nsSliderFrame::SetCurrentThumbPosition(nsIContent* aScrollbar, nscoord aNewThumb
   nsRect crect;
   GetClientRect(crect);
   nscoord offset = IsHorizontal() ? crect.x : crect.y;
-  int32_t newPos = NSToIntRound((aNewThumbPos - offset) / mRatio);
+  int64_t newPos = (int64_t)((aNewThumbPos - offset) / double(mRatio));
   
   if (aMaySnap && mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::snap,
                                         nsGkAtoms::_true, eCaseMatters)) {
     // If snap="true", then the slider may only be set to min + (increment * x).
     // Otherwise, the slider may be set to any positive integer.
-    int32_t increment = GetIncrement(aScrollbar);
-    newPos = NSToIntRound(newPos / float(increment)) * increment;
+    int64_t increment = GetIncrement(aScrollbar);
+    newPos = (int64_t)(newPos / double(increment)) * increment;
   }
-  
-  SetCurrentPosition(aScrollbar, newPos, aIsSmooth);
+  if (newPos > INT32_MAX) {
+    newPos = INT32_MAX;
+  }
+  if (newPos < 0) {
+    newPos = 0;
+  }
+  SetCurrentPosition(aScrollbar, (int32_t)newPos, aIsSmooth);
 }
 
 // Use this function when you know the target scroll position of the scrolled content.

	Mats Palmgren (inactive)
	Comment 63 • 10 years ago

Yonggang Luo, please file a separate bug for the regression, thanks.
(for status tracking purposes)

	:kip (Kearwood Gilbert) Assignee
	Comment 64 • 10 years ago

(In reply to Yonggang Luo from comment #62)
> Handling the status that newPos would overflow int32_t
> 
> 
>  layout/xul/nsSliderFrame.cpp | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/layout/xul/nsSliderFrame.cpp b/layout/xul/nsSliderFrame.cpp
> index 3d3d734..91aed75 100644
> --- a/layout/xul/nsSliderFrame.cpp
> +++ b/layout/xul/nsSliderFrame.cpp
> @@ -734,17 +734,22 @@ nsSliderFrame::SetCurrentThumbPosition(nsIContent*
> aScrollbar, nscoord aNewThumb
>    nsRect crect;
>    GetClientRect(crect);
>    nscoord offset = IsHorizontal() ? crect.x : crect.y;
> -  int32_t newPos = NSToIntRound((aNewThumbPos - offset) / mRatio);
> +  int64_t newPos = (int64_t)((aNewThumbPos - offset) / double(mRatio));
>    
>    if (aMaySnap && mContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::snap,
>                                          nsGkAtoms::_true, eCaseMatters)) {
>      // If snap="true", then the slider may only be set to min + (increment
> * x).
>      // Otherwise, the slider may be set to any positive integer.
> -    int32_t increment = GetIncrement(aScrollbar);
> -    newPos = NSToIntRound(newPos / float(increment)) * increment;
> +    int64_t increment = GetIncrement(aScrollbar);
> +    newPos = (int64_t)(newPos / double(increment)) * increment;
>    }
> -  
> -  SetCurrentPosition(aScrollbar, newPos, aIsSmooth);
> +  if (newPos > INT32_MAX) {
> +    newPos = INT32_MAX;
> +  }
> +  if (newPos < 0) {
> +    newPos = 0;
> +  }
> +  SetCurrentPosition(aScrollbar, (int32_t)newPos, aIsSmooth);
>  }
>  
>  // Use this function when you know the target scroll position of the
> scrolled content.

Needsinfo'ing myself until I have a chance to review this.

	Mats Palmgren (inactive)
	Comment 65 • 10 years ago

(In reply to :kip (Kearwood Gilbert) from comment #64)
> Needsinfo'ing myself until I have a chance to review this.

FYI, I think that issue is now filed as bug 1066460.