Bug 22564

Summary: Make HTML elements' constructors take a QualifiedName
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, koivisto
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed changes - First try
eric: review+
2nd part: Update the remaining constructors eric: review+

Description Julien Chaffraix 2008-11-30 06:09:23 PST
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.
Comment 1 Julien Chaffraix 2008-11-30 06:17:56 PST
Created attachment 25609 [details]
Proposed changes - First try
Comment 2 Eric Seidel (no email) 2008-11-30 14:23:52 PST
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.
Comment 3 Julien Chaffraix 2008-12-01 16:27:12 PST
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).
Comment 4 Julien Chaffraix 2008-12-02 14:57:08 PST
Re-opening the bug as I have closed it a bit too fast (there is a more constructors to update).
Comment 5 Julien Chaffraix 2008-12-02 14:58:46 PST
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.
Comment 6 Julien Chaffraix 2008-12-02 15:03:18 PST
Created attachment 25685 [details]
2nd part: Update the remaining constructors
Comment 7 Eric Seidel (no email) 2008-12-02 15:28:19 PST
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!
Comment 8 Julien Chaffraix 2008-12-04 14:00:05 PST
(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!
Comment 9 Julien Chaffraix 2008-12-04 14:40:00 PST
Landed in r39007.