Bug 71013 – Add allowScriptFromSource callback to FrameLoaderClient

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

Add allowJavaScriptFromSource callback to FrameLoaderClient

Created attachment 112674 [details]
Patch

Hey Adam, can you have a look please?

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 112674 [details]
Patch

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

> Source/WebCore/loader/FrameLoaderClient.h:302
>          virtual bool allowJavaScript(bool enabledPerSettings) { return enabledPerSettings; }

perhaps these methods should really be allowScript and allowScriptFromSource.  we normally leave off the "java" prefix in front of script.

[Adam Barth]

Comment on attachment 112674 [details]
Patch

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

> Source/WebCore/loader/FrameLoaderClient.h:303
> +        virtual bool allowJavaScriptFromSource(bool enabledPerSettings, const KURL&) { return enabledPerSettings; }

At least for allowJavaScriptFromSource, it should be allowScriptFromSource.  I think it's fine to change allowJavaScript in a follow-up patch.  (That's mostly a search-and-replace job.)

(In reply to comment #5)
> (From update of attachment 112674 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112674&action=review
> 
> > Source/WebCore/loader/FrameLoaderClient.h:303
> > +        virtual bool allowJavaScriptFromSource(bool enabledPerSettings, const KURL&) { return enabledPerSettings; }
> 
> At least for allowJavaScriptFromSource, it should be allowScriptFromSource.  I think it's fine to change allowJavaScript in a follow-up patch.  (That's mostly a search-and-replace job.)

Basically, renaming allowJavaScript to allowScript is easy. However, there's still isJavaScriptEnabled(), and renaming that touches lots of ports. not sure how happy they would be about a setting being renamed

[Adam Barth]

They'll be fine as long as you don't change their public API.  In any case, doing this in a bunch of small patches seems best.

Created attachment 112721 [details]
Patch

[Adam Barth]

Comment on attachment 112721 [details]
Patch

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

LGTM, but we need fishd to do the API review.

> Tools/DumpRenderTree/chromium/WebPermissions.h:62
> -    // Resets the policy to allow images, storage, displaying insecure, but not running insecure.
> +    // Resets the policy to allow images, scripts, storage, displaying insecure, but not running insecure.

I'd just remove this comment or generalize it so we don't need to edit it all the time.

Created attachment 112820 [details]
Patch

[WebKit Review Bot]

Comment on attachment 112820 [details]
Patch

Clearing flags on attachment: 112820

Committed r98708: <http://trac.webkit.org/changeset/98708>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.