Closed
Bug 745422
Opened 13 years ago
Closed 13 years ago
Wrapper fundamental traps should be factored out into a separate base class
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(1 file)
(deleted),
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Attaching a patch. Flagging luke for review.
Attachment #615011 -
Flags: review?(luke)
Assignee | ||
Comment 2•13 years ago
|
||
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.
Attachment #615011 -
Flags: feedback?(bzbarsky)
Assignee | ||
Comment 3•13 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=7d9604bcf506
Comment 4•13 years ago
|
||
Comment on attachment 615011 [details] [diff] [review] Factor fundamental traps into js::AbstractWrapper. v1 Nice. I like it.
Attachment #615011 -
Flags: review?(luke) → review+
Assignee | ||
Comment 5•13 years ago
|
||
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
Attachment #615011 -
Flags: feedback?(bzbarsky)
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → mozilla14
![]() |
||
Comment 6•13 years ago
|
||
Looks good, but s/funamental/fundamental/?
Assignee | ||
Comment 7•13 years ago
|
||
(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•13 years ago
|
||
Sure.
Comment 9•13 years ago
|
||
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 10•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1b19214a0a50
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
![]() |
||
Comment 11•13 years ago
|
||
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•13 years ago
|
||
And yes, I'm running into this right this moment.
You need to log in
before you can comment on or make changes to this bug.
Description
•