RESOLVED WONTFIX153342
Document::createElement has too many function overrides
https://bugs.webkit.org/show_bug.cgi?id=153342
Summary Document::createElement has too many function overrides
Ryosuke Niwa
Reported 2016-01-21 21:34:36 PST
Document::createElement has one variant exposed to DOM and another used for internal element creation. Rename the one used internally to createBuiltinElement and add createBuiltinHTMLElement.
Attachments
Cleanup (39.63 KB, patch)
2016-01-21 21:47 PST, Ryosuke Niwa
mcatanzaro: review-
Ryosuke Niwa
Comment 1 2016-01-21 21:47:29 PST
Darin Adler
Comment 2 2016-01-22 17:51:01 PST
Comment on attachment 269550 [details] Cleanup View in context: https://bugs.webkit.org/attachment.cgi?id=269550&action=review built-in is two words so it should be spelled createBuiltInHTMLElement > Source/WebCore/editing/IndentOutdentCommand.cpp:75 > + RefPtr<HTMLElement> newList = document().createBuiltinHTMLElement(listNode->tagQName()); Type should be Ref here, not RefPtr. Better, use auto. > Source/WebCore/editing/htmlediting.cpp:983 > + RefPtr<HTMLElement> spanElement = document.createBuiltinHTMLElement(spanTag); Ditto. > Source/WebCore/editing/cocoa/EditorCocoa.mm:60 > + Ref<HTMLElement> styleElement = frame->document()->createBuiltinHTMLElement(HTMLNames::spanTag); Better to use auto. > Source/WebCore/editing/ios/EditorIOS.mm:494 > + Ref<HTMLElement> anchor = frame.document()->createBuiltinHTMLElement(HTMLNames::aTag); Ditto. > Source/WebCore/editing/ios/EditorIOS.mm:583 > + Ref<HTMLElement> imageElement = m_frame.document()->createBuiltinHTMLElement(HTMLNames::imgTag); Ditto. > Source/WebCore/editing/mac/EditorMac.mm:592 > + Ref<HTMLElement> anchor = frame.document()->createBuiltinHTMLElement(HTMLNames::aTag); Ditto. > Source/WebCore/editing/mac/EditorMac.mm:633 > + Ref<HTMLElement> imageElement = document().createBuiltinHTMLElement(HTMLNames::imgTag); Ditto. > Source/WebCore/html/FTPDirectoryDocument.cpp:107 > + Ref<HTMLElement> element = document()->createBuiltinHTMLElement(tdTag); Ditto. > Source/WebCore/html/FTPDirectoryDocument.cpp:138 > + Ref<HTMLElement> anchorElement = document()->createBuiltinHTMLElement(aTag); Ditto. > Source/WebCore/html/FTPDirectoryDocument.cpp:142 > + Ref<HTMLElement> tdElement = document()->createBuiltinHTMLElement(tdTag); Ditto. > Source/WebCore/html/FTPDirectoryDocument.cpp:328 > + Ref<HTMLElement> bodyElement = document()->createBuiltinHTMLElement(bodyTag); Ditto. > Source/WebCore/html/FTPDirectoryDocument.cpp:332 > + Ref<HTMLElement> tableElement = document()->createBuiltinHTMLElement(tableTag); Ditto. > Source/WebCore/html/HTMLSelectElement.cpp:479 > + Ref<HTMLElement> option = HTMLOptionElement::create(document()); > + add(option.ptr(), nullptr, ec); How about doing this as a one-liner? add(HTMLOptionElement::create(document()).ptr(), nullptr, ec); > Source/WebCore/html/ImageDocument.cpp:213 > + Ref<HTMLElement> rootElement = Document::createBuiltinHTMLElement(htmlTag); How about auto? > Source/WebCore/html/ImageDocument.cpp:219 > + Ref<HTMLElement> body = Document::createBuiltinHTMLElement(bodyTag); Ditto. > Source/WebCore/html/MediaDocument.cpp:81 > + Ref<HTMLElement> rootElement = document()->createBuiltinHTMLElement(htmlTag); Ditto. > Source/WebCore/html/MediaDocument.cpp:90 > + Ref<HTMLElement> headElement = document()->createBuiltinHTMLElement(headTag); Ditto. > Source/WebCore/html/MediaDocument.cpp:93 > + Ref<HTMLElement> metaElement = document()->createBuiltinHTMLElement(metaTag); Ditto. > Source/WebCore/html/MediaDocument.cpp:99 > + Ref<HTMLElement> body = document()->createBuiltinHTMLElement(bodyTag); Ditto. > Source/WebCore/html/MediaDocument.cpp:102 > + Ref<HTMLElement> mediaElement = document()->createBuiltinHTMLElement(videoTag); Ditto. > Source/WebCore/html/MediaDocument.cpp:117 > + Ref<HTMLElement> sourceElement = document()->createBuiltinHTMLElement(sourceTag); Ditto. > Source/WebCore/html/MediaDocument.cpp:243 > + Ref<HTMLEmbedElement> embedElement = HTMLEmbedElement::create(embedTag, *this, false); Ditto. > Source/WebCore/html/PluginDocument.cpp:70 > + Ref<HTMLHtmlElement> rootElement = HTMLHtmlElement::create(*document()); Ditto. > Source/WebCore/html/PluginDocument.cpp:82 > + Ref<HTMLElement> body = document()->createBuiltinHTMLElement(bodyTag); Ditto. > Source/WebCore/html/PluginDocument.cpp:93 > + Ref<HTMLElement> embedElement = document()->createBuiltinHTMLElement(embedTag); Ditto. > Source/WebCore/html/parser/HTMLConstructionSite.cpp:621 > + RefPtr<Element> element = ownerDocumentForCurrentNode().createBuiltinElement(tagName, true); Should be Ref, not RefPtr. > Source/WebCore/html/shadow/MediaControlElements.cpp:808 > + Ref<HTMLElement> captionsHeader = document().createBuiltinHTMLElement(h3Tag); Ditto. > Source/WebCore/html/shadow/MediaControlElements.cpp:811 > + Ref<HTMLElement> captionsMenuList = document().createBuiltinHTMLElement(ulTag); Ditto. > Source/WebCore/html/shadow/MediaControlElements.cpp:814 > + Ref<HTMLElement> menuItem = document().createBuiltinHTMLElement(liTag); Ditto. > Source/WebCore/xml/XMLErrors.cpp:89 > + Ref<Element> reportElement = doc->createBuiltinElement(QualifiedName(nullAtom, "parsererror", xhtmlNamespaceURI), true); Ditto. > Source/WebKit2/WebProcess/Plugins/PDF/DeprecatedPDFPlugin.mm:527 > + Ref<HTMLElement> annotationStyleElement = document->createBuiltinHTMLElement(styleTag); Ditto. > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:520 > + Ref<HTMLElement> annotationStyleElement = document->createBuiltinHTMLElement(styleTag); Ditto. > Source/WebKit2/WebProcess/Plugins/PDF/PDFPlugin.mm:797 > + m_passwordContainer = document->createBuiltinHTMLElement(divTag); Ditto. > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm:74 > + Ref<HTMLElement> styledElement = document.createBuiltinHTMLElement(selectTag); Ditto. > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm:84 > + Ref<HTMLElement> choiceOption = document.createBuiltinHTMLElement(optionTag); Ditto. > Source/WebKit2/WebProcess/Plugins/PDF/PDFPluginChoiceAnnotation.mm:94 > + return styledElement.ptr(); I think this is wrong. It should be WTFMove(styledElement), I think.
Michael Catanzaro
Comment 3 2016-02-15 13:08:29 PST
Comment on attachment 269550 [details] Cleanup I'm not really rejecting this patch, just getting it off request queue until you have time to respond to Darin's review.
Note You need to log in before you can comment on or make changes to this bug.