863102 - Automatically scroll down upon new network requests

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Victor Porof [:vporof][:vp]]

Only if the scroll position is already at the bottom and no item is currently selected.

Comment 1

[David Creswick]

Attachment: patch v1

Comment 2

[Victor Porof [:vporof][:vp]]

Comment on attachment 740132 [details] [diff] [review]
patch v1

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

Good start, Thanks a bunch!
See comment below, and add a test for the network monitor and I'll r+.

::: browser/devtools/shared/widgets/SideMenuWidget.jsm
@@ +106,5 @@
> +
> +    let appended = (aIndex < 0 ||
> +                    aIndex >= this._orderedMenuElementsArray.length);
> +    let scrolledToBottom = (this._list.scrollTop + this._list.clientHeight
> +                            >= this._list.scrollHeight);

I'm worried about the performance issues when this.autoscrollWithAppendedItems isn't true (e.g the debugger). Move these inside some conditionals first.

if (this.autoscrollWithAppendedItems) {
  // Check if appended.
  if (aIndex < 0 || aIndex >= this._orderedMenuElementsArray.length) {
    // Check if already scrolled to bottom.
    if (this._list.scrollTop + this._list.clientHeight >= this._list.scrollHeight) {
      this.ensureElementIsVisible(element);
    }
  }
}

Comment 3

[David Creswick]

(In reply to Victor Porof [:vp] from comment #2)

> ::: browser/devtools/shared/widgets/SideMenuWidget.jsm
> @@ +106,5 @@
> > +
> > +    let appended = (aIndex < 0 ||
> > +                    aIndex >= this._orderedMenuElementsArray.length);
> > +    let scrolledToBottom = (this._list.scrollTop + this._list.clientHeight
> > +                            >= this._list.scrollHeight);
> 
> I'm worried about the performance issues when
> this.autoscrollWithAppendedItems isn't true (e.g the debugger). Move these
> inside some conditionals first.

1) Is what I proposed more expensive than a few property lookups and a bit of arithmetic? I wouldn't expect that to cost very much.

2) One can only determine whether the scroll position is at the bottom *before* adding a new item, so the arithmetic has to happen before the new element is added to the container. Calling group.insertItemAt(...) changes both the scrollHeight of the container and the number of elements in _orderedMenuElementsArray.

Comment 4

[Victor Porof [:vporof][:vp]]

(In reply to David Creswick from comment #3)
> 
> 1) Is what I proposed more expensive than a few property lookups and a bit
> of arithmetic? I wouldn't expect that to cost very much.
> 

Not talking about property lookups and arithmetic here :) .scrollTop, .scrollHeight and .clientHeight cause reflows afaik. The webconsole used to be really slow because of this: "40% of the time is spent flushing layout from scrollHeight getters", see bug 746869.

Comment 5

[David Creswick]

Attachment: patch v2, with test

Comment 6

[Victor Porof [:vporof][:vp]]

Comment on attachment 741344 [details] [diff] [review]
patch v2, with test

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

Lovely, thanks! Just a question about waitForFrameLoad, a suggestion for using two conditionals to avoid calculating scroll positions, and some very awful nits from my side :) R+ with at least the question and suggestion addressed.

::: browser/devtools/netmonitor/test/browser_net_autoscroll.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

Please add a comment describing what the test does.

@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +function test() {
> +
> +  const LOTS_OF_REQUESTS = 8;

Uber Nit: is this const really necessary? It's not like it makes a difference whether it's 8 or something else every single time. I'd just use the scalar everywhere.

@@ +5,5 @@
> +
> +  const LOTS_OF_REQUESTS = 8;
> +
> +  let monitor, debuggee, requestsContainer, scrollTop;
> +  initNetMonitor(INFINITE_GET_URL).then(function ([aTab, aDebuggee, aMonitor]) {

Uber Nit: every other test uses arrow functions.

@@ +8,5 @@
> +  let monitor, debuggee, requestsContainer, scrollTop;
> +  initNetMonitor(INFINITE_GET_URL).then(function ([aTab, aDebuggee, aMonitor]) {
> +    monitor = aMonitor;
> +    debuggee = aDebuggee;
> +    return waitForFrameLoad(monitor.panelWin);

I may be missing something, but I don't understand why you need to wait for the netmonitor's panel iframe to load here. It should already be ready by the time initNetMonitor's promise is resolved. If so, please remove this.

@@ +16,5 @@
> +    requestsContainer = topNode.getElementsByTagName("scrollbox")[0];
> +    ok(!!requestsContainer, "container element exists as expected");
> +
> +    // (1) Make sure the scroll position is maintained at the bottom
> +    // when there are too many requests to display all at once.

Nit: This comment is slightly misleading. The test is making sure the scroll position is maintained, the next couple of calls just perform requests and allow the container to overflow.

@@ +19,5 @@
> +    // (1) Make sure the scroll position is maintained at the bottom
> +    // when there are too many requests to display all at once.
> +    debuggee.performRequests();
> +    return waitForRequestsToOverflowContainer(monitor, requestsContainer);
> +  }).then(function () {

Uber Nit: I prefer a newline after each .then(...)

@@ +51,5 @@
> +  }).then(null, function (err) {
> +    ok(false, err);
> +    console.error(err);
> +    finish();
> +    throw err;

Just ok(false, err) and finish() should be enough here, shouldn't it? For example, having access to the console when this test has failed is a bit of a stretch, stdout is more than enough.

@@ +65,5 @@
> +      }
> +    });
> +  }
> +
> +  function scrolledToBottom(element) {

Nit: s/element/aElement/

::: browser/devtools/shared/widgets/SideMenuWidget.jsm
@@ +110,5 @@
> +      // needlessly expensive reflows.
> +      appended = (aIndex < 0 ||
> +                  aIndex >= this._orderedMenuElementsArray.length);
> +      scrolledToBottom = (this._list.scrollTop + this._list.clientHeight
> +                          >= this._list.scrollHeight);

If |appended| is false you'll still uselessly calculate scroll positions. That's why I suggested two conditionals in my previous review comment.

Comment 7

[David Creswick]

(In reply to Victor Porof [:vp] from comment #6)
> Comment on attachment 741344 [details] [diff] [review]
> patch v2, with test

> @@ +2,5 @@
> > +   http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +function test() {
> > +
> > +  const LOTS_OF_REQUESTS = 8;
> 
> Uber Nit: is this const really necessary? It's not like it makes a
> difference whether it's 8 or something else every single time. I'd just use
> the scalar everywhere.

I suppose it's not strictly necessary, but it makes it slightly easier to change later, if the need arises.

> 
> @@ +8,5 @@
> > +  let monitor, debuggee, requestsContainer, scrollTop;
> > +  initNetMonitor(INFINITE_GET_URL).then(function ([aTab, aDebuggee, aMonitor]) {
> > +    monitor = aMonitor;
> > +    debuggee = aDebuggee;
> > +    return waitForFrameLoad(monitor.panelWin);
> 
> I may be missing something, but I don't understand why you need to wait for
> the netmonitor's panel iframe to load here. It should already be ready by
> the time initNetMonitor's promise is resolved. If so, please remove this.

It *should* be ready, but I've run into problems when writing tests for other devtools. The test passes locally when I remove it though, so I guess it's fine?

Comment 8

[David Creswick]

Attachment: patch v3, to address comments from review

Comment 9

[Victor Porof [:vporof][:vp]]

I'll land this asap.

Comment 10

[Victor Porof [:vporof][:vp]]

Rebased and waiting on 869497.

Comment 11

[Victor Porof [:vporof][:vp]]

https://hg.mozilla.org/integration/fx-team/rev/869b002272b3

Comment 12

[Joe Walker [:jwalker]]

https://hg.mozilla.org/mozilla-central/rev/869b002272b3