Bug 137007 – Use downcast<HTML*Element>() instead of toHTML*Element()

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

[Chris Dumez]

Use downcast<HTML*Element>() instead of toHTML*Element() and get rid of the transition macros for HTML Elements.

[Chris Dumez]

Created attachment 238517 [details]
Patch

[Chris Dumez]

Created attachment 238519 [details]
Patch

[Chris Dumez]

Created attachment 238541 [details]
Patch

[Chris Dumez]

Created attachment 238543 [details]
Patch

[Benjamin Poulain]

Comment on attachment 238543 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238543&action=review

Some comments are completely unrelated to the change, please don't fix them in the same patch.

> Source/WebCore/ChangeLog:405
> +        Generate helpers to SVGAElement.

Please describe "why?" in addition to "what?".

> Source/WebCore/accessibility/AccessibilityMenuListOption.cpp:86
> -    toHTMLOptionElement(m_element.get())->setSelected(b);
> +    downcast<HTMLOptionElement>(*m_element).setSelected(b);

b ->isSelected/selected?

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1754
>      if (isNativeTextControl() && (isHTMLTextAreaElement(node) || isHTMLInputElement(node)))

It is weird this checks for (isHTMLTextAreaElement(node) || isHTMLInputElement(node)) instead of isHTMLTextFormControlElement()...

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:969
>          unsigned len = list->length();
>          for (unsigned i = 0; i < len; ++i) {

len -> length.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:970
>              if (isHTMLInputElement(list->item(i))) {

list->item(i) should go in a temporary, NodeList isn't exactly cheap.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1690
>          // FIXME: This is not safe!  Other elements could have a TextField renderer.

I suggest adding a RELEASE_ASSERT() or a if() here for the type of element.

At a minimum, an ASSERT().

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1693
>          // FIXME: This is not safe!  Other elements could have a TextArea renderer.

ditto.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2523
> -        const AtomicString& type = input->getAttribute(typeAttr);
> +        const AtomicString& type = input.getAttribute(typeAttr);
>          if (equalIgnoringCase(type, "color"))

This is really strange. I wonder why not use isColorControl() directly.

> Source/WebCore/accessibility/AccessibilityTable.cpp:613
> +        HTMLTableCaptionElement* caption = downcast<HTMLTableElement>(*tableElement).caption();
>          if (caption)
>              title = caption->innerText();

if (HTMLTableCaptionElement* caption = downcast<HTMLTableElement>(*tableElement).caption())
    title = caption->innerText();

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:-145
> -    if (!select)
> -        return;

Good catch.

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:182
> -        String summary = toHTMLTableElement(node)->summary();
> +        String summary = downcast<HTMLTableElement>(*node).summary();

String -> const AtomicString& to avoid the implicit conversion and ref()+deref().

> Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:-72
> -    if (!inputElement)
> -        return false;

Another good catch.

> Source/WebCore/dom/DataTransfer.cpp:260
>      CachedImage* image;
>      if (element && isHTMLImageElement(element) && !element->inDocument())
> -        image = toHTMLImageElement(element)->cachedImage();
> +        image = downcast<HTMLImageElement>(*element).cachedImage();
>      else
> -        image = 0;
> +        image = nullptr;

For simplicity, this could be:
    CachedImage* image = nullptr;
    if (element && isHTMLImageElement(element) && !element->inDocument())
        image = downcast<HTMLImageElement>(*element).cachedImage();

> Source/WebCore/editing/Editor.cpp:335
>      if (!document.isImageDocument())
> -        return 0;
> +        return nullptr;
>      
>      HTMLElement* body = document.body();
>      if (!body)
> -        return 0;
> +        return nullptr;
>      
>      Node* node = body->firstChild();
>      if (!node)
> -        return 0;
> +        return nullptr;
>      if (!isHTMLImageElement(node))
> -        return 0;
> -    return toHTMLImageElement(node);
> +        return nullptr;
> +    return downcast<HTMLImageElement>(node);

This whole thing should be 2 lines with ImageDocument::imageElement().

+ a test obviously :)

> Source/WebCore/html/ColorInputType.cpp:228
>      HTMLDataListElement* dataList = element().dataList();

I did not know we had datalist. We should totally finish the implementation and enable the flag.

> Source/WebCore/html/ColorInputType.cpp:231
> -        for (unsigned i = 0; HTMLOptionElement* option = toHTMLOptionElement(options->item(i)); i++) {
> +        for (unsigned i = 0; HTMLOptionElement* option = downcast<HTMLOptionElement>(options->item(i)); i++) {

i++ -> ++i

Gosh, we should have an iterator for HTMLCollection, this for() loop is disgusting.

> Source/WebCore/html/FormAssociatedElement.cpp:103
> -        HTMLFormElement* newForm = 0;
> +        HTMLFormElement* newForm = nullptr;
>          Element* newFormCandidate = element->treeScope().getElementById(formId);
>          if (newFormCandidate && isHTMLFormElement(newFormCandidate))
> -            newForm = toHTMLFormElement(newFormCandidate);
> +            newForm = downcast<HTMLFormElement>(newFormCandidate);
>          return newForm;

I wonder if there is a test for <label> with a for="id" where there are multiple #id... if not we should have one.

> Source/WebCore/html/HTMLElement.cpp:430
>          if (hasTagName(templateTag))

Should this be is<HTMLTemplateElement>(templateTag)?

> Source/WebCore/html/HTMLElement.cpp:920
>  TextDirection HTMLElement::directionality(Node** strongDirectionalityTextNode) const

Node**...you don't see those every day

> Source/WebCore/html/HTMLInputElement.cpp:1856
> -        for (unsigned i = 0; HTMLOptionElement* option = toHTMLOptionElement(options->item(i)); ++i) {
> +        for (unsigned i = 0; HTMLOptionElement* option = downcast<HTMLOptionElement>(options->item(i)); ++i) {

arg :(

> Source/WebCore/html/MediaDocument.cpp:103
>      m_mediaElement->setAttribute(controlsAttr, "");
>      m_mediaElement->setAttribute(autoplayAttr, "");

"" -> emptyAtom

> Source/WebCore/rendering/HitTestResult.cpp:347
> -    if (element->serviceType() == "application/pdf" || (element->serviceType().isEmpty() && url.path().lower().endsWith(".pdf")))
> +    if (element.serviceType() == "application/pdf" || (element.serviceType().isEmpty() && url.path().lower().endsWith(".pdf")))

url.path().lower().endsWith(".pdf") does an allocation for the lower() string before doing endsWith.

endsWith() can be really cheap, allocation aren't. This should use the case insensitive endsWidth() instead of transforming the string.

This also mean we can lower() 4 characters instead of an entire string.

> Source/WebCore/rendering/RenderEmbeddedObject.cpp:243
>      if (snapshot)
>          paintSnapshotImage(paintInfo, paintOffset, snapshot);

if (Image* snapshot = downcast<HTMLPlugInImageElement>(plugInElement).snapshotImage())
    paintSnapshotImage(paintInfo, paintOffset, snapshot);

> Source/WebCore/rendering/RenderMenuList.cpp:66
>      for (size_t i = 0; i < numberOfItems; ++i) {

We don't really need numberOfItems, it could be numberOfItems->listItems.size()

> Source/WebKit2/WebProcess/WebPage/ios/WebPageIOS.mm:2072
> -        size_t count = element->listItems().size();
> +        size_t count = element.listItems().size();

Let's put element.listItems() in a temporary.

This code should not modify the listItem.

[Chris Dumez]

Created attachment 238568 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 238568 [details]
Patch

Clearing flags on attachment: 238568

Committed r173893: <http://trac.webkit.org/changeset/173893>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Debug build fix landed in http://trac.webkit.org/changeset/173895

[Daniel Bates]

(In reply to comment #7)
> (From update of attachment 238568 [details])
> Clearing flags on attachment: 238568
> 
> Committed r173893: <http://trac.webkit.org/changeset/173893>

This broke the iOS build. Committed fix in <http://trac.webkit.org/changeset/173923>.