WebKit Bugzilla
New
Browse
Search+
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
153342
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-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1
2016-01-21 21:47:29 PST
Created
attachment 269550
[details]
Cleanup
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.
Top of Page
Format For Printing
XML
Clone This Bug