Bug 71013 - Add allowScriptFromSource callback to FrameLoaderClient
Summary: Add allowScriptFromSource callback to FrameLoaderClient
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: jochen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-27 06:39 PDT by jochen
Modified: 2011-10-28 01:17 PDT (History)
6 users (show)

See Also:


Attachments
Patch (15.57 KB, patch)
2011-10-27 06:40 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (15.61 KB, patch)
2011-10-27 12:07 PDT, jochen
no flags Details | Formatted Diff | Diff
Patch (15.59 KB, patch)
2011-10-28 00:02 PDT, jochen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jochen 2011-10-27 06:39:40 PDT
Add allowJavaScriptFromSource callback to FrameLoaderClient
Comment 1 jochen 2011-10-27 06:40:26 PDT
Created attachment 112674 [details]
Patch
Comment 2 jochen 2011-10-27 06:41:04 PDT
Hey Adam, can you have a look please?
Comment 3 WebKit Review Bot 2011-10-27 06:43:52 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 4 Darin Fisher (:fishd, Google) 2011-10-27 10:48:27 PDT
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.
Comment 5 Adam Barth 2011-10-27 11:33:42 PDT
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.)
Comment 6 jochen 2011-10-27 11:43:49 PDT
(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
Comment 7 Adam Barth 2011-10-27 11:45:52 PDT
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.
Comment 8 jochen 2011-10-27 12:07:32 PDT
Created attachment 112721 [details]
Patch
Comment 9 Adam Barth 2011-10-27 12:20:16 PDT
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.
Comment 10 jochen 2011-10-28 00:02:22 PDT
Created attachment 112820 [details]
Patch
Comment 11 WebKit Review Bot 2011-10-28 01:17:17 PDT
Comment on attachment 112820 [details]
Patch

Clearing flags on attachment: 112820

Committed r98708: <http://trac.webkit.org/changeset/98708>
Comment 12 WebKit Review Bot 2011-10-28 01:17:24 PDT
All reviewed patches have been landed.  Closing bug.