Last Comment Bug 771081 - Rename CrossOriginWrapper
: Rename CrossOriginWrapper
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Gabor Krizsanits [:krizsa :gabor]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-05 03:02 PDT by Bobby Holley (PTO through June 13)
Modified: 2012-07-17 02:13 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part1: Rename CrossOriginWrapper class (5.85 KB, patch)
2012-07-15 12:45 PDT, Gabor Krizsanits [:krizsa :gabor]
gal: review+
Details | Diff | Review
part2: Rename CrossOriginWrapper files (4.00 KB, patch)
2012-07-15 12:46 PDT, Gabor Krizsanits [:krizsa :gabor]
gal: review+
Details | Diff | Review
part3: Rename WaiveXrayWrapperWrapper (3.31 KB, patch)
2012-07-15 12:48 PDT, Gabor Krizsanits [:krizsa :gabor]
gal: review+
Details | Diff | Review

Description Bobby Holley (PTO through June 13) 2012-07-05 03:02:32 PDT
The name is terribly confusing. Now that NoWaiverWrapper is gone, we should fix this up.

Luke suggested TransitivelyWaivingWrapper, which sounds fine to me, unless anyone has a better suggestion.

Blake, how does TransitivelyWaivingWrapper sound?
Comment 1 Gabor Krizsanits [:krizsa :gabor] 2012-07-05 04:10:41 PDT
Yaaay. Also, once we have the name you can assign this one to me anytime.
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-07-12 19:03:51 PDT
How about WaiveXrayWrapper? TransitivelyWaiving sounds like it could waive just about anything.
Comment 3 Gabor Krizsanits [:krizsa :gabor] 2012-07-12 23:18:38 PDT
(In reply to Blake Kaplan (:mrbkap) from comment #2)
> How about WaiveXrayWrapper? 

Sounds good, but isn't that kind of taken already (WaiveXrayWrapperWrapper)?
Comment 4 Bobby Holley (PTO through June 13) 2012-07-13 00:36:13 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> (In reply to Blake Kaplan (:mrbkap) from comment #2)
> > How about WaiveXrayWrapper? 
> 
> Sounds good, but isn't that kind of taken already (WaiveXrayWrapperWrapper)?

We should just rename that to XrayWaiver IMO.
Comment 5 Gabor Krizsanits [:krizsa :gabor] 2012-07-13 01:14:16 PDT
So XrayWaiver and WaiveXrayWrapper are a bit too easy to mix up for me. One of the trivial difference between them is one of them is CCW the other is SCW. Would it make sense to make that difference explicit in their names to avoid confusion?

If I'm the only one who finds them easy to mix up, I like them already better than the current names. Btw what is against simply calling it TransparentWrapper?
Comment 6 Bobby Holley (PTO through June 13) 2012-07-13 01:22:29 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> So XrayWaiver and WaiveXrayWrapper are a bit too easy to mix up for me. One
> of the trivial difference between them is one of them is CCW the other is
> SCW. Would it make sense to make that difference explicit in their names to
> avoid confusion?

They accomplish two very distinct things. The second isn't just the SCW version of the first - it's serves as a second identity to make the operation of the first possible.

In our parlance, an object "has a waiver" (a noun) if it's bound inside XrayWaiver. We then use the WaiveXrayWrapper to access it.

How about s/WaiveXrayWrapper/WaivedXrayWrapper/? I think that's more precise, and might help alleviate the confusion a little bit.
 
> If I'm the only one who finds them easy to mix up, I like them already
> better than the current names. Btw what is against simply calling it
> TransparentWrapper?

Because that's much less explicit about what's actually going on, IMO. Waiving Xray gives transparent semantics, but it _also_ clamps the principal. And that would also imply that same-origin content wrappers are somehow not transparent.
Comment 7 Gabor Krizsanits [:krizsa :gabor] 2012-07-13 01:43:27 PDT
(In reply to Bobby Holley (:bholley) from comment #6)
> They accomplish two very distinct things. 

I know, but it seemed like an easy way to distinguish them.

> In our parlance, an object "has a waiver" (a noun) if it's bound inside
> XrayWaiver. We then use the WaiveXrayWrapper to access it.

This sold it to me.

> How about s/WaiveXrayWrapper/WaivedXrayWrapper/? I think that's more
> precise, and might help alleviate the confusion a little bit.

I prefer the other one. Only one character difference is too easy for the eyes and the ears to mix up.
Comment 8 Bobby Holley (PTO through June 13) 2012-07-13 02:00:24 PDT
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> > In our parlance, an object "has a waiver" (a noun) if it's bound inside
> > XrayWaiver. We then use the WaiveXrayWrapper to access it.
> 
> This sold it to me.

w00t! Care to whip up a patch? ;-)
Comment 9 Gabor Krizsanits [:krizsa :gabor] 2012-07-15 12:45:24 PDT
Created attachment 642405 [details] [diff] [review]
part1: Rename CrossOriginWrapper class
Comment 10 Gabor Krizsanits [:krizsa :gabor] 2012-07-15 12:46:59 PDT
Created attachment 642406 [details] [diff] [review]
part2: Rename CrossOriginWrapper files
Comment 11 Gabor Krizsanits [:krizsa :gabor] 2012-07-15 12:48:14 PDT
Created attachment 642407 [details] [diff] [review]
part3: Rename WaiveXrayWrapperWrapper
Comment 12 Andreas Gal :gal 2012-07-15 12:50:59 PDT
Comment on attachment 642407 [details] [diff] [review]
part3: Rename WaiveXrayWrapperWrapper

Much better naming.
Comment 13 Gabor Krizsanits [:krizsa :gabor] 2012-07-15 21:30:09 PDT
https://tbpl.mozilla.org/?tree=Try&rev=1dfd8c22d506
Comment 14 Luke Wagner [:luke] 2012-07-16 08:51:35 PDT
\o/
Comment 16 Ed Morley [:emorley] 2012-07-16 11:18:23 PDT
Didn't make it to mozilla-central before the uplift (merge was blocked on bug 774259). Adjusting milestone accordingly.

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