590973 - toolkit component for the parser API

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Dave Herman [:dherman]]

Once the parser API (bug 533874) stabilizes (there are still crashes; see bug 590820), expose it to chrome via a toolkit component.

(Note to self: use http://mxr.mozilla.org/mozilla-central/source/toolkit/components/ctypes/ctypes.jsm to see how this is done.)

Dave

Comment 1

[Alex Vincent [:WeirdAl]]

I can easily provide a stub C++ component (where the methods all return NS_ERROR_NOT_IMPLEMENTED), complete with Makefile.in changes, etc. if you like.  I'm assuming the interface will look like:

[scriptable, uuid(...)]
interface nsIASTParser : nsISupports {
  nsIVariant parse(in DOMString source,
                   [optional] in DOMString filename,
                   [optional] in PRUint32 lineNumber);
};

Comment 2

[Dave Herman [:dherman]]

> I can easily provide a stub C++ component (where the methods all return
> NS_ERROR_NOT_IMPLEMENTED), complete with Makefile.in changes, etc. if you like.

That'd be nice, thanks -- you could just attach it to this bug. But only if it doesn't take you much time. I shouldn't have too much trouble getting this to work when the time comes (famous last words, right?). What I'm blocked on right now is just fixing the bugs in Reflect.parse.

>  I'm assuming the interface will look like:
> 
> [scriptable, uuid(...)]
> interface nsIASTParser : nsISupports {
>   nsIVariant parse(in DOMString source,
>                    [optional] in DOMString filename,
>                    [optional] in PRUint32 lineNumber);
> };

That looks about right, although I'm new to the XPCOM/XPConnect stuff.

Dave

Comment 3

[Alex Vincent [:WeirdAl]]

Attachment: stub XPCOM component in xpconnect

Toolkit didn't seem like the right place to put this, especially since XPConnect has lots of JS <=> C++ magic internally available.  I think XPConnect is a better place to put this.

Comment 4

[Dave Herman [:dherman]]

> Toolkit didn't seem like the right place to put this, especially since
> XPConnect has lots of JS <=> C++ magic internally available.  I think XPConnect
> is a better place to put this.

OK. Thanks so much for the starter patch. I'm agnostic as to where this would go, but Dan Witte (now CC'ed) expressed some preference for this going in toolkit.

Thanks,
Dave

Comment 5

[[e-mail]]

Nothing wrong with something like this going in toolkit -- js/src/xpconnect is already an overloaded place. ctypes is implemented in jsengine but has a toolkit component to provide it as a jsm. If you want to mess with JSAPI and avoid xpconnect, that's the way to go.

Comment 6

[Alex Vincent [:WeirdAl]]

dwitte, dherman: *shrug* I can put it in toolkit/components/reflect this weekend.

Comment 7

[Alex Vincent [:WeirdAl]]

Sorry I haven't gotten to this yet - besides, it looks like the Reflect API is very much still in flux.

I'm also thinking about renaming the interface, maybe not to nsIASTReflect.  I'm thinking the interface - once the JSReflect class stabilizes - could be used for other parser reflection components (CSS parser, most obviously).  What would be the right name for that sort of capability?

dherman:  No, I'm not asking the JS engine to parse CSS.  Different components can exist, implementing the same interface in different ways for different purposes.

Comment 8

[[e-mail]]

mozIAST?

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: stub XPCOM component in toolkit/components/ast

Sorry for the lengthy delay.  Starting a new component from scratch, including all the build system artifacts, is always a bit of a challenge.  With the 2.0 base line being new, it was a real learning experience.

On the MPL tri-license, I wasn't exactly sure about contributor credit and initial developers, since I'm providing stub code and interfaces, and dherman's probably providing the implementation.  In particular, Dave works for Mozilla, and I never have.  So if it seems inconsistent, that's why:  I don't know where to take credit and where to give it.  (Personally, I could care less about getting credit for this, along with most of the other patches I've contributed over the last 10 years.)

dwitte:  any comments on the stub?  The last thing I want to do is give Dave a patch which he'll extend upon, but which would fail reviews because of something I did incorrectly.

Comment 10

[Benjamin Smedberg]

1) why is this configurable? If we're going to do it, why not make it on by default?

2) is the patch incomplete? You added a configure option to js/src/configure.in, but it's not actually used anywhere within js/src.

3) use jsval, not nsIVariant

I don't actually understand what the overall goal here is. Isn't Reflect an object already implemented by the JS engine? Can't we just make it available to chrome contexts, and skip most or all of the XPCOM goop?

Comment 11

[Alex Vincent [:WeirdAl]]

(In reply to comment #10)
> 1) why is this configurable? If we're going to do it, why not make it on by
> default?

We discussed this in #developers.  I know shaver was in the discussion, and I believe dwitte was as well.  It seemed a natural assumption, particularly as it's not stable yet.
 
> 2) is the patch incomplete? You added a configure option to
> js/src/configure.in, but it's not actually used anywhere within js/src.

Yes, this patch is deliberately incomplete.  See comments 1, 2.  I guessed that since the Reflect API is built for Spidermonkey but not exposed to Firefox, we'd need something in js/src/configure.in to tell it "Turn this on."
 
> 3) use jsval, not nsIVariant
> 
> I don't actually understand what the overall goal here is. Isn't Reflect an
> object already implemented by the JS engine? Can't we just make it available to
> chrome contexts, and skip most or all of the XPCOM goop?

If we can skip XPCOM, great.  I was simply following the example / pattern of js-ctypes, which implements a component (and JSM) to fetch ctypes support.

I'll go with whatever the consensus is, though.

Comment 12

[Alex Vincent [:WeirdAl]]

Comment on attachment 477430 [details] [diff] [review]
stub XPCOM component in toolkit/components/ast

shaver says XPCOM is the wrong way to go, in rather vehement terms.

Comment 13

[[e-mail]]

Alex -- if this is already implemented and set up in jseng, would the goal not be to just expose it as a .jsm? That won't involve any XPCOM goop other than the single function to bind the Reflect object to the global, just like ctypes.

Comment 14

[Alex Vincent [:WeirdAl]]

I'm going to wash my hands of this for now.  I'm getting conflicting instructions on the path to exposing the Reflect API, and that is definitely neither fun nor interesting to me.

Without consensus on what exposure route to take, I'm just wasting everyone's time.

Comment 15

[Dave Herman [:dherman]]

Attachment: not even compiling yet

I started copying and pasting from the ctypes implementation, to try to get something working. It's not even building yet, and I'm still sort of blindly wandering through the forest of NS_BLAH_BLAH_BLAH macros. If someone (dwitte?) wants to give me a hand, I'd be much obliged...

Dave

Comment 16

[[e-mail]]

This looks good. The tricky bits are build goo. You need to add reflect stuff to the following files (just look for ctypes and do the same thing):

browser/installer/removed-files.in
toolkit/toolkit-makefiles.sh (several places)
toolkit/library/nsStaticXULComponents.cpp
toolkit/library/libxul-config.mk

What kind of build issues do you have?

Comment 17

[Dave Herman [:dherman]]

> This looks good. The tricky bits are build goo. You need to add reflect stuff
> to the following files (just look for ctypes and do the same thing):

Ah, great, thanks for the tip.

> What kind of build issues do you have?

http://pastebin.mozilla.org/910394

I don't understand what kinds of meta-tools are creating/looking for these Module classes, so I can't really tell what I'm doing wrong. Sorry about the blind copying and pasting...

Dave

Comment 18

[[e-mail]]

Comment on attachment 500936 [details] [diff] [review]
not even compiling yet

>diff --git a/toolkit/components/reflect/reflect.cpp b/toolkit/components/reflect/reflect.cpp

>+#include "jsapi.h"
>+#include "mozilla/ModuleUtils.h"
>+#include "nsMemory.h"
>+#include "nsString.h"
>+#include "nsNativeCharsetUtils.h"

You need to #include "reflect.h". ;)

Comment 19

[Dave Herman [:dherman]]

> toolkit/toolkit-makefiles.sh (several places)

Question: why is the Makefile variable called MAKEFILES_ctypes but the reference under add_makefiles is to $MAKEFILES_jsctypes?

> You need to #include "reflect.h". ;)

I could've sworn I did that before! Anyway, now it builds. Who knew? :)

Thanks,
Dave

Comment 20

[Dave Herman [:dherman]]

Attachment: first working version

OK, this one seems to work. Not sure I did the right thing wrt that one makefile I asked about in comment 19.

Also, it bugs me that

    Components.utils.import("resource://gre/modules/reflect.jsm")

mutates the global object. Is there not some way to make this just return the object, so the user can put it wherever they want? I'm guessing the answer is no, but figured I'd ask.

Dave

Comment 21

[(not currently active) Ted Mielczarek]

There's a second parameter to Cu.import that can be the scope:
https://developer.mozilla.org/en/Components.utils.import

Comment 22

[(not currently active) Ted Mielczarek]

(In reply to comment #19)
> > toolkit/toolkit-makefiles.sh (several places)
> 
> Question: why is the Makefile variable called MAKEFILES_ctypes but the
> reference under add_makefiles is to $MAKEFILES_jsctypes?

Looks like a bug. Feel free to just land a fix, changes to that file have blanket-rs=build-peers. (Fortunately missing/incorrect entries in that file don't usuallly cause problems, as the build system will generate missing Makefiles on the fly as it recurses into subdirs.)

Comment 23

[Dave Herman [:dherman]]

> There's a second parameter to Cu.import that can be the scope:
> https://developer.mozilla.org/en/Components.utils.import

Better than nothing, but it just seems like a botch not to simply return an object. If nothing else, this API is fairly hostile to Harmony modules. I wonder if this will cause migration headaches for addon writers as we move forward with Harmony.

> Looks like a bug. Feel free to just land a fix, changes to that file have
> blanket-rs=build-peers. (Fortunately missing/incorrect entries in that file
> don't usuallly cause problems, as the build system will generate missing
> Makefiles on the fly as it recurses into subdirs.)

OK. I've pushed a patch at:

http://hg.mozilla.org/tracemonkey/rev/d13654bb28f1

Thanks,
Dave

Comment 24

[Dave Herman [:dherman]]

Attachment: minor edits

Comment 25

[Dave Herman [:dherman]]

Attachment: rebased; removed js_ReflectClass

Comment 26

[Dave Herman [:dherman]]

API question: since the Reflect object is really just a module containing the parse() function, should Components.utils.import("resource://gre/modules/reflect.jsm"[, obj]) just directly import 'parse' into the namespace? It's a trivial change, just wondering which way would be preferable. I'm leaning towards changing it.

Dave

Comment 27

[(not currently active) Ted Mielczarek]

Convention for existing JSM modules in our tree appears to be that they export a single symbol which is the name of the JSM:
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/XPCOMUtils.jsm#125
http://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/NetUtil.jsm#41
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/loader/ISO8601DateUtils.jsm#44
etc.

Seems pretty straightforward, and future-proofs you if you want to add more methods later.

Comment 28

[Dave Herman [:dherman]]

> Convention for existing JSM modules in our tree appears to be that they
> export a single symbol which is the name of the JSM:

Roger that. Thanks!

> Seems pretty straightforward, and future-proofs you if you want to add more
> methods later.

<standard gripe about Components.utils.import's scope-injecting/mutating API/>

Thanks,
Dave

Comment 29

[Andreas Gal :gal]

Comment on attachment 541278 [details] [diff] [review]
rebased; removed js_ReflectClass

Looks good.

Comment 30

[Dave Herman [:dherman]]

http://hg.mozilla.org/tracemonkey/rev/2ce7546583ff

Comment 31

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2ce7546583ff

Comment 32

[Dave Herman [:dherman]]

Attachment: final version that landed in TM; to cherrypick for aurora

Comment 33

[Benjamin Smedberg]

Comment on attachment 544139 [details] [diff] [review]
final version that landed in TM; to cherrypick for aurora

Aurora nominations should include a rationale. This missed aurora and unless there is something that isn't stated in the bug there isn't any reason we should break feature-freeze for this.

Comment 34

[Dave Herman [:dherman]]

I did what cdleary said to do here:

http://groups.google.com/group/mozilla.dev.tech.js-engine.internals/browse_thread/thread/626c85124555c0c9

Sorry it didn't include a rationale; I've never had to do this before and didn't know the process.

Dave

Comment 35

[Dave Herman [:dherman]]

Comment on attachment 544139 [details] [diff] [review]
final version that landed in TM; to cherrypick for aurora

This landed in tracemonkey last week, but cdleary's merge to m-c today landed after the merge from m-c to aurora:

http://groups.google.com/group/mozilla.dev.tech.js-engine.internals/browse_thread/thread/626c85124555c0c9

Dave

Comment 36

[Benjamin Smedberg]

Comment on attachment 544139 [details] [diff] [review]
final version that landed in TM; to cherrypick for aurora

Yes, well, it missed the merge. The normal rule is that we don't take features on aurora. It's not clear to me why this is important or special to break the rules, so please clarify *why* you want it to land on Aurora.

Comment 37

[Dave Herman [:dherman]]

OK. I really tried to follow the process as best as I could grok it, but I guess it still didn't make it.

Dave, disappointed

Comment 38

[Dave Herman [:dherman]]

Merged into Aurora:

http://hg.mozilla.org/releases/mozilla-aurora/rev/75552dfd25c2

Dave