Last Comment Bug 590973 - toolkit component for the parser API
: toolkit component for the parser API
Status: RESOLVED FIXED
reflect-parse fixed-in-tracemonkey
:
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Dave Herman [:dherman]
:
Mentors:
Depends on: 533874
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-26 11:01 PDT by Dave Herman [:dherman]
Modified: 2011-07-06 17:54 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stub XPCOM component in xpconnect (15.75 KB, patch)
2010-08-26 23:02 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Review
stub XPCOM component in toolkit/components/ast (27.01 KB, patch)
2010-09-22 00:38 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Review
not even compiling yet (17.87 KB, patch)
2011-01-03 16:11 PST, Dave Herman [:dherman]
no flags Details | Diff | Review
first working version (21.36 KB, patch)
2011-01-05 21:52 PST, Dave Herman [:dherman]
no flags Details | Diff | Review
minor edits (21.36 KB, patch)
2011-01-06 09:56 PST, Dave Herman [:dherman]
no flags Details | Diff | Review
rebased; removed js_ReflectClass (25.83 KB, patch)
2011-06-22 20:53 PDT, Dave Herman [:dherman]
gal: review+
Details | Diff | Review
final version that landed in TM; to cherrypick for aurora (25.98 KB, patch)
2011-07-05 20:39 PDT, Dave Herman [:dherman]
no flags Details | Diff | Review

Description Dave Herman [:dherman] 2010-08-26 11:01:05 PDT
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] 2010-08-26 13:24:55 PDT
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] 2010-08-26 14:34:28 PDT
> 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] 2010-08-26 23:02:22 PDT
Created attachment 469776 [details] [diff] [review]
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] 2010-08-29 21:45:58 PDT
> 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 dwitte@gmail.com 2010-08-29 21:55:17 PDT
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] 2010-09-02 17:22:59 PDT
dwitte, dherman: *shrug* I can put it in toolkit/components/reflect this weekend.
Comment 7 Alex Vincent [:WeirdAl] 2010-09-17 13:06:56 PDT
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 dwitte@gmail.com 2010-09-17 13:24:35 PDT
mozIAST?
Comment 9 Alex Vincent [:WeirdAl] 2010-09-22 00:38:10 PDT
Created attachment 477430 [details] [diff] [review]
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 [:bsmedberg] 2010-09-22 10:13:38 PDT
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] 2010-09-22 10:34:18 PDT
(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] 2010-09-24 23:12:14 PDT
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 dwitte@gmail.com 2010-10-01 10:41:18 PDT
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] 2010-10-01 11:21:46 PDT
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] 2011-01-03 16:11:03 PST
Created attachment 500936 [details] [diff] [review]
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 dwitte@gmail.com 2011-01-05 15:50:58 PST
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] 2011-01-05 17:21:31 PST
> 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 dwitte@gmail.com 2011-01-05 17:26:53 PST
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] 2011-01-05 20:20:47 PST
> 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] 2011-01-05 21:52:41 PST
Created attachment 501588 [details] [diff] [review]
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 Ted Mielczarek [:ted.mielczarek] 2011-01-06 05:07:21 PST
There's a second parameter to Cu.import that can be the scope:
https://developer.mozilla.org/en/Components.utils.import
Comment 22 Ted Mielczarek [:ted.mielczarek] 2011-01-06 05:14:39 PST
(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] 2011-01-06 08:16:44 PST
> 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] 2011-01-06 09:56:20 PST
Created attachment 501709 [details] [diff] [review]
minor edits
Comment 25 Dave Herman [:dherman] 2011-06-22 20:53:39 PDT
Created attachment 541278 [details] [diff] [review]
rebased; removed js_ReflectClass
Comment 26 Dave Herman [:dherman] 2011-06-26 14:33:24 PDT
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 Ted Mielczarek [:ted.mielczarek] 2011-06-27 06:13:48 PDT
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] 2011-06-27 08:05:46 PDT
> 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 2011-06-28 10:15:26 PDT
Comment on attachment 541278 [details] [diff] [review]
rebased; removed js_ReflectClass

Looks good.
Comment 30 Dave Herman [:dherman] 2011-06-28 10:59:40 PDT
http://hg.mozilla.org/tracemonkey/rev/2ce7546583ff
Comment 31 Chris Leary [:cdleary] (not checking bugmail) 2011-07-05 18:08:25 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2ce7546583ff
Comment 32 Dave Herman [:dherman] 2011-07-05 20:39:09 PDT
Created attachment 544139 [details] [diff] [review]
final version that landed in TM; to cherrypick for aurora
Comment 33 Benjamin Smedberg [:bsmedberg] 2011-07-05 21:04:08 PDT
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] 2011-07-05 21:15:02 PDT
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] 2011-07-05 21:22:19 PDT
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 [:bsmedberg] 2011-07-05 21:35:41 PDT
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] 2011-07-05 22:18:15 PDT
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] 2011-07-06 17:54:47 PDT
Merged into Aurora:

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

Dave

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