Only load the AutoScroller code when it will be used

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Blocks 1 bug)

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

(Whiteboard: [fxperf:p1][gfx-noted])

Attachments

(2 attachments)

The object called "ClickEventHandler" name is a bit misleading because it's not so generic: it's only responsible for handling the middle-click autoscroller.

It's a big piece of code that is loaded unconditionally, and the object is defined once per tab, so it has memory and startup time impacts.

We can move it to a .jsm and only invoke it when a middle-click occurs. Also, by converting most of the function definitions to class members, each instance of the object will have a smaller memory footprint.
Comment on attachment 8983956 [details]
Bug 1467278 - Part 0 - hg copy the ClickEventHandler code to a new file.

https://reviewboard.mozilla.org/r/249808/#review256014

I'm not a frontend peer and honestly my review here won't mean much. The patch and intention looks sane enough but you might want somebody more familiar with reviewing JS code to take a quick look at these patches and make sure there's no issues here.
Attachment #8983956 - Flags: review?(bugmail) → review+
Comment on attachment 8983957 [details]
Bug 1467278 - Lazily instantiate the AutoScrollController when a middle click occurs.

https://reviewboard.mozilla.org/r/249810/#review256016
Attachment #8983957 - Flags: review?(bugmail) → review+
Attachment #8983957 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8983957 [details]
Bug 1467278 - Lazily instantiate the AutoScrollController when a middle click occurs.

https://reviewboard.mozilla.org/r/249810/#review256178

::: toolkit/content/browser-content.js:22
(Diff revision 1)
>  ChromeUtils.defineModuleGetter(this, "FindContent",
>    "resource://gre/modules/FindContent.jsm");
>  ChromeUtils.defineModuleGetter(this, "RemoteFinder",
>    "resource://gre/modules/RemoteFinder.jsm");
>  
> -var global = this;
> +XPCOMUtils.defineLazyGetter(this, "AutoScrollController", () => {

I'm told these "manual" JS getters aren't as cheap as we might like them to be. Given there's only 1 consumer in the file, I wonder if we could instead just define this as a module getter with `ChromeUtils`, and then in the AutoScrollListener, do something like:

```js
if (/* event checks */) {
  if (!this._controller) {
    this._controller = new AutoScrollController(global);
  }
  this._controller.handleEvent(event);
}
```

Up to you.
Attachment #8983957 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8983957 [details]
Bug 1467278 - Lazily instantiate the AutoScrollController when a middle click occurs.

https://reviewboard.mozilla.org/r/249810/#review256322


Code analysis found 1 defect in this patch:
 - 1 defect 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


::: toolkit/content/browser-content.js:34
(Diff revision 2)
> +  handleEvent(event) {
> +    if (event.isTrusted &
> +        !event.defaultPrevented &&
> +        event.button == 1) {
> +      if (!this._controller) {
> +        this._controller = new AutoScrollController(global);

Error: 'autoscrollcontroller' is not defined. [eslint: no-undef]
Comment on attachment 8983957 [details]
Bug 1467278 - Lazily instantiate the AutoScrollController when a middle click occurs.

https://reviewboard.mozilla.org/r/249810/#review256178

> I'm told these "manual" JS getters aren't as cheap as we might like them to be. Given there's only 1 consumer in the file, I wonder if we could instead just define this as a module getter with `ChromeUtils`, and then in the AutoScrollListener, do something like:
> 
> ```js
> if (/* event checks */) {
>   if (!this._controller) {
>     this._controller = new AutoScrollController(global);
>   }
>   this._controller.handleEvent(event);
> }
> ```
> 
> Up to you.

Done that. For the ChromeUtils definition part, I took the liberty of changing all the individual declarations into the XPCOMUtils.defineLazyModuleGetters, and sort them alphabetically.
For some reason, eslint wasn't picking up the XPCOMUtils.defineLazyModuleGetters correctly, and I wasn't particularly interested in figuring that out right now, so I just used ChromeUtils.defineModuleGetter
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e0e3ad8c81ec
Part 0 - hg copy the ClickEventHandler code to a new file. r=kats
https://hg.mozilla.org/integration/autoland/rev/436a6ebd505e
Lazily instantiate the AutoScrollController when a middle click occurs. r=Gijs,kats
Might have been infra, but I noticed I had accidentally landed a version that didn't contain the `event.button == 1` check that I was using for tests (I only had the Mac trackpad and that doesn't support middle clicks :P)

So let me update the patch with an updated version
Flags: needinfo?(felipc)
So that backout was actually an infra prob (bug 1465117). Please re-land after bug 1465117 lands
Keywords: checkin-needed
In the original push, some bc failures showed up (e.g.  browser_bug1184989_prevent_scrolling_when_preferences_flipped.js). It probably was caused by the mistake that I mentioned in comment 14, but I'll double check on try server before attempting to reland
Keywords: checkin-needed
Priority: -- → P3
Whiteboard: [fxperf:p1] → [fxperf:p1][gfx-noted]
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/57176e81ed9d
Part 0 - hg copy the ClickEventHandler code to a new file. r=kats
https://hg.mozilla.org/integration/autoland/rev/7ac5e24fb3cf
Lazily instantiate the AutoScrollController when a middle click occurs. r=Gijs,kats
https://hg.mozilla.org/mozilla-central/rev/57176e81ed9d
https://hg.mozilla.org/mozilla-central/rev/7ac5e24fb3cf
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.