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

VERIFIED FIXED in Firefox1.5

Status

()

Firefox
General
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Martijn Wargers (dead))

Tracking

({testcase})

Trunk
Firefox1.5
x86
Windows XP
testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

13 years ago
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.
(Assignee)

Comment 1

13 years ago
Created attachment 173013 [details]
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.
(Assignee)

Comment 2

13 years ago
Created attachment 173121 [details] [diff] [review]
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

13 years ago
Good idea. Why don't you ask for review?
(Assignee)

Comment 4

13 years ago
Comment on attachment 173121 [details] [diff] [review]
patch

Well, I sort of forgot about it.
Anyway, it is not a very important bug.
Attachment #173121 - Flags: review?(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);
Attachment #173121 - Flags: review?(mconnor) → review+
(Assignee)

Comment 6

13 years ago
Created attachment 178487 [details] [diff] [review]
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.
Attachment #173121 - Attachment is obsolete: true
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)
(Assignee)

Comment 8

13 years ago
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.
Attachment #178487 - Flags: review?(mconnor)
(Assignee)

Updated

13 years ago
Assignee: firefox → martijn.martijn
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! :)
Attachment #178487 - Flags: review?(mconnor) → review-
(Assignee)

Comment 10

13 years ago
Created attachment 186118 [details] [diff] [review]
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.
Attachment #178487 - Attachment is obsolete: true
Attachment #186118 - Flags: review?(mconnor)

Updated

13 years ago
Attachment #186118 - Flags: review?(mconnor) → review+
(Assignee)

Updated

13 years ago
Attachment #186118 - Flags: approval-aviary1.1a2?

Updated

13 years ago
Attachment #186118 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Whiteboard: [checkin needed]
fix checked in
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 12

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

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

Comment 13

13 years ago
My editor sucks.
I'll remove that whitespace when I need to update a pending patch.
Attachment #186118 - Attachment description: patchv2 → patchv2 [Checked in: Comment 11]
Attachment #186118 - Attachment is obsolete: true
Whiteboard: [checkin needed]
Target Milestone: --- → Firefox1.1

Comment 14

12 years ago
Verifiying fixed using Win FF 1.5. Result: 2016ms.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.