Closed
Bug 811753
Opened 12 years ago
Closed 12 years ago
Add nsIContent::IsEventAttributeName
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
mozilla20
People
(Reporter: bzbarsky, Assigned: jlevesy)
References
Details
(Whiteboard: [mentor=bz][lang=c++])
Attachments
(1 file, 4 obsolete files)
29.60 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Whiteboard: [mentor=bz] → [mentor=bz][lang=c++]
Updated•12 years ago
|
Assignee: nobody → jlevesy
Assignee | ||
Comment 1•12 years ago
|
||
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)
Reporter | ||
Comment 2•12 years ago
|
||
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+
Assignee | ||
Comment 3•12 years ago
|
||
Ok. Thanks for your complete answer.
I'll propose a patch soon !
Assignee | ||
Comment 4•12 years ago
|
||
Here it is.
I also changed calls in nsXMLContentSerializer::IsJavaScript method.
Is there any test case to write ? execute ?
Attachment #695309 -
Flags: review?(bzbarsky)
Updated•12 years ago
|
Attachment #695309 -
Attachment is patch: true
Reporter | ||
Comment 5•12 years ago
|
||
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 6•12 years ago
|
||
Comment on attachment 695309 [details] [diff] [review]
bug-811753-fix proposition
>+
>+bool
>+nsSVGElement::IsEventAttributeName(nsIAtom *aName)
>+{
>+ return nsContentUtils::IsEventAttributeName(aName,
>+ EventNameType_SVGGraphic |
>+ EventNameType_SVGSVG |
>+ EventNameType_SMIL);
>+}
Here's the table of what goes where for SVG: http://www.w3.org/TR/SVG/attindex.html check out the onxxx attributes.
EventNameType_SMIL should go on animation elements, which it does in http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGAnimationElement.cpp#496 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. http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGSVGElement.cpp#858) 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+
Reporter | ||
Comment 7•12 years ago
|
||
Well, it needs to be replaced by IsEventAtributeName overloads on other SVG elements, right?
Comment 8•12 years ago
|
||
Such things already exist.
Reporter | ||
Comment 9•12 years ago
|
||
> Such things already exist.
Where?
Comment 10•12 years ago
|
||
Ahh you're right of course, all the SVG instances need to be renamed from IsEventAttributeName to IsEventName . Basically everything in here:
http://mxr.mozilla.org/mozilla-central/search?string=IsEventAttributeName&find=content/svg
Preferably with a MOZ_OVERRIDE modifier post override too.
Assignee | ||
Comment 11•12 years ago
|
||
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.
Thanks
Comment 12•12 years ago
|
||
(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.
Assignee | ||
Comment 13•12 years ago
|
||
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 14•12 years ago
|
||
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.
Assignee | ||
Comment 15•12 years ago
|
||
I accidentaly removed a header line on the previous patch.
Sorry.
Attachment #696892 -
Attachment is obsolete: true
Attachment #696892 -
Flags: review?(bzbarsky)
Attachment #696894 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 16•12 years ago
|
||
> 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?
Reporter | ||
Comment 17•12 years ago
|
||
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+
Reporter | ||
Comment 18•12 years ago
|
||
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #696894 -
Attachment is obsolete: true
Attachment #697142 -
Flags: review?(bzbarsky)
Reporter | ||
Comment 20•12 years ago
|
||
Comment on attachment 697142 [details] [diff] [review]
Bug 811753 patch proposal, r3.
r=me
Thank you again for the patch! Try looks green; I'll push this.
Attachment #697142 -
Flags: review?(bzbarsky) → review+
Reporter | ||
Comment 21•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd897bfc2469
Julien, thanks for sticking with this!
Flags: in-testsuite-
Target Milestone: --- → mozilla20
Assignee | ||
Comment 22•12 years ago
|
||
You're welcome !
Thanks both for help !
Comment 23•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•