Closed
Bug 771907
Opened 12 years ago
Closed 8 years ago
Remove AbstractWrapper
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: ejpbruel, Unassigned)
References
Details
(Whiteboard: [js:t])
Attachments
(1 file)
10.04 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
The class AbstractWrapper is currently not used anywhere. More precisely, it looks like the only code that uses it does not use the policy enforcement traps, and could therefore inherit from IndirectProxyHandler instead. If that's true, it's probably better to get rid of this class. Proxies are currently in flux, and changes in other parts of the code could easily lead to AbstractWrapper breaking without us noticing it.
Reporter | ||
Comment 1•12 years ago
|
||
Attachment #640200 -
Flags: review?(bobbyholley+bmo)
Comment 2•12 years ago
|
||
Comment on attachment 640200 [details] [diff] [review] Patch to be reviewed This looks good! It also means, I think, that we no longer need the multiple inheritance for DirectWrapper, so we could just fold everything into DirectWrapper, and get rid of those toWrapper and toProxyHandler virtual methods. What do you think?
Attachment #640200 -
Flags: review?(bobbyholley+bmo) → review+
Reporter | ||
Comment 3•12 years ago
|
||
Adding bug 771429 as a dependency, since it looks as bz might have a use for IndirectWrapper at all.
Depends on: 771429
Reporter | ||
Comment 4•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #2) > Comment on attachment 640200 [details] [diff] [review] > Patch to be reviewed > > This looks good! It also means, I think, that we no longer need the multiple > inheritance for DirectWrapper, so we could just fold everything into > DirectWrapper, and get rid of those toWrapper and toProxyHandler virtual > methods. What do you think? Provided bz doesn't need IndirectWrapper, I totally agree :-)
Updated•12 years ago
|
Whiteboard: [js:t]
Reporter | ||
Comment 5•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #2) > Comment on attachment 640200 [details] [diff] [review] > Patch to be reviewed > > This looks good! It also means, I think, that we no longer need the multiple > inheritance for DirectWrapper, so we could just fold everything into > DirectWrapper, and get rid of those toWrapper and toProxyHandler virtual > methods. What do you think? My understanding of our last irc convo was that it is ok for us to move forward with this. Can you confirm?
Comment 6•12 years ago
|
||
Well, I think this patch wouldn't be correct until IndirectProxyHandler is unwrappable, right? Otherwise, we change the behavior of SandboxProxy.
Reporter | ||
Comment 7•12 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #6) > Well, I think this patch wouldn't be correct until IndirectProxyHandler is > unwrappable, right? Otherwise, we change the behavior of SandboxProxy. You're right. Good thing that I double checked :-)
Reporter | ||
Updated•11 years ago
|
Assignee: ejpbruel → general
Assignee | ||
Updated•10 years ago
|
Assignee: general → nobody
Comment 8•8 years ago
|
||
Abstract/IndirectMapper was removed in https://hg.mozilla.org/mozilla-central/rev/da2e3ebd1c26
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•