Last Comment Bug 752042 - Support running code after successful wrapping in Paris bindings
: Support running code after successful wrapping in Paris bindings
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Peter Van der Beken [:peterv]
:
Mentors:
Depends on:
Blocks: 748267
  Show dependency treegraph
 
Reported: 2012-05-04 14:18 PDT by Peter Van der Beken [:peterv]
Modified: 2012-05-10 18:45 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (11.08 KB, patch)
2012-05-04 14:18 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v1.1 (10.31 KB, patch)
2012-05-05 04:26 PDT, Peter Van der Beken [:peterv]
bzbarsky: review+
Details | Diff | Splinter Review
Part 4 now renumbered as part 3 and merged on top of the patch in (9.00 KB, patch)
2012-05-05 11:17 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
v1.2 (12.19 KB, patch)
2012-05-06 05:21 PDT, Peter Van der Beken [:peterv]
peterv: review+
Details | Diff | Splinter Review
v1.2 (12.20 KB, patch)
2012-05-06 05:25 PDT, Peter Van der Beken [:peterv]
peterv: review+
Details | Diff | Splinter Review

Description Peter Van der Beken [:peterv] 2012-05-04 14:18:58 PDT
Created attachment 621174 [details] [diff] [review]
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] 2012-05-05 04:26:51 PDT
Created attachment 621275 [details] [diff] [review]
v1.1

Got rid of the declareJSVal stuff.
Comment 2 Boris Zbarsky [:bz] (TPAC) 2012-05-05 08:25:51 PDT
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 [:bz] (TPAC) 2012-05-05 09:00:46 PDT
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 [:bz] (TPAC) 2012-05-05 09:03:01 PDT
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 [:bz] (TPAC) 2012-05-05 11:17:16 PDT
Created attachment 621324 [details] [diff] [review]
Part 4 now renumbered as part 3 and merged on top of the patch in
Comment 6 Boris Zbarsky [:bz] (TPAC) 2012-05-05 11:18:02 PDT
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] 2012-05-06 03:29:15 PDT
(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] 2012-05-06 05:21:12 PDT
Created attachment 621407 [details] [diff] [review]
v1.2

Let me know if there's still docs missing.
Comment 9 Peter Van der Beken [:peterv] 2012-05-06 05:25:46 PDT
Created attachment 621409 [details] [diff] [review]
v1.2

Forgot to qref.
Comment 10 Boris Zbarsky [:bz] (TPAC) 2012-05-06 11:21:31 PDT
> 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 [:bz] (TPAC) 2012-05-06 11:57:35 PDT
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] 2012-05-10 05:11:16 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaea76d5bc92
Comment 13 Joe Drew (not getting mail) 2012-05-10 18:45:11 PDT
https://hg.mozilla.org/mozilla-central/rev/eaea76d5bc92

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