Summary: | Use Document::createElement(const QualifiedName&, bool) when creating a known element inside WebCore | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | ap | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Julien Chaffraix
2009-02-12 17:44:30 PST
Created attachment 27635 [details]
Proposed changes
Comment on attachment 27635 [details]
Proposed changes
@@ -969,9 +969,8 @@ void Document::setTitle(const String& ti
m_titleElement = 0;
else if (!m_titleElement) {
if (HTMLElement* headElement = head()) {
+ m_titleElement = createElement(titleTag, false);
ExceptionCode ec = 0;
- m_titleElement = createElement("title", ec);
I think that this changes the behavior for SVG documents - previously, QualifiedName(nullAtom, "title", nullAtom) was used via createElement(). Is this intentional? Please add a test case for this regardless.
- RefPtr<Element> reportElement = doc->createElementNS(HTMLNames::xhtmlNamespaceURI, "parsererror", ec);
- reportElement->setAttribute(HTMLNames::styleAttr, "display: block; white-space: pre; border: 2px solid #c77; padding: 0 1em 0 1em; margin: 1em; background-color: #fdd; color: black");
+ RefPtr<Element> reportElement = doc->createElement(QualifiedName(nullAtom, "parsererror", xhtmlNamespaceURI), true);
This changes createdByParser from false to true - is it intentional? I don't see how that is correct.
Same question elsewhere in error header creation code.
- // make the span to hold the tab
+ // Make the span to hold the tab
It's great to fix comments - we also prefer to have periods at the end of full sentences.
I like this patch a lot! r- for now, to consider the comments.
(In reply to comment #2) > (From update of attachment 27635 [details] [review]) > @@ -969,9 +969,8 @@ void Document::setTitle(const String& ti > m_titleElement = 0; > else if (!m_titleElement) { > if (HTMLElement* headElement = head()) { > + m_titleElement = createElement(titleTag, false); > ExceptionCode ec = 0; > - m_titleElement = createElement("title", ec); > > I think that this changes the behavior for SVG documents - previously, > QualifiedName(nullAtom, "title", nullAtom) was used via createElement(). Is > this intentional? Please add a test case for this regardless. Yes, it is. We are creating a title tag for an HTML document (there is a "if(!HTMLDocument()) ... else" guard above). I can add a test case if you request it in light of my point. As you were pointing out, there will be a change in behaviour for all my createElement(const AtomicString&, ...) -> createElement(const QualifiedName&, bool) changes. I have double-checked all the changes and the only problematic change I see is in DragController - the other changes are (X)HTML-specific. I do not think that creating an element in the null namespace is right either. > - RefPtr<Element> reportElement = > doc->createElementNS(HTMLNames::xhtmlNamespaceURI, "parsererror", ec); > - reportElement->setAttribute(HTMLNames::styleAttr, "display: block; > white-space: pre; border: 2px solid #c77; padding: 0 1em 0 1em; margin: 1em; > background-color: #fdd; color: black"); > + RefPtr<Element> reportElement = doc->createElement(QualifiedName(nullAtom, > "parsererror", xhtmlNamespaceURI), true); > > This changes createdByParser from false to true - is it intentional? I don't > see how that is correct. > > Same question elsewhere in error header creation code. > This is called from the parser so I have though it was correct to set createdByParser to true. I have no strong feelings about the value given thought. > - // make the span to hold the tab > + // Make the span to hold the tab > > It's great to fix comments - we also prefer to have periods at the end of full > sentences. Right, I have missed that. (In reply to comment #3) > Yes, it is. We are creating a title tag for an HTML document (there is a > "if(!HTMLDocument()) ... else" guard above). I can add a test case if you > request it in light of my point. My mistake. I don't think this needs a test case now. > the only problematic > change I see is in DragController - the other changes are (X)HTML-specific. I think DragController is fine, too -the results was casted to HTMLAnchorElement* anyway. I wonder if the cast can be avoided altogether by creating the element more directly. > This is called from the parser so I have though it was correct to set > createdByParser to true. I have no strong feelings about the value given > thought. Let's change it back then - from my understanding of the code, "createdByParser" is tightly coupled to the parser behavior, which is unlikely to be followed here. > > the only problematic > > change I see is in DragController - the other changes are (X)HTML-specific. > > I think DragController is fine, too -the results was casted to > HTMLAnchorElement* anyway. I wonder if the cast can be avoided altogether by > creating the element more directly. Right, I have missed the cast. I will make this change too as a short-term enhancement but I feel more and more that such manual creation are bad and will backfire on us someday. We should have the Elements' factories exports a way to create a "default" element using the static names. > Let's change it back then - from my understanding of the code, > "createdByParser" is tightly coupled to the parser behavior, which is unlikely > to be followed here. Ok. Created attachment 27744 [details]
Updated version: should solve Alexey's comments
Comment on attachment 27744 [details]
Updated version: should solve Alexey's comments
r=me
|