[webkit-reviews] review granted: [Bug 137007] Use downcast<HTML*Element>() instead of toHTML*Element() : [Attachment 238543] Patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Chris Dumez
<cdumez at apple.com>'s request for review:
Bug 137007: Use downcast<HTML*Element>() instead of toHTML*Element()
https://bugs.webkit.org/show_bug.cgi?id=137007

Attachment 238543: Patch
https://bugs.webkit.org/attachment.cgi?id=238543&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
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.