Closed Bug 811753 Opened 12 years ago Closed 12 years ago

Add nsIContent::IsEventAttributeName


(Core :: DOM: Events, defect)

Not set





(Reporter: bzbarsky, Assigned: jlevesy)



(Whiteboard: [mentor=bz][lang=c++])


(1 file, 4 obsolete files)

Right now we have various places where we call nsContentUtils::IsEventAttributeName with various flags, and those flags depend on the element we're dealing with.  We should just have a virtual nsIContent::IsEventAttributeName which will call nsContentUtils with the right flags for that element.
Whiteboard: [mentor=bz] → [mentor=bz][lang=c++]
Assignee: nobody → jlevesy
Need feedback on this patch for validating my approach. Do not try to compile.

My main problem is that i don't know class model very well, and this is a huge model i think. So it's difficult to me to determine where i have to implement the IsEventAttributeName method.
I don't want to make useless code duplications.
I'll try to do my best to deal with that !
Attachment #695260 - Flags: feedback?(bzbarsky)
Comment on attachment 695260 [details]
Need feedback on this patch for validating my approach. Do not try to compile.

So I think the right thing is to have IsEventAttributeName on nsIContent not be pure virtual and just return false.

You'll then need overrides on nsGenericHTMLElement (which all HTML elements inherit from), nsXULElement, nsSVGElement (which all SVG elements inherit from), and probably nsHTMLBodyElement and nsHTMLFrameSetElement because some event attribute names are only supported on those.

Comments and text nodes and document fragments and document types and so forth can't have attributes, so always returning false for those is fine.

Thank you for working on this!
Attachment #695260 - Flags: feedback?(bzbarsky) → feedback+
Ok. Thanks for your complete answer.
I'll propose a patch soon !
Attached patch bug-811753-fix proposition (obsolete) — Splinter Review
Here it is. 
I also changed calls in nsXMLContentSerializer::IsJavaScript method.
Is there any test case to write ? execute ?
Attachment #695309 - Flags: review?(bzbarsky)
Attachment #695309 - Attachment is patch: true
Comment on attachment 695309 [details] [diff] [review]
bug-811753-fix proposition

Sorry for the lag here, holidays...

This is a patch on top of the first patch in this bug, right?  At some point we'll need a combined patch against current tip for check-in.

>+   * @note base behaviour return false, it is overrided 

"overridden by subclasses as needed"?

nssXMLContentSerializer::IsJavaScript needs to return the return value of IsEventAttributeName.

>+  return nsContentUtils::IsEventAttributeName(aName,EventNameType_HTML);

Space after comma, please.

>+++ b/content/html/content/src/nsGenericHTMLElement.h

I don't think you need to copy/paste the API documentation comment here, especially since it's not actually true as written for this class.  Just remove the comment, please.  Same for the other headers where this comment ended up.

>+  return nsContentUtils::IsEventAttributeName(aName,EventNameType_HTML |
>+                                                EventNameType_HTMLBodyOrFramesetOnly);

Space after comma, and fix the indent please, in both body and frameset code.

You probably want to have an SVG peer look at the nsSVGElement change now that I look at it more closely.  In particular, SVG elements already have an IsEventName function that does what we want, I think; it might be enough to rename it.

>+  return nsContentUtils::IsEventAttributeName(aName,EventNameType_XUL);

Space after comma, please.

You can simplify the code in nsGenericHTMLElement::AfterSetAttr to use this new function, I believe.

r=me with the nits above picked, on all but the SVG bit.
Attachment #695309 - Flags: review?(longsonr)
Attachment #695309 - Flags: review?(bzbarsky)
Attachment #695309 - Flags: review+
Comment on attachment 695309 [details] [diff] [review]
bug-811753-fix proposition

>+nsSVGElement::IsEventAttributeName(nsIAtom *aName)
>+  return nsContentUtils::IsEventAttributeName(aName,
>+    EventNameType_SVGGraphic |
>+    EventNameType_SVGSVG |
>+    EventNameType_SMIL);

Here's the table of what goes where for SVG: check out the onxxx attributes.

EventNameType_SMIL should go on animation elements, which it does in but it shouldn't go on other SVG elements which means the flag should come out of nsSVGElement::IsEventAttributeName

EventNameType_SVGGraphic should go on some but not all SVG elements (again, it does in various places e.g. so that should not be here either.

Which leaves EventNameType_SVGSVG which occurs only on the <svg> element of all the SVG elements so that should be set only in nsSVGSVGElement::IsEventAttributeName, and again that is so.

So in conclusion we don't want any of these flags set with nsSVGElement::IsEventAttributeName. Which means, we don't want the method at all here.

r=longsonr with nsSVGElement::IsEventAttributeName removed.
Attachment #695309 - Flags: review?(longsonr) → review+
Well, it needs to be replaced by IsEventAtributeName overloads on other SVG elements, right?
Such things already exist.
> Such things already exist.

Ahh you're right of course, all the SVG instances need to be renamed from IsEventAttributeName to IsEventName . Basically everything in here:

Preferably with a MOZ_OVERRIDE modifier post override too.
Thanks for answers.

Sorry about "stupid" mistakes(spaces after comma, or duplicated comments...)

Robert, you mean from IsEventName to IsEventAttributeName ?  (to enable polymorphism on those classes ?) and remove the nsSVGElement:IsEventName hook ? 

I'll put MOZ_OVERRIDE modifier on all overriden methods. 

(In reply to Julien Levesy from comment #11)
> Robert, you mean from IsEventName to IsEventAttributeName ?  (to enable
> polymorphism on those classes ?) and remove the nsSVGElement:IsEventName
> hook ? 

Yes to both.
Attached patch Bug 811753 patch proposal, r2. (obsolete) — Splinter Review
Here is a patch proposal.
I ran local test suite, it leads on an unexpected failure.

/tests/Harness_sanity/test_sanityEventUtils.html | [SimpleTest.finish()] waitForFocus() was called a different number of times from the number of callbacks run.  Maybe the test terminated prematurely -- be sure to use SimpleTest.waitForExplicitFinish(). - got 1, expected 0

I'm sorry but i need a little help here.
Thanks !
Attachment #695260 - Attachment is obsolete: true
Attachment #695309 - Attachment is obsolete: true
Attachment #696892 - Flags: review?(bzbarsky)
Comment on attachment 696892 [details] [diff] [review]
Bug 811753 patch proposal, r2.

>diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h
>--- a/content/html/content/src/nsGenericHTMLElement.h
>+++ b/content/html/content/src/nsGenericHTMLElement.h
>@@ -1,9 +1,9 @@
>-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

I think this is a mistake. Won't help you pass the test though unfortunately.
Attached patch Bug 811753 patch proposal, r2. (obsolete) — Splinter Review
I accidentaly removed a header line on the previous patch.
Attachment #696892 - Attachment is obsolete: true
Attachment #696892 - Flags: review?(bzbarsky)
Attachment #696894 - Flags: review?(bzbarsky)
> I ran local test suite, it leads on an unexpected failure.

Hmm.  The only event attribute in there is the onload="starttest()" on the <body>, which is clearly getting called.

That said, the test as written looks bogus.  The "SimpleTest.waitForExplicitFinish();" should be the first line of starttest(), not the first line of the waitForFocus callback.  Does doing that fix things for you locally?
Comment on attachment 696894 [details] [diff] [review]
Bug 811753 patch proposal, r2.

>+++ b/content/base/public/nsIContent.h
>+    * a given element type. Types are from the EventNameType enumeration
>+    * defined above.

Please drop the sentence starting with "Types", since it's nonsensical here.

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>@@ -797,17 +797,17 @@ nsGenericHTMLElement::AfterSetAttr(int32

>     uint32_t eventType = EventNameType_HTML;
>     if (mNodeInfo->Equals(nsGkAtoms::body) ||
>         mNodeInfo->Equals(nsGkAtoms::frameset)) {
>       eventType |= EventNameType_HTMLBodyOrFramesetOnly;
>     }

All of that calculation of eventType is now dead code and can go away, right?

>+    if (IsEventAttributeName(aName) &&
>         aValue) {

Please pull up aValue to the same line as the first part of the condition now that it fits.

r=me with that.  Thank you again!

I'll push this patch to try to see what tests look like.
Attachment #696894 - Flags: review?(bzbarsky) → review+
Attachment #696894 - Attachment is obsolete: true
Attachment #697142 - Flags: review?(bzbarsky)
Comment on attachment 697142 [details] [diff] [review]
Bug 811753 patch proposal, r3.


Thank you again for the patch!  Try looks green; I'll push this.
Attachment #697142 - Flags: review?(bzbarsky) → review+

Julien, thanks for sticking with this!
Flags: in-testsuite-
Target Milestone: --- → mozilla20
You're welcome !
Thanks both for help !
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 869657
Blocks: 869657
No longer depends on: 869657
You need to log in before you can comment on or make changes to this bug.