Bug 35205

Summary:

[Chromium API] Disambiguate allowJavaScript from didFailToExecuteJavaScript

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

fishd, pkasting

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	none
patch v3	none
patch v4	fishd: review+, fishd: commit-queue+

Adam Barth

Reported 2010-02-20 22:04:14 PST

[Chromium API] Disambiguous allowJavaScript from didFailToExecuteJavaScript

Attachments
Patch (4.94 KB, patch) 2010-02-20 22:06 PST, Adam Barth	no flags	Details Formatted Diff Diff
Patch (29.45 KB, patch) 2010-02-21 01:03 PST, Adam Barth	no flags	Details Formatted Diff Diff
patch v3 (29.13 KB, patch) 2010-02-24 14:17 PST, Peter Kasting	no flags	Details Formatted Diff Diff
patch v4 (29.12 KB, patch) 2010-02-24 14:34 PST, Peter Kasting	fishd: review+ fishd: commit-queue+	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2010-02-20 22:06:39 PST

Created attachment 49141 [details] Patch

Peter Kasting

Comment 2 2010-02-20 22:48:10 PST

You might want to comment more (perhaps in comments on the function declarations?) on why a piece of code would care whether it can execute script if it doesn't actually intend to execute script. There are other possible APIs here whose usefulness probably depends on the answer. (I'm mostly worried about future code calling allowJavaScript() without calling didFailToExecuteJavaScript() when it should. Perhaps we should create a single access point you call when you want to run script that checks if it's legal and calls this callback if not, or something.)

Adam Barth

Comment 3 2010-02-20 23:55:00 PST

Comment on attachment 49141 [details] Patch This patch is wrong because V8 cheatzors.

Adam Barth

Comment 4 2010-02-21 01:03:06 PST

Created attachment 49146 [details] Patch

Adam Barth

Comment 5 2010-02-21 01:04:02 PST

Done and done. This patch got a bit bigger and probably harder to merge, but I think you're right that we should force clients to be explicit about this bit so we do the right thing in the future.

Peter Kasting

Comment 6 2010-02-22 15:45:18 PST

Comment on attachment 49146 [details] Patch > +enum ReasonForCallingCanExecuteScripts { > + AboutToExecuteScript, > + NotAboutToExecuteScript > +}; I wonder if there could be a better name for the second choice... "NotPlanningToExecuteScript"? "JustCuriousAboutCurrentSetting"? I dunno :(. In any case it seems like rationale for this enum should be given in comments above it.

Darin Fisher (:fishd, Google)

Comment 7 2010-02-23 14:38:48 PST

Comment on attachment 49146 [details] Patch > + [Chromium API] Disambiguate allowJavaScript from didFailToExecuteJavaScript As discussed elsewhere, I'd still much prefer a name for didFailToExecuteJavaScript that better indicates failure due to a policy decision as opposed to other problems such as a syntax error. How about renaming this method to didNotAllowScript? That nicely corresponds with allowScript.

Peter Kasting

Comment 8 2010-02-24 14:17:01 PST

Created attachment 49435 [details] patch v3 Renamed didFailToExecuteJavaScript() to didNotAllowScript(). Updated patch to apply against current tree.

Peter Kasting

Comment 9 2010-02-24 14:34:45 PST

Created attachment 49440 [details] patch v4 Last patch didn't fix everywhere because I didn't realize the first patch used two different function names (for no good reason)

Peter Kasting

Comment 10 2010-02-24 16:33:15 PST

Landed in r55207.

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