[webkit-reviews] review granted: [Bug 136694] Use toDocument instead of static_cast<Document*> : [Attachment 237881] Patch

[bugzilla-daemon at webkit.org]

Andy Estes <aestes at apple.com> has granted Gyuyoung Kim
<gyuyoung.kim at webkit.org>'s request for review:
Bug 136694: Use toDocument instead of static_cast<Document*>
https://bugs.webkit.org/show_bug.cgi?id=136694

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

------- Additional Comments from Andy Estes <aestes at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237881&action=review


r=me, but I think you should fix the behavior change in
XMLHttpRequest::document().

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1799
>      ASSERT(scriptExecutionContext()->isDocument());
> -    return *static_cast<Document*>(scriptExecutionContext());
> +    return *toDocument(scriptExecutionContext());

toDocument() calls ASSERT_WITH_SECURITY_IMPLICATION(!scriptExecutionContext()
|| scriptExecutionContext()->isDocument()), which is almost the same as the
ASSERT above except with regard to how null is handled. I think you can
simplify this code to just ASSERT that scriptExecutionContext() is non-null.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:328
>      ASSERT(m_scriptExecutionContext &&
m_scriptExecutionContext->isDocument());
> -    return static_cast<Document*>(m_scriptExecutionContext);
> +    return toDocument(m_scriptExecutionContext);

Ditto.

> Source/WebCore/workers/DefaultSharedWorkerRepository.cpp:231
>      ASSERT_WITH_SECURITY_IMPLICATION(context->isDocument());
>      ASSERT(!isClosing());
>      MutexLocker lock(m_workerDocumentsLock);
> -    Document* document = static_cast<Document*>(context);
> -    m_workerDocuments.add(document);
> +    m_workerDocuments.add(toDocument(context));

Ditto.

> Source/WebCore/workers/WorkerMessagingProxy.cpp:86
>     
ASSERT_WITH_SECURITY_IMPLICATION(m_scriptExecutionContext->isDocument());
> -    Document* document =
static_cast<Document*>(m_scriptExecutionContext.get());
> +    Document* document = toDocument(m_scriptExecutionContext.get());

Ditto.

> Source/WebCore/xml/XMLHttpRequest.cpp:157
> -   
ASSERT_WITH_SECURITY_IMPLICATION(scriptExecutionContext()->isDocument());
> -    return static_cast<Document*>(scriptExecutionContext());
> +    return toDocument(scriptExecutionContext());

You're introducing a behavior change by removing the
ASSERT_WITH_SECURITY_IMPLICATION. Previously, if scriptExecutionContext() was
null then we'd crash evaluating the ASSERT. Now we won't, since the ASSERT used
by toDocument() allows null.