Summary: | Make HTML elements' constructors take a QualifiedName | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | eric, koivisto | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Julien Chaffraix
2008-11-30 06:09:23 PST
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! |