Bug 153342

Summary: Document::createElement has too many function overrides
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED WONTFIX    
Severity: Normal    
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 150225    
Attachments:
Description Flags
Cleanup mcatanzaro: review-

Description Ryosuke Niwa 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.
Comment 1 Ryosuke Niwa 2016-01-21 21:47:29 PST 
Created attachment 269550 [details]
Cleanup
Comment 2 Darin Adler 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.
Comment 3 Michael Catanzaro 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.