| Summary: | ScriptController::isEnabled needs to be renamed | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Patrik Persson <patrik.j.persson> | ||||||
| Component: | WebCore JavaScript | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | abarth, commit-queue, darin, dglazkov, eric, fishd, webkit.review.bot | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | All | ||||||||
| OS: | All | ||||||||
| Attachments: |
|
||||||||
> 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.
(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. 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. (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. fishd would know, but I think this was due to paranoia about XSS in browser UI surfaces built out of HTML. Created attachment 46202 [details]
Patch
Attachment 46202 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/176231 Created attachment 46227 [details]
Patch
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 on attachment 46227 [details] Patch Clearing flags on attachment: 46227 Committed r53046: <http://trac.webkit.org/changeset/53046> All reviewed patches have been landed. Closing bug. Filed bug 33490 about the crash. (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. |
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.