Bug 32063 - ScriptController::isEnabled needs to be renamed
: ScriptController::isEnabled needs to be renamed
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-02 04:40 PST by Patrik Persson
Modified: 2010-01-22 11:02 PST (History)
7 users (show)

See Also:


Attachments
Patch (25.25 KB, patch)
2010-01-09 01:18 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (25.16 KB, patch)
2010-01-09 16:30 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrik Persson 2009-12-02 04:40:21 PST
During the work with #21288, we realized that the method ScriptController::isEnabled() has grown to do more than the name says. The name should reflect what the method does (checks settings, checks sandboxing status, passes decision to frame loader client): 

bool ScriptController::isEnabled()
{
    Settings* settings = m_frame->settings();
    return m_frame->loader()->client()->allowJavaScript(settings && settings->isJavaScriptEnabled() && !m_frame->loader()->isSandboxed(SandboxScripts));
}

Darin Adler suggested 'canExecuteScripts' below. Is that OK?

(In reply to comment #51 in bug 21288)
> Loose end for after this lands: ScriptController::isEnabled needs to be
> renamed. It already checked more than just whether JavaScript was enabled.
> Maybe canExecuteScripts is the right name.
Comment 1 Alexey Proskuryakov 2009-12-02 09:38:42 PST
> Darin Adler suggested 'canExecuteScripts' below. Is that OK?

The name looks good in abstract. Once you post  a final patch, it will be easier to see if it makes sense at all call sites.

    return m_frame->loader()->client()->allowJavaScript(settings &&
settings->isJavaScriptEnabled() &&
!m_frame->loader()->isSandboxed(SandboxScripts));

By the way, given that the argument to FrameLoaderClient::allowJavaScript() is called "enabledPerSettings", it may be more appropriate to have the new isSandboxed() check outside the parentheses.
Comment 2 Darin Adler 2009-12-02 09:42:07 PST
(In reply to comment #1)
> By the way, given that the argument to FrameLoaderClient::allowJavaScript() is
> called "enabledPerSettings", it may be more appropriate to have the new
> isSandboxed() check outside the parentheses.

Patrik, sorry for sidetracking in your bug.

Alexey, I think the real issue there is whether we want the frame loader client to be able to override the sandboxing.

If we do want the client to have this capability, and I personally think we do, then we could change the name of the argument to allowJavaScript. Or consider other API refinements so the client can make the decision in a well-informed way.

If we don't, then I think we should change the rules so that ScriptController won't even call allowJavaScript when sandboxed.

We could look at what clients currently do with the allowJavaScript function to inform that decision.
Comment 3 Adam Barth 2009-12-10 09:01:39 PST
Why would you want the client to be able to override the sandbox?  I'm not opposed, I just don't understand the use case.
Comment 4 Darin Adler 2009-12-10 13:15:52 PST
(In reply to comment #3)
> Why would you want the client to be able to override the sandbox?

I'm not sure there is a use case.

I think it depends on why we have allowJavaScript as a client call at all. This was added for Chromium and is currently not used in any other port.
Comment 5 Adam Barth 2009-12-10 13:34:52 PST
fishd would know, but I think this was due to paranoia about XSS in browser UI surfaces built out of HTML.
Comment 6 Adam Barth 2010-01-09 01:18:48 PST
Created attachment 46202 [details]
Patch
Comment 7 WebKit Review Bot 2010-01-09 01:57:51 PST
Attachment 46202 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/176231
Comment 8 Adam Barth 2010-01-09 16:30:41 PST
Created attachment 46227 [details]
Patch
Comment 9 WebKit Commit Bot 2010-01-09 18:16:13 PST
Comment on attachment 46227 [details]
Patch

Rejecting patch 46227 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11902 test cases.
fast/profiler/profile-calls-in-included-file.html -> crashed

Exiting early after 1 failures. 7848 tests run.
270.16s total testing time

7847 test cases (99%) succeeded
1 test case (<1%) crashed
3 test cases (<1%) had stderr output

Full output: http://webkit-commit-queue.appspot.com/results/175716
Comment 10 Adam Barth 2010-01-09 18:36:36 PST
Comment on attachment 46227 [details]
Patch

Clearing flags on attachment: 46227

Committed r53046: <http://trac.webkit.org/changeset/53046>
Comment 11 Adam Barth 2010-01-09 18:36:43 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Eric Seidel 2010-01-11 13:44:03 PST
Filed bug 33490 about the crash.
Comment 13 Darin Fisher (:fishd, Google) 2010-01-22 11:02:58 PST
(In reply to comment #5)
> > I think it depends on why we have allowJavaScript as a client call at all. 
> > This was added for Chromium and is currently not used in any other port.
> fishd would know, but I think this was due to paranoia about XSS in browser UI
> surfaces built out of HTML.

Chromium wants to be able to enable JavaScript for chrome:// pages as well as file:// and ftp:// directory listings even when the user has disabled JavaScript.

I believe allowJavaScript is also enables a feature like the noscript Firefox extension.