:Felipe Gomes (needinfo for replies!) Assignee
	Description • 6 years ago

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.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 3 • 6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f66486d6060ecef5918b718b0a82b1e9df3ceaf5

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 4 • 6 years ago
mozreview-review

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 5 • 6 years ago
mozreview-review

Comment on attachment 8983957 [details]
Bug 1467278 - Lazily instantiate the AutoScrollController when a middle click occurs.

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

	:Gijs (he/him)
	Comment 6 • 6 years ago
mozreview-review

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.

	Code Review Bot [:reviewbot]
	Comment 8 • 6 years ago
mozreview-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]

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 9 • 6 years ago
mozreview-review-reply

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.

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 11 • 6 years ago

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

	Pulsebot
	Comment 12 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 13 • 6 years ago

Backed out for failing gecko decision task action backfill, causing mass osx intermittent failures

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=436a6ebd505e53d110c4e238e9dce0a7bbfa9db1

failure logs:
gecko decision task: https://treeherder.mozilla.org/logviewer.html#?job_id=182345647&repo=autoland&lineNumber=43

Intermittent failure: https://treeherder.mozilla.org/logviewer.html#?job_id=182338595&repo=autoland&lineNumber=1353

Backout: https://hg.mozilla.org/integration/autoland/rev/f718887bdbc347451109825b62131eaca9518672

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 14 • 6 years ago

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

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 17 • 6 years ago

So that backout was actually an infra prob (bug 1465117). Please re-land after bug 1465117 lands

	:Felipe Gomes (needinfo for replies!) Assignee
	Comment 18 • 6 years ago

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

	Pulsebot
	Comment 23 • 6 years ago

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

	Andreea Pavel [:apavel]
	Comment 24 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/57176e81ed9d
https://hg.mozilla.org/mozilla-central/rev/7ac5e24fb3cf