Bug 102601 - Add ScriptWrappable to more WebCore classes which are commonly JS-wrapped
Summary: Add ScriptWrappable to more WebCore classes which are commonly JS-wrapped
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on: 102539
Blocks:
  Show dependency treegraph
 
Reported: 2012-11-17 15:51 PST by Eric Seidel (no email)
Modified: 2012-11-17 20:50 PST (History)
8 users (show)

See Also:


Attachments
Patch (16.03 KB, patch)
2012-11-17 16:42 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-11-17 15:51:33 PST
Add ScriptWrappable to more WebCore classes which are commonly JS-wrapped
Comment 1 Eric Seidel (no email) 2012-11-17 16:42:14 PST
Created attachment 174833 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-11-17 16:47:36 PST
These were all found using:
https://bugs.webkit.org/show_bug.cgi?id=102539#c2
and then surfing around to a few common sites (google.com, amazon.com, yahoo.com, apple.com) and running robohornet, parts of dromaeo, html5test.com and acid3.

I'm sure there are many more little ones to find, but I doubt even this patch will show up in benchmarks.

This patch should be a small memory savings (there are not many of these objects created) and a small perf savings on a few sites.

I think if we want to do more of these, we should come up with a better way to test for over/under-usage of ScriptWrappable.  The current manual-testing solution is not sustainable in the long term. :)
Comment 3 Adam Barth 2012-11-17 19:19:19 PST
Comment on attachment 174833 [details]
Patch

Ok
Comment 4 WebKit Review Bot 2012-11-17 19:38:29 PST
Comment on attachment 174833 [details]
Patch

Clearing flags on attachment: 174833

Committed r135058: <http://trac.webkit.org/changeset/135058>
Comment 5 WebKit Review Bot 2012-11-17 19:38:33 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Eric Seidel (no email) 2012-11-17 20:50:08 PST
I suspect there are *many* more classes in WebCore which are only ever used from the JS DOM API and thus technically should be ScriptWrappable.  Most notable being all the SVG pod types and DOM objects.  But since those didn't show up in my brief testing, I doubt they're particularly performance critical.

Ideally we'd come up with a better model for making sure we always have the right set of ScriptWrappable objects.  Adam suggested we might eventually make objects with an IDL have to opt-out of the bindings assuming ScriptWrappable-ness, and thus catch many more cases.  Not sure.