1454358 - Remove ScrollBoxObject

Status: RESOLVED FIXED

(Core :: XUL, enhancement, P5)

Description

[:Paolo Amadini]

Most methods of ScrollBoxObject now have alternatives available, so this object can probably be removed. Some methods are probably never used, and definitely some are not exercised by the test suites:

https://codecov.io/gh/marco-c/gecko-dev/src/bd326a2a6b729dc62a5aee57354a97ceac4d1dc0/layout/xul/ScrollBoxObject.cpp

Comment 1

[:Paolo Amadini]

As an alternative to fixing bug 1454360, we can also proceed with these steps that would also allow us to remove some XBL bindings:

1. Use "arrowscrollbox" instead of "scrollbox" in the "popup-scrollbars" binding.

We would hide the arrows with CSS. This way, the call to scrollByIndex would go to the JavaScript implementation in "arrowscrollbox" rather than the C++ implementation in ScrollBoxObject.

Separately, after doing this, we can also consider adding back the scroll arrows on macOS only, to match the native <select> behavior of Safari more closely.

2. Convert the few other remaining calls to ScrollBoxObject methods to use the equivalent DOM methods.

There are only a few of those in the source tree.

3. Remove ScrollBoxObject entirely.



The changes above are sufficient to fix this bug, but they also allow some further improvements:

4. Move the drag-to-scroll implementation to the "arrowscrollbox" binding.

This can be controlled by an attribute like dragtoscroll="true" on the binding. This would probably allow us to reuse the same timer as the other scrolling methods.

5. Consolidate the "popup-scrollbars" binding with the "popup" binding.

This would allow us to get rid of the special "menulist > menupopup" descendant selector used by the "popup-scrollbars" binding. Instead, the "menulist" implementation would just set an attribute on the contained "menupopup" element that would be used by the "popup" binding to set dragtoscroll="true" and hide the scrolling arrows on Windows and Linux.

Comment 2

[Emma Malysz]

Attachment: Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review commit: https://reviewboard.mozilla.org/r/249828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249828/

Comment 3

[Emma Malysz]

Attachment: Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review commit: https://reviewboard.mozilla.org/r/249830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/249830/

Comment 4

[Neil Deakin]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

I realized that there are some additional usages of scrollbox methods that don't use 'scrollBoxObject', but 'boxObject', for example boxObject.scrollBy

We'll need to handle these as well.

See, for example,
https://searchfox.org/mozilla-central/search?q=boxobject.scroll&case=false&regexp=false&path=

I'm going to clear the review until we sort these out.

Comment 5

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/1-2/

Comment 6

[Emma Malysz]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249830/diff/1-2/

Comment 7

[Emma Malysz]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249830/diff/2-3/

Comment 8

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag.  I'm not going to get to this until at least Monday at this point.

Comment 9

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/2-3/

Comment 10

[Emma Malysz]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249830/diff/3-4/

Comment 11

[Emma Malysz]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249830/diff/4-5/

Comment 12

[Neil Deakin]

In the first patch, there's some extra MenuBoxObject files in the patch that i think got mixed up here.

In the second patch, dom/xul/nsXULScrollElement.cpp has some extra functions that were removed from the webidl so they should be removed from there as well (and the header file too).

Comment 13

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/3-4/

Comment 14

[Emma Malysz]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249830/diff/5-6/

Comment 15

[Code Review Bot [:reviewbot]]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249830/#review257276


Code analysis found 4 defects in this patch:
 - 4 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/shared/widgets/FastListWidget.js:196
(Diff revision 6)
>      if (!element) {
>        return;
>      }
>  
>      // Ensure the element is visible but not scrolled horizontally.
> -    const boxObject = this._list.boxObject;
> +    let scrollbox = this._list._scrollbox;

Error: 'scrollbox' is never reassigned. use 'const' instead. [eslint: prefer-const]

::: devtools/client/shared/widgets/SideMenuWidget.jsm:230
(Diff revision 6)
>      if (!aElement) {
>        return;
>      }
>  
>      // Ensure the element is visible but not scrolled horizontally.
> -    const boxObject = this._list.boxObject;
> +    let scrollbox = this._list._scrollbox;

Error: 'scrollbox' is never reassigned. use 'const' instead. [eslint: prefer-const]

::: devtools/client/shared/widgets/SimpleListWidget.jsm:173
(Diff revision 6)
>      if (!aElement) {
>        return;
>      }
>  
>      // Ensure the element is visible but not scrolled horizontally.
> -    const boxObject = this._list.boxObject;
> +    let scrollbox = this._list._scrollbox;

Error: 'scrollbox' is never reassigned. use 'const' instead. [eslint: prefer-const]

::: devtools/client/shared/widgets/VariablesView.jsm:4012
(Diff revision 6)
>      this._input.removeEventListener("keydown", this._onKeydown);
>      this._input.removeEventListener("blur", this.deactivate);
>      this._input.parentNode.replaceChild(this.label, this._input);
>      this._input = null;
>  
> -    const { boxObject } = this._variable._variablesView;
> +    let scrollbox = this._variable._variablesView._scrollbox;

Error: 'scrollbox' is never reassigned. use 'const' instead. [eslint: prefer-const]

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249830/#review257342

I'm sorry for the lag here...  There are a bunch of issues that need fixing, but I can probably do reasonably fast turnaround on a second review if those happen today or tomorrow.

After that, I will be on vacation for a week, but back on the 25th.

::: dom/tests/mochitest/general/test_interfaces.js:1256
(Diff revision 6)
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      {name: "XULElement", insecureContext: true, xbl: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      {name: "XULPopupElement", insecureContext: true, xbl: true},
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    {name: "XULScrollElement", insecureContext: true, xbl: true},

Did this test get run?  I would expect it to fail, since the patch makes the new interface [NoInterfaceObject].

Please either remove this line (and leave the interface [NoInterfaceObject] or make it [ChromeOnly]) or make that interface [Func="IsChromeOrXBL"] depending on the desired behavior.

::: dom/webidl/XULScrollElement.webidl:9
(Diff revision 6)
> +
> +

Please remove the weird blank lines.

::: dom/xul/moz.build:38
(Diff revision 6)
>          'nsXULContentUtils.cpp',
>          'nsXULElement.cpp',
>          'nsXULPopupListener.cpp',
>          'nsXULPrototypeCache.cpp',
>          'nsXULPrototypeDocument.cpp',
> +        'nsXULScrollElement.cpp',

Please call this XULScrollElement.cpp.

::: dom/xul/nsXULElement.cpp:78
(Diff revision 6)
>  #include "mozAutoDocUpdate.h"
>  #include "nsCCUncollectableMarker.h"
>  #include "nsICSSDeclaration.h"
>  #include "nsLayoutUtils.h"
>  #include "XULPopupElement.h"
> +#include "nsXULScrollElement.h"

Please call that XULScrollElement.h and remove the bindings.conf annotation.

::: dom/xul/nsXULElement.cpp:180
(Diff revision 6)
>    if (nodeInfo->Equals(nsGkAtoms::menupopup) ||
>        nodeInfo->Equals(nsGkAtoms::popup) ||
>        nodeInfo->Equals(nsGkAtoms::panel) ||
>        nodeInfo->Equals(nsGkAtoms::tooltip)) {
>      return NS_NewXULPopupElement(nodeInfo.forget());
> +  } else if (nodeInfo->Equals(nsGkAtoms::scrollbox)){

No else after return, please.  So:

    if (something) {
      return foo;
    }
    
    if (somethingElse) {
      return bar;
    }

::: dom/xul/nsXULScrollElement.h:24
(Diff revision 6)
> +	} // namespace dom
> +} // namespace mozilla
> +
> +nsXULElement* NS_NewXULScrollElement(already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo);
> +
> +class nsXULScrollElement final : public nsXULElement

Please make this mozilla::dom::XULScrollElement.

::: dom/xul/nsXULScrollElement.h:31
(Diff revision 6)
> +public:
> +  explicit nsXULScrollElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo): nsXULElement(aNodeInfo)
> +  {
> +  }
> +
> +  void ScrollByIndex(int32_t dindexes, ErrorResult& aRv);

I know you just copied this, but "dindexes" is not a very clear arg name and the args should be consistent in whether they use the 'a' prefix.

::: dom/xul/nsXULScrollElement.h:32
(Diff revision 6)
> +  explicit nsXULScrollElement(already_AddRefed<mozilla::dom::NodeInfo>& aNodeInfo): nsXULElement(aNodeInfo)
> +  {
> +  }
> +
> +  void ScrollByIndex(int32_t dindexes, ErrorResult& aRv);
> +  void EnsureElementIsVisible(Element& child, ErrorResult& aRv);

Args should be consistent about 'a' prefix.

::: dom/xul/nsXULScrollElement.h:33
(Diff revision 6)
> +
> +
> +

Weird extra blank lines.

::: dom/xul/nsXULScrollElement.h:37
(Diff revision 6)
> +  void EnsureElementIsVisible(Element& child, ErrorResult& aRv);
> +
> +
> +
> +protected:
> +  virtual ~nsXULScrollElement(){

Please either put a space before the '{' and put the '}' on the same line, or put a linebreak before '{' and then remove the blank line between the curlies.

::: dom/xul/nsXULScrollElement.h:44
(Diff revision 6)
> +  }
> +  virtual JSObject* WrapNode(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;
> +
> +private:
> +  nsIFrame* GetFrame(bool aFlushLayout);
> +  nsIFrame* GetScrolledElement(nsXULElement *aScrollElement); 

Trailing whitespace.

::: dom/xul/nsXULScrollElement.h:47
(Diff revision 6)
> +private:
> +  nsIFrame* GetFrame(bool aFlushLayout);
> +  nsIFrame* GetScrolledElement(nsXULElement *aScrollElement); 
> +  nsIScrollableFrame* GetScrollFrame(nsIFrame **aStyledFrame = nullptr,
> +                                     mozilla::FlushType aFlushType = mozilla::FlushType::Layout);
> +

Extra whitespace line should go away.

::: dom/xul/nsXULScrollElement.cpp:17
(Diff revision 6)
> +#include "nsPresContext.h"
> +#include "nsBox.h"
> +#include "nsIScrollableFrame.h"
> +#include "mozilla/dom/XULScrollElementBinding.h"
> +
> +using namespace mozilla; 

Trailin whitespace here, and various other places in the file.

::: dom/xul/nsXULScrollElement.cpp:20
(Diff revision 6)
> +#include "mozilla/dom/XULScrollElementBinding.h"
> +
> +using namespace mozilla; 
> +using namespace mozilla::dom; 
> +
> +nsXULElement* NS_NewXULScrollElement(already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo){

Please put the '{' on the next line.  That applies to all the functions in this file.

::: dom/xul/nsXULScrollElement.cpp:29
(Diff revision 6)
> +JSObject* nsXULScrollElement::WrapNode(JSContext *aCx, JS::Handle<JSObject*> aGivenProto){
> +  return XULScrollElementBinding::Wrap(aCx, this, aGivenProto); 
> +}
> +
> +nsIFrame* nsXULScrollElement::GetFrame(bool aFlushLayout){
> +  nsCOMPtr<nsIContent> hold = this;

Please don't.  Instead, annotate this method as MOZ_CAN_RUN_SCRIPT in the header and then annotate callers as needed, etc.  By the time you are done, you will be guaranteed that caller is holding a useful strong ref to "this".

::: dom/xul/nsXULScrollElement.cpp:42
(Diff revision 6)
> +}
> +
> +nsIScrollableFrame*
> +nsXULScrollElement::GetScrollFrame(nsIFrame **aStyledFrame, FlushType aFlushType)
> +{
> +  // it isn't clear what to return for SVG nodes, so just return nothing

Why is this code here?  We're a XULScrollElement.  We aren't SVG by definition; we're in a different namespace.

It looks like you basically copied Element::GetScrollFrame, but why can't we just use it directly?

If we do need to copy it, please remove all the irrelevant bits (e.g we can't have a ComboboxControl frame here).

This is also different behavior from what ScrollBoxObject had.  Why the behavior change?

::: dom/xul/nsXULScrollElement.cpp:110
(Diff revision 6)
> +    if (!sf) {
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return;
> +    }
> +
> +    nsIFrame* scrolledBox = GetScrolledElement(this);

Why is this not called GetScrolledFrame?

Also, why are we not just getting it off "sf" (followed by the GetChildXULBox thing)?

::: dom/xul/nsXULScrollElement.cpp:118
(Diff revision 6)
> +      return;
> +    }
> +
> +    nsRect rect;
> +
> +    // now get the scrolled boxes first child.

This looks like it was copied from ScrollBoxObject.  It would be good, for blame-preservation, to "hg mv" ScrollBoxObject into this file and then edit it to look like this.  That will also make it clear which code is the same and which is changing.

::: dom/xul/nsXULScrollElement.cpp:222
(Diff revision 6)
> +
> +
> +void nsXULScrollElement::EnsureElementIsVisible(Element& child, ErrorResult& aRv)
> +{
> +    nsIDocument* doc = OwnerDoc();
> +    nsIPresShell* shell = doc->GetShell();

This needs to be a strong reference...  Again, this wold be clearer if this were done on top of "hg mv".

::: dom/xul/nsXULScrollElement.cpp:253
(Diff revision 6)
> +    if (!scrolledFrame)
> +      return nullptr;
> +    return nsBox::GetChildXULBox(scrolledFrame);
> +}
> +
> +void nsXULScrollElement::ScrollToElement(Element& child, ErrorResult& aRv)

This looks unused to me.  And since it's not declared in the class, I expect it doesn't compile...

::: dom/xul/nsXULScrollElement.cpp:272
(Diff revision 6)
> +
> +
> +
> +
> +

Please remove the extra blank lines.

Comment 17

[Neil Deakin]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249828/#review257350

::: browser/components/places/content/menu.xml:86
(Diff revision 4)
>              let elt = aEvent.target;
>              if (elt.localName == "menupopup")
>                elt = elt.parentNode;
>  
>              // Calculate positions taking care of arrowscrollbox
> -            let scrollbox = this._scrollBox;
> +            let scrollbox = this._scrollbox;

I think the original 'this._scrollBox' was correct. It is defined earlier in the file.

::: browser/components/places/content/menu.xml:88
(Diff revision 4)
>                elt = elt.parentNode;
>  
>              // Calculate positions taking care of arrowscrollbox
> -            let scrollbox = this._scrollBox;
> +            let scrollbox = this._scrollbox;
>              let eventY = aEvent.layerY + (scrollbox.boxObject.y - this.boxObject.y);
> -            let scrollboxOffset = scrollbox.scrollBoxObject.y -
> +            let scrollboxOffset = this.boxObject.y;

I don't understand this change.

::: browser/components/places/content/menu.xml:436
(Diff revision 4)
>            event.stopPropagation();
>            return;
>          }
>  
>          // We should display the drop indicator relative to the arrowscrollbox.
> -        let sbo = this._scrollBox.scrollBoxObject;
> +        let sbo = this.boxObject;

I think this should be this._scrollBox.boxObject

::: browser/components/places/content/menu.xml:449
(Diff revision 4)
>                                 sbo.height;
>          } else if (scrollDir == 1)
>            newMarginTop = sbo.height;
>  
>          // Set the new marginTop based on arrowscrollbox.
> -        newMarginTop += sbo.y - this._scrollBox.boxObject.y;
> +        newMarginTop += sbo.y - this.boxObject.y;

Same here.

::: testing/mochitest/browser-harness.xul:327
(Diff revision 4)
>      function scrollTo(id) {
>        var line = document.getElementById(id);
>        if (!line)
>          return;
>  
> -      var boxObject = document.getElementById("results").parentNode.boxObject;
> +      var scrollbox = document.getElementById("results").parentNode._scrollbox;

I think this can just use parentNode (that is, no _scrollbox needed)

::: toolkit/content/widgets/richlistbox.xml:277
(Diff revision 4)
>  
>              // If the current item is visible, scroll by one page so that
>              // the new current item is at approximately the same position as
>              // the existing current item.
>              if (this._isItemVisible(this.currentItem))
> -              this.scrollBoxObject.scrollBy(0, this.scrollBoxObject.height * aDirection);
> +              this._scrollbox.scrollBy(0, this.boxObject.height * aDirection);

Should 'this.boxObject.height' be 'this._scrollbox.boxObject.height' ?

::: toolkit/content/widgets/richlistbox.xml:361
(Diff revision 4)
>                if (currentItem) {
>                  this.currentItem = currentItem;
>                  if (this.selType != "multiple" && this.selectedCount == 0)
>                    this.selectedItem = currentItem;
>  
> -                if (this.scrollBoxObject.height) {
> +                if (this.boxObject.height) {

Same here.

::: toolkit/content/widgets/richlistbox.xml:430
(Diff revision 4)
>  
> -            var y = this.scrollBoxObject.positionY + this.scrollBoxObject.y;
> +            var y = this._scrollbox.scrollTop + this._scrollbox.boxObject.y;
>  
>              // Partially visible items are also considered visible
>              return (aItem.boxObject.y + aItem.boxObject.height > y) &&
> -                   (aItem.boxObject.y < y + this.scrollBoxObject.height);
> +                        (aItem.boxObject.y < y + this.boxObject.height);

Same here.

::: toolkit/content/widgets/scrollbox.xml
(Diff revision 4)
>      <content>
>        <xul:box class="box-inherit scrollbox-innerbox" xbl:inherits="orient,align,pack,dir" flex="1">
>          <children/>
>        </xul:box>
>      </content>
> -

Technically, this change should go in the other patch since that's the one that changes scrollByIndex.

::: toolkit/modules/SelectParentHelper.jsm:250
(Diff revision 4)
>        // opened menulist is, and what browser it belongs to...
>        if (!currentMenulist) {
>          return;
>        }
>  
> -      let scrollBox = currentMenulist.menupopup.scrollBox;
> +      let scrollbox = currentMenulist.menupopup._scrollbox;

I think menupopup.scrollBox was correct. There is a definition of scrollBox in popup.xml

Comment 18

[Emma Malysz]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249830/diff/6-7/

Comment 19

[Jorg K (CEST = GMT+2)]

Are you really going to remove this? We followed bug 1465866 in bug 1469014, but if you remove it completely, we have more work.

Comment 20

[Emma Malysz]

(In reply to Jorg K (GMT+2) from comment #19)
Hi Jorg, I'm an intern working on the XBL replacement team, and the goal is to start completely removing some of the BoxObjects. This patch removes all instances of the ScrollBoxObject and looks to replace the calls with a new XULScrollElement and other calls to the element class. Neil's my mentor monitoring my work on this bug

Comment 21

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jorg K (GMT+2) from comment #19)
> Are you really going to remove this? We followed bug 1465866 in bug 1469014,
> but if you remove it completely, we have more work.

Hi Jorg, yes, this is going away. You'll likely need to find a way of preserving it within comm-central unless you can find a way of getting off of it completely.

Comment 22

[Neil Deakin]

Most properties/methods of scrollBoxObject can be replaced by standard scroll properties/methods on the element. For example, use scrollLeft instead of positionX.

Only two methods remain that have no standard counterpart, scrollByIndex and ensureElementIsVisible, but they are now available on scrollbox elements directly, so use scrollbox.scrollByIndex instead of scrollbox.boxObject.scrollByIndex

Comment 23

[Neil Deakin]

Comment on attachment 8983974 [details]
Bug #1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249830/#review259182

::: browser/components/places/content/menu.xml:86
(Diff revision 7)
>              let elt = aEvent.target;
>              if (elt.localName == "menupopup")
>                elt = elt.parentNode;
>  
>              // Calculate positions taking care of arrowscrollbox
> -            let scrollbox = this._scrollbox;
> +            let scrollbox = this._scrollBox;

You should merge the changes to this file from the previous patch, since this one mostly just undoes the first set of changes.

::: dom/bindings/Bindings.conf:1379
(Diff revision 7)
>  'XULDocument': {
>      'headerFile': 'XULDocument.h'
>  },
>  
>  'XULElement': {
> -    'nativeType': 'nsXULElement',
> +    'nativeType': 'nsXULElement'

We don't need this change.

::: dom/xul/XULScrollElement.h:14
(Diff revision 7)
> +
> +#include "nsXULElement.h"
> +#include "Units.h"
> +
> +class nsIScrollableFrame;
> +struct nsRect;

Neither nsIScrollableFrame nor nsRect are used in the header so these declarations could be removed.

::: dom/xul/XULScrollElement.h:20
(Diff revision 7)
> +
> +namespace mozilla {
> +namespace dom {
> +
> +
> +nsXULElement* 

Extra space at the end of this line

::: dom/xul/XULScrollElement.h:21
(Diff revision 7)
> +namespace mozilla {
> +namespace dom {
> +
> +
> +nsXULElement* 
> +NS_NewXULScrollElement(already_AddRefed<mozilla::dom::NodeInfo>&& aNodeInfo);

In bug 1437638 it was suggested that this wrapper function be removed and the constructor be called directly. Let's do that here too. See the patch in that bug for how this was done.

::: dom/xul/XULScrollElement.cpp:43
(Diff revision 7)
> +      aRv.Throw(NS_ERROR_FAILURE);
> +      return;
> +    }
> +
> +    nsIFrame* scrolledFrame = sf->GetScrolledFrame();
> +    scrolledFrame = nsBox::GetChildXULBox(scrolledFrame);

scrolledFrame should be null-checked before calling GetChildXULBox.

::: dom/xul/XULScrollElement.cpp:62
(Diff revision 7)
> +    int32_t curIndex = 0;
> +    bool isLTR = scrolledFrame->IsXULNormalDirection();
> +
> +    int32_t frameWidth = 0;
> +    if (!isLTR && horiz) {
> +      nsCOMPtr<nsIDocument> doc = GetUncomposedDoc();

The original code had a GetWidth call here.

::: dom/xul/nsXULElement.cpp:182
(Diff revision 7)
>        nodeInfo->Equals(nsGkAtoms::panel) ||
>        nodeInfo->Equals(nsGkAtoms::tooltip)) {
>      return NS_NewXULPopupElement(nodeInfo.forget());
>    }
>  
> +  if (nodeInfo->Equals(nsGkAtoms::scrollbox)){

Add a space before the curly brace.

::: layout/reftests/invalidation/540247-1-ref.xul:12
(Diff revision 7)
>  document.addEventListener("MozReftestInvalidate", runtest, false);
>  
>  function runtest() {
>    document.getElementById('b').height = '100';
>  
> -  var sbo = document.getElementById('s').boxObject;
> +  var sbo = document.getElementById('s');

Should probably rename this scrollbox instead of 'sbo'.

::: layout/reftests/invalidation/540247-1.xul:13
(Diff revision 7)
>  
>  function runtest() {
>    // Make sure that the effects of the scroll are painted to the screen before
>    // shrinking the size of the scrolled frame.
>    window.addEventListener("MozAfterPaint", finish, false);
> -  var sbo = document.getElementById('s').boxObject;
> +  var sbo = document.getElementById('s');

Same here.

::: testing/mochitest/browser-harness.xul:327
(Diff revision 7)
>      function scrollTo(id) {
>        var line = document.getElementById(id);
>        if (!line)
>          return;
>  
> -      var scrollbox = document.getElementById("results").parentNode._scrollbox;
> +      var scrollbox = document.getElementById("results").parentNode;

The scrollbox isn't used here, so this line could be removed.

::: toolkit/modules/SelectParentHelper.jsm:250
(Diff revision 7)
>        // opened menulist is, and what browser it belongs to...
>        if (!currentMenulist) {
>          return;
>        }
>  
> -      let scrollbox = currentMenulist.menupopup._scrollbox;
> +      let scrollBox = currentMenulist.menupopup.scrollBox;

This just undoes the changes from the previous patch, so let's remove this to avoid changing the file unneccsarily.

Comment 24

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/4-5/

Comment 25

[Neil Deakin]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249828/#review259584

::: browser/components/places/content/menu.xml
(Diff revision 5)
>  
>              // Calculate positions taking care of arrowscrollbox
>              let scrollbox = this._scrollBox;
>              let eventY = aEvent.layerY + (scrollbox.boxObject.y - this.boxObject.y);
> -            let scrollboxOffset = scrollbox.scrollBoxObject.y -
> +            let scrollboxOffset = scrollbox.boxObject.y;
> -                                  (scrollbox.boxObject.y - this.boxObject.y);

Not sure why part of this line was removed.

::: dom/xul/XULScrollElement.h:12
(Diff revision 5)
> +#ifndef XULScrollElement_h__
> +#define XULScrollElement_h__
> +
> +class nsIScrollableFrame;
> +struct nsRect;
> +

I had meant that the nsIScrollableFrame and nsRect declarations are unneeded. The includes of course would still be needed. (Although our unified build system is probably masking/hiding which includes are actually needed.

::: dom/xul/XULScrollElement.cpp:59
(Diff revision 5)
> +    nsPoint cp = sf->GetScrollPosition();
> +    nscoord diff = 0;
> +    int32_t curIndex = 0;
> +    bool isLTR = scrolledFrame->IsXULNormalDirection();
> +
> +    int frameWidth = 0;

OK, I understand better what was the issue was here. 'scrolledFrame->GetRect().width' returns an nscoord, so frameWidth should be an nscoord.

You don't need the CSSPixelsToAppUnits conversion at all anymore, since an 'AppUnit' is an nscoord.

And looking over this again, I think we should use the 'nsLayoutUtils::GetAllInFlowRectsUnion(frame, parent)' that GetOffsetRect uses, where frame is the scrolled frame and parent is the root frame. The root frame can be retrived from presShell->GetRootFrame()

::: dom/xul/XULScrollElement.cpp:67
(Diff revision 5)
> +      nsCOMPtr<nsIDocument> doc = GetUncomposedDoc();
> +      if (!doc) {
> +        return;
> +      }
> +
> +

Remove the extra blank line.

::: toolkit/content/widgets/richlistbox.xml:277
(Diff revision 5)
>  
>              // If the current item is visible, scroll by one page so that
>              // the new current item is at approximately the same position as
>              // the existing current item.
>              if (this._isItemVisible(this.currentItem))
> -              this.scrollBoxObject.scrollBy(0, this.scrollBoxObject.height * aDirection);
> +              this._scrollbox.scrollBy(0, this.boxObject.height * aDirection);

There's a number of places where 'this.scrollBoxObject.height' should be changed to 'this._scrollbox.boxObject.height'. Make sure to change them all.

Comment 26

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/5-6/

Comment 27

[Neil Deakin]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249828/#review260000

::: browser/components/places/content/menu.xml:88
(Diff revision 6)
>                elt = elt.parentNode;
>  
>              // Calculate positions taking care of arrowscrollbox
>              let scrollbox = this._scrollBox;
>              let eventY = aEvent.layerY + (scrollbox.boxObject.y - this.boxObject.y);
> -            let scrollboxOffset = scrollbox.scrollBoxObject.y -
> +            let scrollboxOffset = this.boxObject.y;

This should be 
let scrollboxOffset = scrollbox.boxObject.y -
(scrollbox.boxObject.y - this.boxObject.y);

Note the paranthesis as A - (B - C) isn't the same as A - B - C

Comment 28

[Emma Malysz]

(In reply to Neil Deakin from comment #27)
> This should be 
> let scrollboxOffset = scrollbox.boxObject.y -
> (scrollbox.boxObject.y - this.boxObject.y);
> 
> Note the paranthesis as A - (B - C) isn't the same as A - B - C

But A - (A - B) simplify to just B, so isn't let scrollboxOffset = this.boxObject.y correct?

Comment 29

[Mike Conley (:mconley) (:⚙️)]

needinfo?ing Enn for comment 28

Comment 30

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/6-7/

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249828/#review260778

::: dom/chrome-webidl/XULScrollElement.webidl:9
(Diff revision 7)
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + */
> +
> +[HTMLConstructor, Func="IsChromeOrXBL"]
> +interface XULScrollElement : XULElement {
> +

Please remove the blank line.

::: dom/chrome-webidl/XULScrollElement.webidl:16
(Diff revision 7)
> +  void scrollByIndex(long aIndexes);
> +  [Throws]
> +  void ensureElementIsVisible(Element child);
> +  [Throws]
> +  void scrollToElement(Element child);
> +

And here.

::: dom/xul/XULScrollElement.cpp:28
(Diff revision 7)
> +{
> +  return XULScrollElementBinding::Wrap(aCx, this, aGivenProto);
> +}
> +
> +void
> +XULScrollElement::ScrollByIndex(int32_t aIndex, ErrorResult& aRv)

Again, there should have been an "hg mv" from ScrollBoxObject.cpp here to preserve blame and make it clearer what's actually changing.  Please do that; it will make this much simpler to review.

::: dom/xul/XULScrollElement.cpp:30
(Diff revision 7)
> +    nsIScrollableFrame* sf = GetScrollFrame();
> +    if (!sf) {
> +      aRv.Throw(NS_ERROR_FAILURE);

Is this file being two-space indented or 4-space?  Needs to be consistent.  Modeline says 2-space, but there's clearly a bunch of 4-space indent going on here.

Comment 32

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/7-8/

Comment 33

[Boris Zbarsky [:bzbarsky]]

I'm a little confused.  This diff shows code being removed that was already removed in bug 1465866.  Is this change not on top of that change?  (Ideally that change would just land and this change would definitely be on top of it, so it's clear what's going on here.)

Comment 34

[Emma Malysz]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #33)
> I'm a little confused.  This diff shows code being removed that was already
> removed in bug 1465866.  Is this change not on top of that change?

I had tried checking in my changes to that bug, but I didn't have permission. I think Neil may check it in still, so should I apply those changes to this bug?

Comment 35

[Boris Zbarsky [:bzbarsky]]

Looks like Brian Grinstead checked that in.  Please rebase these changes on top of that.

Comment 36

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/8-9/

Comment 37

[Neil Deakin]

As suggested by https://bugzilla.mozilla.org/show_bug.cgi?id=1437638#c25 , a block of code also needs to be added there for scrollbox.

Comment 38

[Neil Deakin]

(In reply to Emma Malysz from comment #28)
> But A - (A - B) simplify to just B, so isn't let scrollboxOffset =
> this.boxObject.y correct?

Yes, of course. I think I must have misread the code.

Comment 39

[Dão Gottwald [:dao]]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249828/#review261650

::: toolkit/content/widgets/richlistbox.xml:424
(Diff revision 9)
>        <method name="_isItemVisible">
>          <parameter name="aItem"/>
>          <body>
>            <![CDATA[
>              if (!aItem)
> -              return false;
> +            return false;

please revert this ...

::: toolkit/content/widgets/richlistbox.xml:430
(Diff revision 9)
>  
> -            var y = this.scrollBoxObject.positionY + this.scrollBoxObject.y;
> +            var y = this._scrollbox.scrollTop + this._scrollbox.boxObject.y;
>  
>              // Partially visible items are also considered visible
>              return (aItem.boxObject.y + aItem.boxObject.height > y) &&
> -                   (aItem.boxObject.y < y + this.scrollBoxObject.height);
> +                        (aItem.boxObject.y < y + this._scrollbox.boxObject.height);

... as well as this indentation change

Comment 40

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/9-10/

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249828/#review262348

Just to make sure we are on the same page, I only reviewed the C++ changes to dom/ and layout, the changes to test_interfaces.js, and the webidl changes.  If there was something else you were relying on me reviewing, please let me know.

::: dom/chrome-webidl/XULScrollElement.webidl
(Diff revision 10)
>    [Throws]
> -  void scrollBy(long dx, long dy);
> +  void scrollByIndex(long aIndexes);
> -  [Throws]
> -  void scrollByIndex(long dindexes);
> -  [Throws]
> -  void scrollToElement(Element child);

The C++ still has ScrollToElement, and there is a consumer in toolkit/content/widgets/richlistbox.xml as far as I can tell.  Should this really be removed from the idl?  If not, and tests did not catch this, please add a test that would have caught it.

::: dom/xul/XULScrollElement.h:25
(Diff revision 10)
> -  virtual JSObject* WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +  }
> -
> -  virtual nsIScrollableFrame* GetScrollFrame();
>  
> -  void ScrollTo(int32_t x, int32_t y, ErrorResult& aRv);
> -  void ScrollBy(int32_t dx, int32_t dy, ErrorResult& aRv);
> +  MOZ_CAN_RUN_SCRIPT void ScrollByIndex(int32_t aIndex, ErrorResult& aRv);
> +  void EnsureElementIsVisible(Element& aChild, ErrorResult& aRv);

Should this one be MOZ_CAN_RUN_SCRIPT too?

::: dom/xul/XULScrollElement.h:26
(Diff revision 10)
> -  virtual nsIScrollableFrame* GetScrollFrame();
>  
> -  void ScrollTo(int32_t x, int32_t y, ErrorResult& aRv);
> -  void ScrollBy(int32_t dx, int32_t dy, ErrorResult& aRv);
> +  MOZ_CAN_RUN_SCRIPT void ScrollByIndex(int32_t aIndex, ErrorResult& aRv);
> +  void EnsureElementIsVisible(Element& aChild, ErrorResult& aRv);
> -  void ScrollByIndex(int32_t dindexes, ErrorResult& aRv);
>    void ScrollToElement(Element& child, ErrorResult& aRv);

And this one?

::: dom/xul/XULScrollElement.h:30
(Diff revision 10)
> -  void EnsureElementIsVisible(Element& child, ErrorResult& aRv);
> -
>  
>  protected:
> -  void GetScrolledSize(nsRect& aRect, ErrorResult& aRv);
> -  void GetPosition(CSSIntPoint& aPos, ErrorResult& aRv);
> +  virtual ~XULScrollElement() {}
> +  virtual JSObject* WrapNode(JSContext *aCx, JS::Handle<JSObject*> aGivenProto) override;

Please drop the "virtual"; see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods last paragraph in the C++ section.  And given this is a final class, this function should probably be marked "final", not "override".

::: dom/xul/XULScrollElement.cpp:24
(Diff revision 10)
>  namespace dom {
>  
> -ScrollBoxObject::ScrollBoxObject()
> +JSObject*
> +XULScrollElement::WrapNode(JSContext *aCx, JS::Handle<JSObject*> aGivenProto)
>  {
> +  return XULScrollElementBinding::Wrap(aCx, this, aGivenProto);

You're going to need XULScrollElement_Binding::Wrap here now.

::: dom/xul/XULScrollElement.cpp:42
(Diff revision 10)
> -    if (!sf) {
> -      aRv.Throw(NS_ERROR_FAILURE);
> +    aRv.Throw(NS_ERROR_FAILURE);
> -      return;
> +    return;
> -    }
> +  }
>  
> -    nsIFrame* scrolledBox = GetScrolledBox(this);
> +  scrolledFrame = nsBox::GetChildXULBox(scrolledFrame);

Why did we rename this from scrolledBox to scrolledFrame?  It might be OK, but could at least use mention in the commit message.

::: dom/xul/XULScrollElement.cpp:61
(Diff revision 10)
> -    int32_t curIndex = 0;
> +  int32_t curIndex = 0;
> -    bool isLTR = scrolledBox->IsXULNormalDirection();
> +  bool isLTR = scrolledFrame->IsXULNormalDirection();
>  
> -    int32_t frameWidth = 0;
> +  nscoord frameWidth = 0;
> -    if (!isLTR && horiz) {
> +  if (!isLTR && horiz) {
> -      GetWidth(&frameWidth);
> +    nsCOMPtr<nsIDocument> doc = GetUncomposedDoc();

Why GetUncomposedDoc?  GetComposedDoc would make more sense to me...  and the old code sure seems to be getting the composed doc, as of bug 1472651.

::: dom/xul/XULScrollElement.cpp:62
(Diff revision 10)
> -    bool isLTR = scrolledBox->IsXULNormalDirection();
> +  bool isLTR = scrolledFrame->IsXULNormalDirection();
>  
> -    int32_t frameWidth = 0;
> +  nscoord frameWidth = 0;
> -    if (!isLTR && horiz) {
> +  if (!isLTR && horiz) {
> -      GetWidth(&frameWidth);
> -      nsCOMPtr<nsIPresShell> shell = GetPresShell(false);
> +    nsCOMPtr<nsIDocument> doc = GetUncomposedDoc();
> +    if (!doc) {

And if we get the composed doc, can it really be null here given we have a frame?

::: dom/xul/XULScrollElement.cpp:71
(Diff revision 10)
> +    nsCOMPtr<nsIPresShell> shell = doc->GetShell();
> -      if (!shell) {
> +    if (!shell) {
> -        aRv.Throw(NS_ERROR_UNEXPECTED);
> +      aRv.Throw(NS_ERROR_UNEXPECTED);
> -        return;
> +      return;
> -      }
> +    }
> -      frameWidth = nsPresContext::CSSPixelsToAppUnits(frameWidth);
> +    nsRect rcFrame = nsLayoutUtils::GetAllInFlowRectsUnion(scrolledFrame, shell->GetRootFrame());

This seems to be a behavior change, no?  The old code used the width of this element's primary frame as far as I can tell; this is using the width of the thing that used to be called scrolledBox.

Why is this change being made?  If there's a reason, it should be in the commit message.  But I suspect this change shouldn't have been made and that we need better test coverage that would have caught this.  Please add those tests.

::: dom/xul/XULScrollElement.cpp:158
(Diff revision 10)
>  }
>  
> -void ScrollBoxObject::ScrollToElement(Element& child, ErrorResult& aRv)
> +void
> +XULScrollElement::ScrollToElement(Element& child, ErrorResult& aRv)
>  {
> -  nsCOMPtr<nsIPresShell> shell = GetPresShell(false);
> +  nsCOMPtr<nsIDocument> doc = GetUncomposedDoc();

Again, GetComposedDoc.

::: dom/xul/XULScrollElement.cpp:160
(Diff revision 10)
> -void ScrollBoxObject::ScrollToElement(Element& child, ErrorResult& aRv)
> +void
> +XULScrollElement::ScrollToElement(Element& child, ErrorResult& aRv)
>  {
> -  nsCOMPtr<nsIPresShell> shell = GetPresShell(false);
> +  nsCOMPtr<nsIDocument> doc = GetUncomposedDoc();
> +  if (!doc) {
> +    return;

We used to throw in this case.  Why the behavior change?

::: dom/xul/XULScrollElement.cpp:170
(Diff revision 10)
>      aRv.Throw(NS_ERROR_UNEXPECTED);
>      return;
>    }
>  
>    shell->ScrollContentIntoView(&child,
> -                               nsIPresShell::ScrollAxis(
> +                                nsIPresShell::ScrollAxis(

This indentation change is wrong.

::: dom/xul/XULScrollElement.cpp:173
(Diff revision 10)
>  
>    shell->ScrollContentIntoView(&child,
> -                               nsIPresShell::ScrollAxis(
> +                                nsIPresShell::ScrollAxis(
>                                   nsIPresShell::SCROLL_TOP,
>                                   nsIPresShell::SCROLL_ALWAYS),
> -                               nsIPresShell::ScrollAxis(
> +                                nsIPresShell::ScrollAxis(

And this one.

::: dom/xul/XULScrollElement.cpp:176
(Diff revision 10)
> -                               nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY |
> +                                nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY |
> -                               nsIPresShell::SCROLL_OVERFLOW_HIDDEN);
> +                                nsIPresShell::SCROLL_OVERFLOW_HIDDEN);

And both of these.

::: dom/xul/XULScrollElement.cpp:184
(Diff revision 10)
> -}
>  
> -/* private helper */
> -void ScrollBoxObject::GetScrolledSize(nsRect& aRect, ErrorResult& aRv)
> +void
> +XULScrollElement::EnsureElementIsVisible(Element& aChild, ErrorResult& aRv)
>  {
> -    nsIFrame* scrolledBox = GetScrolledBox(this);
> +  nsCOMPtr<nsIDocument> doc = GetUncomposedDoc();

GetComposedDoc.

::: dom/xul/XULScrollElement.cpp:186
(Diff revision 10)
> -void ScrollBoxObject::GetScrolledSize(nsRect& aRect, ErrorResult& aRv)
> +XULScrollElement::EnsureElementIsVisible(Element& aChild, ErrorResult& aRv)
>  {
> -    nsIFrame* scrolledBox = GetScrolledBox(this);
> -    if (!scrolledBox) {
> +  nsCOMPtr<nsIDocument> doc = GetUncomposedDoc();
> +  if (!doc) {
> -      aRv.Throw(NS_ERROR_FAILURE);
> -      return;
> +    return;

We used to throw in this case. Why the behavior change?

::: dom/xul/XULScrollElement.cpp:196
(Diff revision 10)
> -                                 nsIPresShell::ScrollAxis(),
> +                                nsIPresShell::ScrollAxis(),
> -                                 nsIPresShell::ScrollAxis(),
> +                                nsIPresShell::ScrollAxis(),
> -                                 nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY |
> +                                nsIPresShell::SCROLL_FIRST_ANCESTOR_ONLY |
> -                                 nsIPresShell::SCROLL_OVERFLOW_HIDDEN);
> +                                nsIPresShell::SCROLL_OVERFLOW_HIDDEN);

These are all mis-indented.

Comment 42

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/10-11/

Comment 43

[Emma Malysz]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #41)

> ::: dom/xul/XULScrollElement.cpp:71
> (Diff revision 10)
> > +    nsCOMPtr<nsIPresShell> shell = doc->GetShell();
> > -      if (!shell) {
> > +    if (!shell) {
> > -        aRv.Throw(NS_ERROR_UNEXPECTED);
> > +      aRv.Throw(NS_ERROR_UNEXPECTED);
> > -        return;
> > +      return;
> > -      }
> > +    }
> > -      frameWidth = nsPresContext::CSSPixelsToAppUnits(frameWidth);
> > +    nsRect rcFrame = nsLayoutUtils::GetAllInFlowRectsUnion(scrolledFrame, shell->GetRootFrame());
> 
> This seems to be a behavior change, no?  The old code used the width of this
> element's primary frame as far as I can tell; this is using the width of the
> thing that used to be called scrolledBox.
> 
> Why is this change being made?  If there's a reason, it should be in the
> commit message.  But I suspect this change shouldn't have been made and that
> we need better test coverage that would have caught this.  Please add those
> tests.

I had discussed with Neil that this would be a valid replacement, similar to how GetOffsetRect handles it. I have changed scrolledFrame to GetPrimaryFrame.

Comment 44

[Boris Zbarsky [:bzbarsky]]

> I had discussed with Neil that this would be a valid replacement, similar to how GetOffsetRect handles it

My question wasn't about why we're using GetAllInFlowRectsUnion instead of frameWidth.

My question was why we're now using the width of a child of the scrollframe, whereas we used to use the width of the scrollframe.

Looks like that was unintentional, right?  I'm not seeing tests that would have caught the unintentional hange here....

Comment 45

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

https://reviewboard.mozilla.org/r/249828/#review262688

r=me if the tests for the missing scrollToElement and the right width in XULScrollElement::ScrollByIndex are added.

::: dom/chrome-webidl/XULScrollElement.webidl:12
(Diff revisions 10 - 11)
>  [HTMLConstructor, Func="IsChromeOrXBL"]
>  interface XULScrollElement : XULElement {
>    [Throws]
>    void scrollByIndex(long aIndexes);
>    [Throws]
> +  void scrollToElement(Element child); 

Please remove the trailing whitespace.

Again, did tests catch this being missing?  If not, please add some tests.

Comment 46

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/11-12/

Comment 47

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/12-13/

Comment 48

[Boris Zbarsky [:bzbarsky]]

Emma, I just wanted to check that you're not blocked on me for review at this point.  Is that correct?

Comment 49

[Emma Malysz]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #48)
> Emma, I just wanted to check that you're not blocked on me for review at
> this point.  Is that correct?

No, it's ready to be checked in now, but I just don't have permission to do so. Thanks for reviewing it!

Comment 50

[Mike Conley (:mconley) (:⚙️)]

I'll autoland this for you - thanks for your work here, Emma, and thanks for reviewing Enn and bz!

Comment 51

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s e82ee1db2349e4952653f4c1eda4c65837fe0328 -d 7ef930709d66: rebasing 472407:e82ee1db2349 "Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102 r=bz,enndeakin+6102" (tip)
merging dom/base/Element.h
merging dom/base/nsDocument.cpp
warning: conflicts while merging dom/base/Element.h! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 52

[Mike Conley (:mconley) (:⚙️)]

Whoops - looks like this needs a rebase.

Comment 53

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/13-14/

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b099e7e0b264
removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102 r=bz,enndeakin+6102

Comment 55

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d610dd7bfbcd
Backed out changeset b099e7e0b264 for build bustages on Element.h CLOSED TREE

Comment 56

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset b099e7e0b264 (bug 1454358) for build bustages on Element.h

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b099e7e0b26494e0b2cdc549f75ff19291627968

Backout link: https://hg.mozilla.org/integration/autoland/rev/d610dd7bfbcdc0ec00ce393dff833f2d8e1b7986 

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=187688460&repo=autoland&lineNumber=12439

[task 2018-07-11T22:37:54.531Z] 22:37:54     INFO -  /usr/bin/yasm -o x86_abi_support.o -f elf64 -rnasm -pnasm -g dwarf2 -I/builds/worker/workspace/build/src/media/libaom/config/linux/x64/ -DPIC -I. -I/builds/worker/workspace/build/src/third_party/aom   /builds/worker/workspace/build/src/third_party/aom/aom_ports/x86_abi_support.asm
[task 2018-07-11T22:37:54.536Z] 22:37:54     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/media/libaom'
[task 2018-07-11T22:37:54.536Z] 22:37:54     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/media/libaom'
[task 2018-07-11T22:37:54.536Z] 22:37:54     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/media/libaom'
[task 2018-07-11T22:37:54.536Z] 22:37:54     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/xpcom/base'
[task 2018-07-11T22:37:54.536Z] 22:37:54     INFO -  /builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/gcc/bin/g++ -o Unified_cpp_xpcom_base0.o -c -I/builds/worker/workspace/build/src/obj-firefox/dist/stl_wrappers -I/builds/worker/workspace/build/src/obj-firefox/dist/system_wrappers -include /builds/worker/workspace/build/src/config/gcc_hidden.h -DDEBUG=1 -DOS_POSIX=1 -DOS_LINUX=1 -DSTATIC_EXPORTABLE_JS_API -DMOZ_HAS_MOZGLUE -DMOZILLA_INTERNAL_API -DIMPL_LIBXUL -I/builds/worker/workspace/build/src/xpcom/base -I/builds/worker/workspace/build/src/obj-firefox/xpcom/base -I/builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/_ipdlheaders -I/builds/worker/workspace/build/src/ipc/chromium/src -I/builds/worker/workspace/build/src/ipc/glue -I/builds/worker/workspace/build/src/xpcom/build -I/builds/worker/workspace/build/src/dom/base -I/builds/worker/workspace/build/src/xpcom/ds -I/builds/worker/workspace/build/src/obj-firefox/dist/include -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nspr -I/builds/worker/workspace/build/src/obj-firefox/dist/include/nss -fPIC -DMOZILLA_CLIENT -include /builds/worker/workspace/build/src/obj-firefox/mozilla-config.h -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -Wall -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wduplicated-cond -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -D_GLIBCXX_USE_CXX11_ABI=0 -fno-sized-deallocation -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -Os -fno-omit-frame-pointer -Werror -I/builds/worker/workspace/build/src/widget/gtk/compat-gtk3 -pthread -I/usr/include/gtk-3.0 -I/usr/include/pango-1.0 -I/usr/include/gio-unix-2.0/ -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/gdk-pixbuf-2.0 -I/usr/include/freetype2 -I/usr/include/glib-2.0 -I/usr/lib/x86_64-linux-gnu/glib-2.0/include -I/usr/include/pixman-1 -I/usr/include/libpng12 -I/usr/include/gtk-3.0/unix-print  -MD -MP -MF .deps/Unified_cpp_xpcom_base0.o.pp   /builds/worker/workspace/build/src/obj-firefox/xpcom/base/Unified_cpp_xpcom_base0.cpp
[task 2018-07-11T22:37:54.537Z] 22:37:54     INFO -  In file included from /builds/worker/workspace/build/src/dom/base/nsDOMMutationObserver.h:20:0,
[task 2018-07-11T22:37:54.537Z] 22:37:54     INFO -                   from /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:35,
[task 2018-07-11T22:37:54.537Z] 22:37:54     INFO -                   from /builds/worker/workspace/build/src/obj-firefox/xpcom/base/Unified_cpp_xpcom_base0.cpp:20:
[task 2018-07-11T22:37:54.537Z] 22:37:54     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Element.h:1996:23: error: 'nsIScrollableFrame* mozilla::dom::Element::GetScrollFrame(nsIFrame**, mozilla::FlushType)' cannot be overloaded
[task 2018-07-11T22:37:54.537Z] 22:37:54     INFO -     nsIScrollableFrame* GetScrollFrame(nsIFrame **aFrame = nullptr,
[task 2018-07-11T22:37:54.538Z] 22:37:54     INFO -                         ^~~~~~~~~~~~~~
[task 2018-07-11T22:37:54.538Z] 22:37:54     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/Element.h:599:23: error: with 'nsIScrollableFrame* mozilla::dom::Element::GetScrollFrame(nsIFrame**, mozilla::FlushType)'
[task 2018-07-11T22:37:54.539Z] 22:37:54     INFO -     nsIScrollableFrame* GetScrollFrame(nsIFrame **aStyledFrame = nullptr,
[task 2018-07-11T22:37:54.540Z] 22:37:54     INFO -                         ^~~~~~~~~~~~~~
[task 2018-07-11T22:37:54.540Z] 22:37:54     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1052: recipe for target 'Unified_cpp_xpcom_base0.o' failed
[task 2018-07-11T22:37:54.540Z] 22:37:54     INFO -  make[4]: *** [Unified_cpp_xpcom_base0.o] Error 1
[task 2018-07-11T22:37:54.540Z] 22:37:54     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/xpcom/base'
[task 2018-07-11T22:37:54.540Z] 22:37:54     INFO -  /builds/worker/workspace/build/src/config/recurse.mk:74: recipe for target 'xpcom/base/target' failed
[task 2018-07-11T22:37:54.541Z] 22:37:54     INFO -  make[3]: *** [xpcom/base/target] Error 2
[task 2018-07-11T22:37:54.541Z] 22:37:54     INFO -  make[3]: *** Waiting for unfinished jobs....
[task 2018-07-11T22:37:54.552Z] 22:37:54     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/media/libaom'
[task 2018-07-11T22:37:54.553Z] 22:37:54     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/media/libaom'
[task 2018-07-11T22:37:54.611Z] 22:37:54     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/media/libaom'
[task 2018-07-11T22:37:54.611Z] 22:37:54     INFO -  /usr/bin/yasm -o av1_quantize_ssse3_x86_64.o -f elf64 -rnasm -pnasm -g dwarf2 -I/builds/worker/workspace/build/src/media/libaom/config/linux/x64/ -DPIC -I. -I/builds/worker/workspace/build/src/third_party/aom   /builds/worker/workspace/build/src/third_party/aom/av1/encoder/x86/av1_quantize_ssse3_x86_64.asm
[task 2018-07-11T22:37:54.611Z] 22:37:54     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/media/libaom'
[task 2018-07-11T22:37:54.615Z] 22:37:54     INFO -  make[4]: Entering directory '/builds/worker/workspace/build/src/obj-firefox/media/libaom'
[task 2018-07-11T22:37:54.615Z] 22:37:54     INFO -  make[4]: Leaving directory '/builds/worker/workspace/build/src/obj-firefox/media/libaom'

Comment 57

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/14-15/

Comment 58

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/15-16/

Comment 59

[Emma Malysz]

Comment on attachment 8983973 [details]
Bug 1454358, removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/249828/diff/16-17/

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3af5036936c1
removes unneccessary implementation of ScrollBoxObject rr?enndeakin+6102 r=bz,enndeakin+6102

Comment 61

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/3af5036936c1