The default bug view has changed. See this FAQ.

Support running code after successful wrapping in Paris bindings

RESOLVED FIXED in mozilla15

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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).
Attachment #621174 - Flags: review?(bzbarsky)
(Assignee)

Comment 1

5 years ago
Created attachment 621275 [details] [diff] [review]
v1.1

Got rid of the declareJSVal stuff.
Attachment #621174 - Attachment is obsolete: true
Attachment #621174 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #621275 - Flags: review?(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!
Attachment #621275 - Flags: review?(bzbarsky) → review+
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;
  }
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.
Blocks: 748267
Created attachment 621324 [details] [diff] [review]
Part 4 now renumbered as part 3 and merged on top of the patch in
Attachment #621324 - Flags: review?(peterv)
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.
Attachment #621324 - Attachment is obsolete: true
Attachment #621324 - Flags: review?(peterv)
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 621407 [details] [diff] [review]
v1.2

Let me know if there's still docs missing.
Attachment #621275 - Attachment is obsolete: true
Attachment #621407 - Flags: review+
(Assignee)

Comment 9

5 years ago
Created attachment 621409 [details] [diff] [review]
v1.2

Forgot to qref.
Attachment #621407 - Attachment is obsolete: true
Attachment #621409 - Flags: review+
> 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.
That attachment looks great.  Maybe also mention that wrapCall should evaluate to a boolean where false indicates failure?
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaea76d5bc92
https://hg.mozilla.org/mozilla-central/rev/eaea76d5bc92
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.