Closed Bug 590973 Opened 11 years ago Closed 10 years ago

toolkit component for the parser API

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: dherman, Assigned: dherman)

References

Details

(Whiteboard: reflect-parse fixed-in-tracemonkey)

Attachments

(2 files, 5 obsolete files)

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
Component: JavaScript Engine → General
Product: Core → Toolkit
QA Contact: general → general
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);
};
> 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
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.
> 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
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.
dwitte, dherman: *shrug* I can put it in toolkit/components/reflect this weekend.
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.
mozIAST?
Depends on: 533874
OS: Mac OS X → All
Hardware: x86 → All
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.
Attachment #469776 - Attachment is obsolete: true
Attachment #477430 - Flags: feedback?
Attachment #477430 - Flags: feedback? → feedback?(dwitte)
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?
(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 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.
Attachment #477430 - Attachment is obsolete: true
Attachment #477430 - Flags: feedback?(dwitte)
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.
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.
Attached patch not even compiling yet (obsolete) — Splinter Review
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
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?
> 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 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". ;)
> 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
Attached patch first working version (obsolete) — Splinter Review
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
Attachment #500936 - Attachment is obsolete: true
There's a second parameter to Cu.import that can be the scope:
https://developer.mozilla.org/en/Components.utils.import
(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.)
> 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
Attached patch minor edits (obsolete) — Splinter Review
Attachment #501588 - Attachment is obsolete: true
Whiteboard: reflect-parse
Attachment #501709 - Attachment is obsolete: true
Attachment #541278 - Flags: review?(jst)
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
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.
> 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
Attachment #541278 - Flags: review?(jst) → review?(gal)
Comment on attachment 541278 [details] [diff] [review]
rebased; removed js_ReflectClass

Looks good.
Attachment #541278 - Flags: review?(gal) → review+
http://hg.mozilla.org/tracemonkey/rev/2ce7546583ff
Whiteboard: reflect-parse → reflect-parse fixed-in-tracemonkey
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
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.
Attachment #544139 - Flags: approval-mozilla-aurora?
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 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
Attachment #544139 - Flags: approval-mozilla-aurora?
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.
Attachment #544139 - Flags: approval-mozilla-aurora?
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
You need to log in before you can comment on or make changes to this bug.