752042 - Support running code after successful wrapping in Paris bindings

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

Attachment: v1

I'm going to need this to use the new codegen for list bindings.

This patch allows passing code to run on successful wrapping (like "return true;") in the template values dict under the "success" key. The patch also supports declaring a jsval variable when needed (if filling in a local variable instead of vp).

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Got rid of the declareJSVal stuff.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 621275 [details] [diff] [review]
v1.1

>+def getWrapTemplateForType(type, descriptorProvider, result, templateValues):

There should be a docstring here describing what keys are required/allowed in templateValues and what they mean.

templateValues does _not_ have to have jsvalRef/jsvalPtr, right?  Those will just get substituted into the return value of this function?  What does it need to have?  Just an optional 'success'?  If so, it might be clearer to just have a successCode="return true;" argument.  At least if we don't plan to add more configuration here.


Also, we should document what things will be substituted into the return value in general.  At this point it looks like jsvalRef/jsvalPtr/obj.  Anything else?

In addition to documenting templateValues, I'd still like to include the information from the docstring in https://bugzilla.mozilla.org/attachment.cgi?id=618560

For that matter, documenting the various helper functions being added here would be really useful.  As it is, I'm not quite sure what some of these are trying to do, in the haveSuccessCode case.

>+        success = templateValues['success']

I think successCode might be clearer, but either way.

>+    def setValue(value, callWrapValue=False):

Maybe document?  Not sure.

>+    def wrapAndSetPtr(wrap, failure=None):

Also maybe document?  Certainly document that "failure", if set, assumes a leading newline and a 2-space indent.  And I'd prefer failureCode but again either way.

>+                            CGIndenter(CGGeneric(setValue("JSVAL_NULL")), 2).define() + "\n" +

You don't need the ", 2" bit: that's the default indent for CGIndenter.

>+                failed = CGIndenter(CGGeneric(wrapAndSetPtr("HandleNewBindingWrappingFailure(cx, ${obj}, %s, ${jsvalPtr})" % result)), 2).define()

Again, no need for the ", 2".  And some line-breaks in there to make this not go quite so far right might be nice.

>+%s""" % (result, CGIndenter(CGGeneric(setValue("JSVAL_NULL")), 2).define(),

Again, nix the ", 2" (this is in the nullable primitive part).

>+                                  templateValues.pop('result', 'result'),

Why do we want to pop instead of get? 

>+    # These are only needed for getWrapTemplateForType, so we pop them
>+    templateValues.pop('success', None)

Again, why pop at all?  Just to make sure we don't accidentally substitute them into "wrap"?

r=me with the nits.   Thanks!

Comment 3

[Boris Zbarsky [:bzbarsky]]

Actually, one other comment.  Do we really want the leading "\n" on setValue and company?  It makes for some weirdness, afaict:

  if (result.IsNull()) {

    *vp = JSVAL_NULL;
    return true;
  }

Comment 4

[Boris Zbarsky [:bzbarsky]]

And in general, I think we should aim to have all intermediate strings have no leading/trailing newlines; whoeever is including them knows more about where the newlines should go.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 now renumbered as part 3 and merged on top of the patch in

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 621324 [details] [diff] [review]
Part 4 now renumbered as part 3 and merged on top of the patch in

Gah.  bzexport screwed up.

Comment 7

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> As it is, I'm not quite sure what some of these are
> trying to do, in the haveSuccessCode case.

haveSuccessCode allows us to do

  return Foo();

instead of

  if (!Foo()) {
    return false;
  }
  return true;

> Also maybe document?  Certainly document that "failure", if set, assumes a
> leading newline and a 2-space indent.

I've made wrapAndSetPtr indent and add the newline.

> Again, why pop at all?  Just to make sure we don't accidentally substitute
> them into "wrap"?

That was the intent, I've converted to get.

(In reply to Boris Zbarsky (:bz) from comment #3)
> Actually, one other comment.  Do we really want the leading "\n" on setValue
> and company?

The problem was that getWrapTemplateForType used to return a string with a leading newline, so I had to do it in various places. I've dropped that and made the caller add the newline.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Let me know if there's still docs missing.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1.2

Forgot to qref.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> haveSuccessCode allows us to do

No, I understand the point of haveSuccessCode.  It's just the methods that use it that could have use better docs, I think.

> I've dropped that and made the caller add the newline.

Perfect.

Will look at diff later tonight or tomorrow morning.

Comment 11

[Boris Zbarsky [:bzbarsky]]

That attachment looks great.  Maybe also mention that wrapCall should evaluate to a boolean where false indicates failure?

Comment 12

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eaea76d5bc92

Comment 13

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/eaea76d5bc92