Bug 137169 – Make is<>() / downcast<>() work for HTMLDocument and its subclasses

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Make is<>() / downcast<>() work for HTMLDocument By using the SPECIALIZE_TYPE_TRAITS_*() macro.

[Chris Dumez]

Created attachment 238757 [details]
Patch

[WebKit Commit Bot]

Attachment 238757 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLDocument.h:102:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/ImageDocument.h:87:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/MediaDocument.h:61:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/PluginDocument.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 238757 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238757&action=review

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:83
> +    ASSERT(is<HTMLDocument>(document));

Should be *document if we want to match the old code rather than adding a null check, but probably OK to leave it alone since it’s in an assertion.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:247
> -    if (document->isHTMLDocument()) {
> +    if (is<HTMLDocument>(document)) {

Should be *document if we want to match the old code rather than adding a null check.

> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:324
> -    if (document->isHTMLDocument()) {
> +    if (is<HTMLDocument>(document)) {

Should be *document if we want to match the old code rather than adding a null check.

> Source/WebCore/dom/Document.cpp:5908
> +    if (!element && is<PluginDocument>(doc)) {

Should be *doc if we want to match the old code rather than adding a null check.

> Source/WebCore/dom/Document.cpp:5910
> +        PluginDocument& pluginDocument = downcast<PluginDocument>(*doc);
> +        element = pluginDocument.pluginElement();

This would read better without a local variable.

> Source/WebCore/dom/Document.cpp:5912
> +    if (!element && is<HTMLDocument>(doc))

Should be *doc if we want to match the old code rather than adding a null check.

> Source/WebCore/dom/Element.cpp:1341
> +    HTMLDocument* newDocument = !wasInDocument && inDocument() && is<HTMLDocument>(newScope->documentScope()) ? downcast<HTMLDocument>(&newScope->documentScope()) : nullptr;

The & should be just before downcast, not before newScope.

> Source/WebCore/dom/Element.cpp:1384
> +        HTMLDocument* oldDocument = inDocument() && is<HTMLDocument>(oldScope->documentScope()) ? downcast<HTMLDocument>(&oldScope->documentScope()) : nullptr;

The & should be just before downcast, not before newScope.

> Source/WebCore/html/HTMLElement.cpp:384
> +        const HTMLDocument& htmlDocument = downcast<HTMLDocument>(document);
>          return htmlDocument.inDesignMode();

This would read better without a local variable.

> Source/WebCore/loader/SubframeLoader.cpp:396
> +    bool loadManually = is<PluginDocument>(document()) && !m_containsPlugins && downcast<PluginDocument>(*document()).shouldLoadPluginManually();

Should be *document() if we want to match the old code rather than adding a null check.

> Source/WebCore/page/DragController.cpp:400
> +    if (doc && is<PluginDocument>(doc)) {

Should either be passing *doc or get rid of the "doc &&" part; no need to do the null check twice.

> Source/WebKit/win/DOMHTMLClasses.cpp:261
> +    if (!m_document || !is<HTMLDocument>(m_document))

Should either be passing *m_document or get rid of the "!m_document ||" part; no need to do the null check twice.

> Source/WebKit/win/DOMHTMLClasses.cpp:304
> +    if (!m_document || !is<HTMLDocument>(m_document))

Should either be passing *m_document or get rid of the "!m_document ||" part; no need to do the null check twice.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:3694
> +    if (!is<PluginDocument>(document))
> +        return nullptr;

Should either be passing *document or get rid of the previous if statement; no need to do the null check twice.

[Chris Dumez]

Comment on attachment 238757 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=238757&action=review

>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:247
>> +    if (is<HTMLDocument>(document)) {
> 
> Should be *document if we want to match the old code rather than adding a null check.

There is no null check, at least not in release builds. There is merely an assertion making sure the pointer is not null before dereferencing it. Note that this behavior is not new with is<>(), it matches the previous with isHTMLDivElement() for e.g.

[Chris Dumez]

(In reply to comment #4)
> (From update of attachment 238757 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=238757&action=review
> 
> >> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:247
> >> +    if (is<HTMLDocument>(document)) {
> > 
> > Should be *document if we want to match the old code rather than adding a null check.
> 
> There is no null check, at least not in release builds. There is merely an assertion making sure the pointer is not null before dereferencing it. Note that this behavior is not new with is<>(), it matches the previous with isHTMLDivElement() for e.g.

See implementation of is<>()"

template <typename ExpectedType, typename ArgType>
inline bool is(ArgType& node)
{
    static_assert(!std::is_base_of<ExpectedType, ArgType>::value, "Unnecessary type check");
    return NodeTypeCastTraits<const ExpectedType, const ArgType>::isType(node);
}

template <typename ExpectedType, typename ArgType>
inline bool is(ArgType* node)
{
    ASSERT(node);
    static_assert(!std::is_base_of<ExpectedType, ArgType>::value, "Unnecessary type check");
    return NodeTypeCastTraits<const ExpectedType, const ArgType>::isType(*node);
}

[Chris Dumez]

Darin, given this adds no null check (merely assertions), do you still want me to dereference all the pointers in the is<>() checks?

[Chris Dumez]

Created attachment 238836 [details]
Patch

[Chris Dumez]

+darin in cc (I wish reviewing would auto-cc the reviewer).

Darin, please see my comments above about the null checks.

[WebKit Commit Bot]

Attachment 238836 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLDocument.h:102:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/ImageDocument.h:87:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/MediaDocument.h:61:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/PluginDocument.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

(In reply to comment #6)
> Darin, given this adds no null check (merely assertions), do you still want me to dereference all the pointers in the is<>() checks?

No.

[WebKit Commit Bot]

Comment on attachment 238836 [details]
Patch

Rejecting attachment 238836 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 238836, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebKit/win/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/4646891035820032

[Chris Dumez]

Created attachment 238860 [details]
Patch

[WebKit Commit Bot]

Attachment 238860 [details] did not pass style-queue:


ERROR: Source/WebCore/html/HTMLDocument.h:102:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/ImageDocument.h:87:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/MediaDocument.h:61:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/html/PluginDocument.h:66:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 4 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 238860 [details]
Patch

Clearing flags on attachment: 238860

Committed r174065: <http://trac.webkit.org/changeset/174065>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.