Clean up the hazmat zone that is {jsproxy,jswrapper}.{cpp,h}

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla35
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 1 obsolete attachment)

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.
Posted patch Just so we get a taste (obsolete) — Splinter Review
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 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 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.
Attachment #8482955 - Attachment is obsolete: true
Attachment #8482955 - Flags: feedback?(jwalden+bmo)
Attachment #8486194 - Flags: review?(bobbyholley)
Attachment #8486195 - Flags: review?(bobbyholley)
Attachment #8486196 - Flags: review?(bobbyholley)
Attachment #8486198 - Flags: review?(bobbyholley)
Attachment #8486199 - Flags: review?(bobbyholley)
Attachment #8486205 - Flags: review?(bobbyholley)
Sorry for the noise. Seems like it should be easier to look over things one idea at a time.
Attachment #8486206 - Flags: review?(bobbyholley)
Attachment #8486194 - Flags: review?(bobbyholley) → review+
Attachment #8486195 - Flags: review?(bobbyholley) → review+
Attachment #8486196 - Flags: review?(bobbyholley) → review+
Attachment #8486198 - Flags: review?(bobbyholley) → review+
Attachment #8486199 - Flags: review?(bobbyholley) → review+
Attachment #8486201 - Flags: review?(bobbyholley) → review+
Attachment #8486202 - Flags: review?(bobbyholley) → review+
Attachment #8486204 - Flags: review?(bobbyholley) → review+
Attachment #8486205 - Flags: review?(bobbyholley) → review+
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+
You need to log in before you can comment on or make changes to this bug.