The default bug view has changed. See this FAQ.

toolkit component for the parser API

RESOLVED FIXED in mozilla8

Status

()

Toolkit
General
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: dherman, Assigned: dherman)

Tracking

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Updated

7 years ago
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);
};
(Assignee)

Comment 2

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

Comment 4

7 years ago
> 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

7 years ago
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.

Comment 8

7 years ago
mozIAST?
Depends on: 533874
OS: Mac OS X → All
Hardware: x86 → All
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.
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)

Comment 13

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

Comment 15

6 years ago
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

6 years ago
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?
(Assignee)

Comment 17

6 years ago
> 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

6 years ago
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". ;)
(Assignee)

Comment 19

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

Comment 20

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

Comment 23

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

Comment 24

6 years ago
Created attachment 501709 [details] [diff] [review]
minor edits
Attachment #501588 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: reflect-parse
(Assignee)

Comment 25

6 years ago
Created attachment 541278 [details] [diff] [review]
rebased; removed js_ReflectClass
Attachment #501709 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #541278 - Flags: review?(jst)
(Assignee)

Comment 26

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

Comment 28

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

Updated

6 years ago
Attachment #541278 - Flags: review?(jst) → review?(gal)

Comment 29

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

Looks good.
Attachment #541278 - Flags: review?(gal) → review+
(Assignee)

Comment 30

6 years ago
http://hg.mozilla.org/tracemonkey/rev/2ce7546583ff
Whiteboard: reflect-parse → reflect-parse fixed-in-tracemonkey
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2ce7546583ff
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
(Assignee)

Comment 32

6 years ago
Created attachment 544139 [details] [diff] [review]
final version that landed in TM; to cherrypick for aurora
Attachment #544139 - Flags: approval-mozilla-aurora?
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?
(Assignee)

Comment 34

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

Comment 35

6 years ago
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?
(Assignee)

Comment 37

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

Comment 38

6 years ago
Merged into Aurora:

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

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