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] (PTO until july 3)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-05 03:02 PDT by Bobby Holley (busy)
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] (PTO until july 3)
gal: review+
Details | Diff | Review
part2: Rename CrossOriginWrapper files (4.00 KB, patch)
2012-07-15 12:46 PDT, Gabor Krizsanits [:krizsa :gabor] (PTO until july 3)
gal: review+
Details | Diff | Review
part3: Rename WaiveXrayWrapperWrapper (3.31 KB, patch)
2012-07-15 12:48 PDT, Gabor Krizsanits [:krizsa :gabor] (PTO until july 3)
gal: review+
Details | Diff | Review

Description Bobby Holley (busy) 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] (PTO until july 3) 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) (in and out until 7-14) 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] (PTO until july 3) 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 (busy) 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] (PTO until july 3) 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 (busy) 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] (PTO until july 3) 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 (busy) 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] (PTO until july 3) 2012-07-15 12:45:24 PDT
Created attachment 642405 [details] [diff] [review]
part1: Rename CrossOriginWrapper class
Comment 10 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 2012-07-15 12:46:59 PDT
Created attachment 642406 [details] [diff] [review]
part2: Rename CrossOriginWrapper files
Comment 11 Gabor Krizsanits [:krizsa :gabor] (PTO until july 3) 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] (PTO until july 3) 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.