239919 - Marquee DOMAttrModified - should it stay?

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Doron Rosenberg (IBM)]

http://bugzilla.mozilla.org/show_bug.cgi?id=167224 introduced a DOMAttrModified
handler, which isn't performant.

I'll see if I can hack a way around it, or if we even need it in the end.

Comment 1

[Martijn Wargers (dead)]

Well, it can be removed when you overload the setAttribute method, just like Erik did with his xbl marquee:
http://webfx.eae.net/dhtml/xblmarquee/implementation.html
(probably, putting all the extra properties/methods - that don't belong in a marquee - in a separate __marquee__ object is a good idea, also).

It might very well be that I need to 'steal' some code from him for this (and probably also for other marquee stuff). I've asked him if that's all right, and he agreed.

Also the removeAttribute method needs to be overloaded.

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase2, using scrolldelay, scrollamount and truespeed

Comment 4

[Martijn Wargers (dead)]

Attachment: patch1

Well, I actually didn't go for the approach in comment 1, but I took a kind of a pragmatic approach.
I overloaded the setAttribute and removeAttribute methods, but by using other known dom methods (setAttributeNodeNS and removeAttributeNode).
So these other dom methods don't work with this patch, but I don't think they are used a lot and certainly in combination with marquees.

Comment 5

[Boris Zbarsky [:bzbarsky]]

That breaks setAttributeNS too, no?

Personally, I think we should stop shying away from mutation listeners and just make mutation events stuck less in the core.

Comment 6

[Martijn Wargers (dead)]

(In reply to comment #5)
> That breaks setAttributeNS too, no?
Yes, but that will also not be used on marquees since IE doesn't support setAttributeNS and marquee is generally considered an IE thing.

But I can rewrite the DOMAttrModified part, and leave out the setAttribute/removeAttribute part, if you prefer?
I think the rest of the patch is still necessary for cleanup/more correctness in behavior.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 202052 [details] [diff] [review]
patch1

I agree with bz. This is not a good approach and I think that marquee is used rarly enough that we can avoid ughly hacks just to squeeze performance.

If we can't make mutation attributes listeners in xbl fast enough we have to come up with something better in XBL2.

This patch will not work for C++ callers or for getAttributeNS callers.

Comment 8

[Martijn Wargers (dead)]

Attachment: patch2

patch2, now still using the domattrmodified handler. 
This patch doesn't really have any relevance with this bug, but I started here, so, I'll keep posting the patches here.
This patch is for cleanup/more correctness. See the testcases.

Comment 9

[Doron Rosenberg (IBM)]

Comment on attachment 202317 [details] [diff] [review]
patch2

>+                break;
>+              case "truespeed":
>+                //needed to update this._scrollDelay
>+                setTimeout(function(x){ x._set_scrollDelay(x.getAttribute('scrolldelay'), false); }, 0, this);

it is more readable to do var foo = function(){}; setTimeout(foo);

Comment 10

[Martijn Wargers (dead)]

Comment on attachment 202317 [details] [diff] [review]
patch2

I'll address the review comment in a new patch.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Martijn, could you get someone else to sr this?  I won't be able to get to this for at least a week...

Comment 12

[Martijn Wargers (dead)]

Comment on attachment 202317 [details] [diff] [review]
patch2

Sure, I'll ask Johnny.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 202317 [details] [diff] [review]
patch2

sr=jst

Comment 14

[Martijn Wargers (dead)]

Attachment: patch3

This is with Doron's review comment addressed.

Comment 15

[Martijn Wargers (dead)]

Comment on attachment 202677 [details] [diff] [review]
patch3

So this is ready for check-in, right?

Comment 16

[:Gavin Sharp [email: [e-mail]]]

mozilla/layout/style/xbl-marquee/xbl-marquee.xml; new revision: 1.19;

Comment 17

[Martijn Wargers (dead)]

Ok, patch3 is checked in, I'm marking this bug fixed.