Last Comment Bug 811753 - Add nsIContent::IsEventAttributeName
: Add nsIContent::IsEventAttributeName
Status: RESOLVED FIXED
[mentor=bz][lang=c++]
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla20
Assigned To: Julien Levesy
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 869657
  Show dependency treegraph
 
Reported: 2012-11-14 08:48 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2013-05-08 14:03 PDT (History)
6 users (show)
bzbarsky: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Need feedback on this patch for validating my approach. Do not try to compile. (7.70 KB, text/plain)
2012-12-22 14:46 PST, Julien Levesy
bzbarsky: feedback+
Details
bug-811753-fix proposition (18.71 KB, patch)
2012-12-23 07:55 PST, Julien Levesy
bzbarsky: review+
longsonr: review+
Details | Diff | Splinter Review
Bug 811753 patch proposal, r2. (29.96 KB, patch)
2013-01-01 13:06 PST, Julien Levesy
no flags Details | Diff | Splinter Review
Bug 811753 patch proposal, r2. (29.50 KB, patch)
2013-01-01 14:10 PST, Julien Levesy
bzbarsky: review+
Details | Diff | Splinter Review
Bug 811753 patch proposal, r3. (29.60 KB, patch)
2013-01-02 12:31 PST, Julien Levesy
bzbarsky: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-11-14 08:48:46 PST
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.
Comment 1 Julien Levesy 2012-12-22 14:46:47 PST
Created attachment 695260 [details]
Need feedback on this patch for validating my approach. Do not try to compile.

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 !
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-12-22 15:32:54 PST
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!
Comment 3 Julien Levesy 2012-12-23 03:51:18 PST
Ok. Thanks for your complete answer.
I'll propose a patch soon !
Comment 4 Julien Levesy 2012-12-23 07:55:38 PST
Created attachment 695309 [details] [diff] [review]
bug-811753-fix proposition

Here it is. 
I also changed calls in nsXMLContentSerializer::IsJavaScript method.
Is there any test case to write ? execute ?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-12-28 14:04:59 PST
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.
Comment 6 Robert Longson 2012-12-29 00:55:12 PST
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.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-12-29 09:13:34 PST
Well, it needs to be replaced by IsEventAtributeName overloads on other SVG elements, right?
Comment 8 Robert Longson 2012-12-29 15:00:00 PST
Such things already exist.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-12-29 22:03:48 PST
> Such things already exist.

Where?
Comment 10 Robert Longson 2012-12-29 23:44:43 PST
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.
Comment 11 Julien Levesy 2012-12-31 02:32:27 PST
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 Robert Longson 2012-12-31 03:18:23 PST
(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.
Comment 13 Julien Levesy 2013-01-01 13:06:27 PST
Created attachment 696892 [details] [diff] [review]
Bug 811753 patch proposal, r2.

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 !
Comment 14 Robert Longson 2013-01-01 13:55:32 PST
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.
Comment 15 Julien Levesy 2013-01-01 14:10:25 PST
Created attachment 696894 [details] [diff] [review]
Bug 811753 patch proposal, r2.

I accidentaly removed a header line on the previous patch.
Sorry.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2013-01-02 10:08:17 PST
> 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 17 Boris Zbarsky [:bz] (still a bit busy) 2013-01-02 10:19:38 PST
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.
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2013-01-02 10:51:31 PST
Try push: https://tbpl.mozilla.org/?tree=Try&rev=9bc03b452959
Comment 19 Julien Levesy 2013-01-02 12:31:01 PST
Created attachment 697142 [details] [diff] [review]
Bug 811753 patch proposal, r3.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2013-01-02 18:23:57 PST
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.
Comment 21 Boris Zbarsky [:bz] (still a bit busy) 2013-01-02 19:23:33 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd897bfc2469

Julien, thanks for sticking with this!
Comment 22 Julien Levesy 2013-01-03 00:54:52 PST
You're welcome !
Thanks both for help !
Comment 23 Ed Morley [:emorley] 2013-01-04 10:04:45 PST
https://hg.mozilla.org/mozilla-central/rev/dd897bfc2469

Note You need to log in before you can comment on or make changes to this bug.