Bug 32063 - ScriptController::isEnabled needs to be renamed
: ScriptController::isEnabled needs to be renamed
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2009-12-02 04:40 PST by
Modified: 2010-01-22 11:02 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 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 From 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 From 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 From 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 From 2010-01-09 01:18:48 PST -------
Created an attachment (id=46202) [details]
Patch
------- Comment #7 From 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 From 2010-01-09 16:30:41 PST -------
Created an attachment (id=46227) [details]
Patch
------- Comment #9 From 2010-01-09 18:16:13 PST -------
(From update of attachment 46227 [details])
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 From 2010-01-09 18:36:36 PST -------
(From update of attachment 46227 [details])
Clearing flags on attachment: 46227

Committed r53046: <http://trac.webkit.org/changeset/53046>
------- Comment #11 From 2010-01-09 18:36:43 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #12 From 2010-01-11 13:44:03 PST -------
Filed bug 33490 about the crash.
------- Comment #13 From 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.