[webkit-reviews] review granted: [Bug 137169] Make is<>() / downcast<>() work for HTMLDocument and its subclasses : [Attachment 238757] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 137169: Make is<>() / downcast<>() work for HTMLDocument and its subclasses
https://bugs.webkit.org/show_bug.cgi?id=137169

Attachment 238757: Patch
https://bugs.webkit.org/attachment.cgi?id=238757&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.