Make is<>() / downcast<>() work for HTMLDocument By using the SPECIALIZE_TYPE_TRAITS_*() macro.
Created attachment 238757 [details] Patch
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 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 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.
(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); }
Darin, given this adds no null check (merely assertions), do you still want me to dereference all the pointers in the is<>() checks?
Created attachment 238836 [details] Patch
+darin in cc (I wish reviewing would auto-cc the reviewer). Darin, please see my comments above about the null checks.
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.
(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 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
Created attachment 238860 [details] Patch
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 on attachment 238860 [details] Patch Clearing flags on attachment: 238860 Committed r174065: <http://trac.webkit.org/changeset/174065>
All reviewed patches have been landed. Closing bug.