Closed
Bug 1031092
Opened 10 years ago
Closed 10 years ago
Clean up the hazmat zone that is {jsproxy,jswrapper}.{cpp,h}
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: efaust, Assigned: efaust)
Details
Attachments
(10 files, 1 obsolete file)
2.20 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
23.19 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
18.77 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
79.66 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
41.65 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
11.75 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
2.22 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
20.60 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
45.52 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
9.19 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
jsproxy.cpp is 3000 lines long. It contains definitions of 2 proxy classes, and function implementations of friend API boilerplate, 4 different Proxy handler classes, as well as the engine's interface into the proxy world.
Similarly, jswrapper.cpp contains definitions of a bunch of jsfriendapi helpers related to wrappers, as well as function definitions of some 3 or 4 proxy handler classes.
It is now the case that we have reason to want to move one of these myriad definitions from one file to another.
This is insanity. Jason has had the frankly staggering idea of organizing everything with its own little subfolder and giving each handler type its own header and implementation file.
You know, me just might be onto something.
Assignee | ||
Comment 1•10 years ago
|
||
What do people think about this? Too much. Worth it?
This is obviously a WIP. There's many more handlers to do, and all of jswrapper, though I will likely just break that out into *.cpp and leave the header (which kinda all goes together now) as one unit. The other alternative there is to have it contain the public facing stuff and include the other headers after splitting them out. Thoughts?
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8482955 -
Flags: feedback?(jwalden+bmo)
Attachment #8482955 -
Flags: feedback?(bobbyholley)
Comment 2•10 years ago
|
||
Comment on attachment 8482955 [details] [diff] [review]
Just so we get a taste
Review of attachment 8482955 [details] [diff] [review]:
-----------------------------------------------------------------
Looks fine to me.
Attachment #8482955 -
Flags: feedback?(bobbyholley) → feedback+
Comment 3•10 years ago
|
||
Comment on attachment 8482955 [details] [diff] [review]
Just so we get a taste
Review of attachment 8482955 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/moz.build
@@ +59,3 @@
> 'perf/jsperf.h',
> + 'proxy/jsproxy.h',
> + 'proxy/jswrapper.h',
Drive-by comment: these should be something like proxy/Proxy.h and proxy/Wrapper.h.
@@ +231,5 @@
> 'perf/jsperf.cpp',
> 'prmjtime.cpp',
> + 'proxy/DirectProxyHandler.cpp',
> + 'proxy/jsproxy.cpp',
> + 'proxy/jswrapper.cpp',
Ditto.
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8482955 -
Attachment is obsolete: true
Attachment #8482955 -
Flags: feedback?(jwalden+bmo)
Attachment #8486194 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8486195 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8486196 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8486198 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8486199 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8486201 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8486202 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8486204 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8486205 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 13•10 years ago
|
||
Sorry for the noise. Seems like it should be easier to look over things one idea at a time.
Attachment #8486206 -
Flags: review?(bobbyholley)
Updated•10 years ago
|
Attachment #8486194 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8486195 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8486196 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8486198 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8486199 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8486201 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8486202 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8486204 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8486205 -
Flags: review?(bobbyholley) → review+
Comment 14•10 years ago
|
||
Comment on attachment 8486206 [details] [diff] [review]
Part 10: Factor out SecurityWrapper
Review of attachment 8486206 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for doing this!
Attachment #8486206 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 15•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d095ec7083b
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2096329d05c
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0016652c3e8c
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/72dad42e3912
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/abd86c308ffe
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ae18d60bbd2a
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/dbe450659642
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/91a3c20e5e7f
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d63a483978a5
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/af39f6ba880b
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4d095ec7083b
https://hg.mozilla.org/mozilla-central/rev/b2096329d05c
https://hg.mozilla.org/mozilla-central/rev/72dad42e3912
https://hg.mozilla.org/mozilla-central/rev/abd86c308ffe
https://hg.mozilla.org/mozilla-central/rev/ae18d60bbd2a
https://hg.mozilla.org/mozilla-central/rev/dbe450659642
https://hg.mozilla.org/mozilla-central/rev/91a3c20e5e7f
https://hg.mozilla.org/mozilla-central/rev/d63a483978a5
https://hg.mozilla.org/mozilla-central/rev/af39f6ba880b
https://hg.mozilla.org/mozilla-central/rev/e86946fb9889
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in
before you can comment on or make changes to this bug.
Description
•