Last Comment Bug 745422 - Wrapper fundamental traps should be factored out into a separate base class
: Wrapper fundamental traps should be factored out into a separate base class
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on:
Blocks: 726949
  Show dependency treegraph
 
Reported: 2012-04-13 23:32 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2012-04-19 17:41 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Factor fundamental traps into js::AbstractWrapper. v1 (12.74 KB, patch)
2012-04-13 23:33 PDT, Bobby Holley (:bholley) (busy with Stylo)
gal: review+
Details | Diff | Splinter Review

Description Bobby Holley (:bholley) (busy with Stylo) 2012-04-13 23:32:20 PDT
We need this to avoid duplicating js::Wrapper code in bug 726949.

We want to use the forwarding behavior of js::Wrapper, but have the derived traps implemented with fundamental traps (a la ProxyHandler) rather than implemented directly. This is easy enough to do.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2012-04-13 23:33:36 PDT
Created attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

Attaching a patch. Flagging luke for review.
Comment 2 Bobby Holley (:bholley) (busy with Stylo) 2012-04-13 23:35:02 PDT
Comment on attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

Flagging bz for feedback to make sure it meets his use case.
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2012-04-13 23:35:25 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=7d9604bcf506
Comment 4 Andreas Gal :gal 2012-04-13 23:38:30 PDT
Comment on attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

Nice. I like it.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2012-04-15 16:43:51 PDT
Comment on attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

Given that we talked about it already, I'm going to skip the feedback? and land this so that it's ready for bz on monday.

Pushed to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/1b19214a0a50
Comment 6 Boris Zbarsky [:bz] 2012-04-15 18:01:58 PDT
Looks good, but s/funamental/fundamental/?
Comment 7 Bobby Holley (:bholley) (busy with Stylo) 2012-04-15 18:43:17 PDT
(In reply to Boris Zbarsky (:bz) from comment #6)
> Looks good, but s/funamental/fundamental/?

Already landed it - can you piggyback that on your patch?
Comment 8 Boris Zbarsky [:bz] 2012-04-15 18:46:56 PDT
Sure.
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2012-04-16 01:33:44 PDT
Comment on attachment 615011 [details] [diff] [review]
Factor fundamental traps into js::AbstractWrapper. v1

Review of attachment 615011 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jswrapper.h
@@ +50,5 @@
>  namespace js {
>  
>  class DummyFrameGuard;
>  
> +/* Base class that just implements no-op forwarding methods for funamental

"funamental"
Comment 11 Boris Zbarsky [:bz] 2012-04-19 17:40:20 PDT
So wait.  We have code that assumes that isWrapper() is enough to call Wrapper::wrapperHandler and then examine the flags.  How is that supposed to work after this patch for things that are AbstractWrapper but not Wrapper?  Won't it just examine random memory?
Comment 12 Boris Zbarsky [:bz] 2012-04-19 17:41:24 PDT
And yes, I'm running into this right this moment.

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