Bug 157299 - Drop some unnecessary exception checking in the generated bindings
Summary: Drop some unnecessary exception checking in the generated bindings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-02 21:21 PDT by Chris Dumez
Modified: 2016-05-03 08:44 PDT (History)
5 users (show)

See Also:

Attachments
Patch (29.68 KB, patch)
2016-05-02 21:52 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (29.68 KB, patch)
2016-05-02 21:58 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Patch (29.78 KB, patch)
2016-05-03 08:43 PDT, Chris Dumez 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2016-05-02 21:21:19 PDT 
Drop some unnecessary exception checking in the generated bindings. Reduces the amount on unnecessary branching.
Comment 1 Chris Dumez 2016-05-02 21:52:21 PDT 
Created attachment 277978 [details]
Patch
Comment 2 Darin Adler 2016-05-02 21:54:56 PDT 
Comment on attachment 277978 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277978&action=review

I’m not sure that all of those things that are marked ", 1" in JSValueToNative can actually raise exceptions.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4324
> +# returns (convertString, mayThrowException).

If you are going to have a period on this comment, then you should capitalize the first word, "Returns".
Comment 3 Chris Dumez 2016-05-02 21:57:51 PDT 
(In reply to comment #2)
> Comment on attachment 277978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=277978&action=review
> 
> I’m not sure that all of those things that are marked ", 1" in
> JSValueToNative can actually raise exceptions.

They all take in a "state" to they may throw (in theory). I did not look further and wanted to be on the safe side. Just avoiding these checks for wrapper types is already nice.
Comment 4 Chris Dumez 2016-05-02 21:58:52 PDT 
Created attachment 277979 [details]
Patch
Comment 5 WebKit Commit Bot 2016-05-03 08:24:36 PDT 
Comment on attachment 277979 [details]
Patch

Rejecting attachment 277979 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 277979, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
 96 lines).
Hunk #31 succeeded at 5455 (offset 96 lines).
Hunk #32 succeeded at 5472 (offset 96 lines).
Hunk #33 succeeded at 5586 (offset 96 lines).
Hunk #34 succeeded at 5611 (offset 96 lines).
Hunk #35 succeeded at 5673 (offset 96 lines).
Hunk #36 succeeded at 5831 (offset 96 lines).
patching file Source/WebCore/bindings/scripts/test/JS/JSTestOverloadedConstructors.cpp

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/1261270
Comment 6 Chris Dumez 2016-05-03 08:43:21 PDT 
Created attachment 277999 [details]
Patch
Comment 7 Chris Dumez 2016-05-03 08:43:59 PDT 
Comment on attachment 277999 [details]
Patch

Clearing flags on attachment: 277999

Committed r200374: <http://trac.webkit.org/changeset/200374>
Comment 8 Chris Dumez 2016-05-03 08:44:06 PDT 
All reviewed patches have been landed.  Closing bug.