924937 - There should be an option for evalInWindow to wrap the result instead of cloning

Status: RESOLVED WONTFIX

(Core :: XPConnect, defect)

Description

[Gabor Krizsanits (INACTIVE)]

I talked to Bobby about this during the summit, that for exporting objects in a meaningfull way there should be a way to get a reference to an object from the target scope. So sometimes it seems that wrapping the result makes more sense than cloning it.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Attachment: cloneResult option for evaInWindow

Something like this.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 814919 [details] [diff] [review]
cloneResult option for evaInWindow

Review of attachment 814919 [details] [diff] [review]:
-----------------------------------------------------------------

Conceptually, this is fine. But I'd really like to clean up and generalize our machinery for parsing options objects, since this kind of thing really tends to come up.

I think some sort of Options superclass with the parsing machinery built-in as protected methods would be really nice, so that we have very minimal overrides for SandboxOptions and EvalInWindowOptions, which basically just describe the properties and leave the heavy lifting to the superclass.

Comment 3

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #2)
> Conceptually, this is fine. But I'd really like to clean up and generalize
> our machinery for parsing options objects, since this kind of thing really
> tends to come up.

Sure, but I would prefer doing that in a separate (refactoring) bug.

> 
> I think some sort of Options superclass with the parsing machinery built-in
> as protected methods would be really nice, so that we have very minimal
> overrides for SandboxOptions and EvalInWindowOptions, which basically just
> describe the properties and leave the heavy lifting to the superclass.

I'm not against this. So the this parsing baseclass would store the cx and the option object, and base parsing methods, and all the helper methods should return bool instead of nsresult. The subclasses should have a parse() method and optionally some specific ones like the GlobalProperties parser function for SandboxOptions.

Is there anything else we wanted to clean up in this area?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > Conceptually, this is fine. But I'd really like to clean up and generalize
> > our machinery for parsing options objects, since this kind of thing really
> > tends to come up.
> 
> Sure, but I would prefer doing that in a separate (refactoring) bug.

Ok.

> 
> > 
> > I think some sort of Options superclass with the parsing machinery built-in
> > as protected methods would be really nice, so that we have very minimal
> > overrides for SandboxOptions and EvalInWindowOptions, which basically just
> > describe the properties and leave the heavy lifting to the superclass.
> 
> I'm not against this. So the this parsing baseclass would store the cx and
> the option object

Right. and It'd be MOZ_STACK_CLASS so that we could keep a RootedObject on the stack.

> , and base parsing methods, and all the helper methods
> should return bool instead of nsresult. The subclasses should have a parse()
> method and optionally some specific ones like the GlobalProperties parser
> function for SandboxOptions.

Right. And ideally, the parse implementation would be very compact, because most of the complexity would be handled by the superclass. I'm thinking something like:

bool
SandboxOptions::Parse()
{
  return ParseBool("wantXrays", &wantXrays) &&
         ParseString("sandboxName", &sandboxName) &&
         // etc
         ;
}

What do you think?
 
> Is there anything else we wanted to clean up in this area?

Possibly, but this is a good start for now.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #4)
> Right. And ideally, the parse implementation would be very compact, because
> most of the complexity would be handled by the superclass. I'm thinking
> something like:
> 
> bool
> SandboxOptions::Parse()
> {
>   return ParseBool("wantXrays", &wantXrays) &&
>          ParseString("sandboxName", &sandboxName) &&
>          // etc
>          ;
> }
> 
> What do you think?

Almost. I would break that into separate lines for easy debugging, but if you prefere this style I can do that. Anyway, can you review these patches then? I will do this refactoring on top of it, and land this missing feature asap, so I can move forward the higher level object exporting API in the SDK.

Comment 6

[Gabor Krizsanits (INACTIVE)]

Eh, I mean this single patch not patches...

Refactoring: Bug 925293

Comment 7

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> Almost. I would break that into separate lines for easy debugging, but if
> you prefere this style I can do that.

I'm fine. The former is cleaner and more JSAPI-idiomatic, but doing:
ok = ParseBool("wantXrays", &wantXrays);
NS_ENSURE_TRUE(ok, false);
ok = ParseString("sandboxName", &sandboxName);
NS_ENSURE_TRUE(ok, false);

is ok too.

> Anyway, can you review these patches
> then? I will do this refactoring on top of it, and land this missing feature
> asap, so I can move forward the higher level object exporting API in the SDK.

Sure.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 814919 [details] [diff] [review]
cloneResult option for evaInWindow

Gabor and I discussed this, and eventually decided it was unsafe. The correct long-term solution is JS-Xrays. In the short term, we're going to implement something like Cu.createObjectIn for Sandboxes.