280584 - Maybe this part of the autoscroll code could be optimized (mousemove event handler)?

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Martijn Wargers (dead)]

See:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/browser.xml&rev=1.50&root=/cvsroot#855
Every time when the mouse is moved in Firefox, this code is executed.
I don't think that is really necessary, as this event handler is only necessary
when the autoscroll is enabled and active.
So I think it would probably be better if a mousemove eventhandler would be
attached when the middle mouse button is pressed inside the content area (and
detached when autoscroll is not active anymore).

I'll attach a exaggerated testcase that shows that this indeed is slowing down
Firefox a bit.

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

This takes appr. 4497ms in current trunk Firefox build.
But it takes appr. 2453ms in current trunk Mozilla build.
When I remove the <handler event="mousemove"> in browser.xml of Firefox,
Firefox becomes just as fast as Mozilla.

Comment 2

[Martijn Wargers (dead)]

Attachment: patch

Like this?
This makes Firefox behave just as fast witht the testcase as Mozilla (and the
autoscroll coe still functions the way as it should be).

Comment 3

[Nickolay_Ponomarev]

Good idea. Why don't you ask for review?

Comment 4

[Martijn Wargers (dead)]

Comment on attachment 173121 [details] [diff] [review]
patch

Well, I sort of forgot about it.
Anyway, it is not a very important bug.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 173121 [details] [diff] [review]
patch

r=me, please strip the bogus whitespace

>@@ -728,16 +729,34 @@

here (should be none this line)
>+     
>+     <method name="handlemousemove">
>+        <parameter name="evt"/>

and here

>-
>+            
>+              this.addEventListener('mousemove', this.handlemousemove, false);

Comment 6

[Martijn Wargers (dead)]

Attachment: patch

Wow, that's fast. Thanks, Mike!
I've done what you asked. But also, I've changed the indenting at the last code
in the mousemove handler, which I'm now beginning to believe was wrong.

Comment 7

[Peter van der Woude [:Peter6]]

Mike is not on the cc list Martijn, so you may either need to cc him or ask
review again (or bug him on IRC)

Comment 8

[Martijn Wargers (dead)]

Comment on attachment 178487 [details] [diff] [review]
patch

Yes, I know Peter. I just wanted to recheck the patch, that it worked correctly
(which it still seems to do). 

Mike, this patch looks quite different than the first one, but it is
essentially the same.
All it does is removing the two lines you asked for and it removes some
white-space, which is the code you see at the end of this patch.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 178487 [details] [diff] [review]
patch

>+     <method name="handlemousemove">

interCaps please (handleMouseMove), sorry I missed this

>+        <parameter name="evt"/>

convention is to use aEvent for method/function args, makes a lot of code
clearer

>+            if ((x > this._AUTOSCROLL_SNAP || x < -this._AUTOSCROLL_SNAP) || (y > this._AUTOSCROLL_SNAP || y < -this._AUTOSCROLL_SNAP))

	  if ((x > this._AUTOSCROLL_SNAP || x < -this._AUTOSCROLL_SNAP) ||
	      (y > this._AUTOSCROLL_SNAP || y < -this._AUTOSCROLL_SNAP))

some crazy people use 80 col editors, huge lines are bad

>+              this._snapOn = false;            

trailing whitespace again! :)

almost there! :)

Comment 10

[Martijn Wargers (dead)]

Attachment: patchv2 [Checked in: Comment 11]

Ok, I fixed those things you told me.

> convention is to use aEvent for method/function args, makes a lot of code
> clearer
Well, there are quite some more method/functions in browser.xml that seem to
make use of evt.

Comment 11

[Daniel Brooks [:db48x]]

fix checked in

Comment 12

[Eric Portelance]

+            this.removeEventListener('mousemove', this.handleMouseMove, false);
           

Still some whitespace here, I don't know how anal you want to be about that.  :)

Comment 13

[Martijn Wargers (dead)]

My editor sucks.
I'll remove that whitespace when I need to update a pending patch.

Comment 14

[Greg K.]

Verifiying fixed using Win FF 1.5. Result: 2016ms.