[webkit-reviews] review granted: [Bug 207427] Expose "allowsContentJavaScript" on WKWebpagePreferences : [Attachment 390784] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 207427: Expose "allowsContentJavaScript" on WKWebpagePreferences
https://bugs.webkit.org/show_bug.cgi?id=207427

Attachment 390784: Patch

https://bugs.webkit.org/attachment.cgi?id=390784&action=review




--- Comment #20 from Darin Adler <darin at apple.com> ---
Comment on attachment 390784
  --> https://bugs.webkit.org/attachment.cgi?id=390784
Patch

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

I like the public allowsContentJavaScript name so much better than the internal
scriptMarkupPolicy name.

> Source/WebCore/dom/Document.cpp:6817
> +    if (!settings().scriptMarkupEnabled())

Seems slightly dangerous to still have this Settings::scriptMarkupEnabled
function. If anyone calls it and it returns true, they will likely do the wrong
thing. Maybe at some point we can rename it?

> Source/WebCore/dom/Document.cpp:6826
> +	   if (m_contextDocument)
> +	       return m_contextDocument->scriptMarkupEnabled();
> +
> +	   // If this Document is frameless or in the wrong frame, *and* it has
no context document,
> +	   // then it has no business running markup script.
> +	   return false;

I’d find this slightly easier to read as an && expression:

    return m_contextDocument && m_contextDocument->scriptMarkupEnabled();

Also slightly annoying that there’s recursion here, but I suppose it’s unlikely
to recurse deeply.

> Source/WebCore/xml/XMLHttpRequest.cpp:194
>	       m_responseDocument->setContextDocument(context);
> +	      
m_responseDocument->setContent(m_responseBuilder.toStringPreserveCapacity());
>	      
m_responseDocument->setSecurityOriginPolicy(context.securityOriginPolicy());
>	       m_responseDocument->overrideMIMEType(mimeType);

Maybe we should also move up the setSecurityOriginPolicy and overrideMIMEType
calls up before we call setContent too? Seems like we’d want all those things
set before we do any parsing.

> Source/WebKit/WebProcess/WebPage/WebFrame.cpp:269
> +	   ASSERT(page());

What guarantees page() is not null?