Bug 22564 – Make HTML elements' constructors take a QualifiedName

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Julien Chaffraix]

This is a follow-up of bug22441. As requested on the previous bug, we will add the required assertions while doing the changes.

Patch following.

[Julien Chaffraix]

Created attachment 25609 [details]
Proposed changes - First try

[Eric Seidel (no email)]

Comment on attachment 25609 [details]
Proposed changes - First try

I find this pattern a bit strange:

     if (!MediaPlayer::isAvailable())
         return new HTMLElement(tagName, doc);
-    return new HTMLVideoElement(doc);
+    return new HTMLVideoElement(videoTag, doc);

As antti pointed out before, that will break any uses of if (element->hasTagName(videoTag) static_cast<HTMLVideoElement*>(element)...

Agreed, we should always be using something like element->isVideoElement() instead.

The patch looks fine.

[Julien Chaffraix]

Landed patch in r38881.

(In reply to comment #2)
> (From update of attachment 25609 [details] [review])
> I find this pattern a bit strange:
> 
>      if (!MediaPlayer::isAvailable())
>          return new HTMLElement(tagName, doc);
> -    return new HTMLVideoElement(doc);
> +    return new HTMLVideoElement(videoTag, doc);
> 
> As antti pointed out before, that will break any uses of if
> (element->hasTagName(videoTag) static_cast<HTMLVideoElement*>(element)...
> 
> Agreed, we should always be using something like element->isVideoElement()
> instead.

This will also break <source> and <audio> which do not have a custom isXXXElement() method. I have filed bug22578 about that to keep this information easily available (especially since the broken pattern is used in WebCore).

[Julien Chaffraix]

Re-opening the bug as I have closed it a bit too fast (there is a more constructors to update).

[Julien Chaffraix]

Comment on attachment 25609 [details]
Proposed changes - First try

Marking the committed first version as obsolete so that it does not show on the commit queue.

[Julien Chaffraix]

Created attachment 25685 [details]
2nd part: Update the remaining constructors

[Eric Seidel (no email)]

Comment on attachment 25685 [details]
2nd part: Update the remaining constructors

Might as well make this one line:
 static PassRefPtr<HTMLElement> objectConstructor(const QualifiedName&, Document* doc, HTMLFormElement*, bool createdByParser)
 {
-    RefPtr<HTMLObjectElement> object = new HTMLObjectElement(doc, createdByParser);
+    RefPtr<HTMLObjectElement> object = new HTMLObjectElement(objectTag, doc, createdByParser);
     return object.release();
 }

otherwise looks great!

[Julien Chaffraix]

(In reply to comment #7)
> (From update of attachment 25685 [details] [review])
> Might as well make this one line:
>  static PassRefPtr<HTMLElement> objectConstructor(const QualifiedName&,
> Document* doc, HTMLFormElement*, bool createdByParser)
>  {
> -    RefPtr<HTMLObjectElement> object = new HTMLObjectElement(doc,
> createdByParser);
> +    RefPtr<HTMLObjectElement> object = new HTMLObjectElement(objectTag, doc,
> createdByParser);
>      return object.release();
>  }
> 

Good catch! However I will need to sort out what to do with those createdByParser boolean before rolling out HTMLElementFactory (there is currently 2 syntax that should be merged somehow). I will keep this line as-is for now.

> otherwise looks great!

Thanks!

[Julien Chaffraix]

Landed in r39007.