Bug 137169

Summary: Make is<>() / downcast<>() work for HTMLDocument and its subclasses
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: DOMAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, calvaris, cmarcelo, commit-queue, darin, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, japhet, jer.noble, kangil.han, kling, philipj, rniwa, sergio
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 137056    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2014-09-26 21:03:21 PDT
Make is<>() / downcast<>() work for HTMLDocument By using the SPECIALIZE_TYPE_TRAITS_*() macro.
Comment 1 Chris Dumez 2014-09-26 21:53:55 PDT
Created attachment 238757 [details]
Patch
Comment 2 WebKit Commit Bot 2014-09-26 21:55:34 PDT
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.
Comment 3 Darin Adler 2014-09-28 17:09:17 PDT
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.
Comment 4 Chris Dumez 2014-09-28 17:46:04 PDT
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.
Comment 5 Chris Dumez 2014-09-28 17:47:08 PDT
(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);
}
Comment 6 Chris Dumez 2014-09-28 19:16:15 PDT
Darin, given this adds no null check (merely assertions), do you still want me to dereference all the pointers in the is<>() checks?
Comment 7 Chris Dumez 2014-09-28 19:33:17 PDT
Created attachment 238836 [details]
Patch
Comment 8 Chris Dumez 2014-09-28 19:34:37 PDT
+darin in cc (I wish reviewing would auto-cc the reviewer).

Darin, please see my comments above about the null checks.
Comment 9 WebKit Commit Bot 2014-09-28 19:35:54 PDT
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.
Comment 10 Darin Adler 2014-09-29 08:33:03 PDT
(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.
Comment 11 WebKit Commit Bot 2014-09-29 08:34:53 PDT
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
Comment 12 Chris Dumez 2014-09-29 09:30:44 PDT
Created attachment 238860 [details]
Patch
Comment 13 WebKit Commit Bot 2014-09-29 09:32:58 PDT
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.
Comment 14 WebKit Commit Bot 2014-09-29 09:58:46 PDT
Comment on attachment 238860 [details]
Patch

Clearing flags on attachment: 238860

Committed r174065: <http://trac.webkit.org/changeset/174065>
Comment 15 WebKit Commit Bot 2014-09-29 09:58:53 PDT
All reviewed patches have been landed.  Closing bug.