Bug 22564

Created attachment 25609 [details] Proposed changes - First try

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.

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).

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

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.

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

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!

(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!

Landed in r39007.