Bug 23940 – Use Document::createElement(const QualifiedName&, bool) when creating a known element inside WebCore

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

[Julien Chaffraix]

Patch forthcoming.

[Julien Chaffraix]

Created attachment 27635 [details]
Proposed changes

[Alexey Proskuryakov]

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.

[Julien Chaffraix]

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

[Alexey Proskuryakov]

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

[Julien Chaffraix]

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

[Julien Chaffraix]

Created attachment 27744 [details]
Updated version: should solve Alexey's comments

[Alexey Proskuryakov]

Comment on attachment 27744 [details]
Updated version: should solve Alexey's comments

r=me

[Julien Chaffraix]

Landed in r41120.