Use downcast<HTML*Element>() instead of toHTML*Element() and get rid of the transition macros for HTML Elements.
Created attachment 238517 [details] Patch
Created attachment 238519 [details] Patch
Created attachment 238541 [details] Patch
Created attachment 238543 [details] Patch
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.
Created attachment 238568 [details] Patch
Comment on attachment 238568 [details] Patch Clearing flags on attachment: 238568 Committed r173893: <http://trac.webkit.org/changeset/173893>
All reviewed patches have been landed. Closing bug.
Debug build fix landed in http://trac.webkit.org/changeset/173895
(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>.