Note: There are a few cases of duplicates in user autocompletion which are being worked on.

CTypes prototype hierarchy should behave more realistically

RESOLVED FIXED in mozilla11

Status

()

Core
js-ctypes
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Over in bug 704147, jorendorff suggested (via me) doing FunctionType.prototype.prototype.call = Function.prototype.call.

This broke for two reasons:
1 - We don't support toString or toSource on CTypeProto and CDataProto objects, meaning that the web console would hit an exception every time it tried to print the result. This is arguably a red herring, but nasty nonetheless.

2 - We don't support accessing the 'prototype' property of CTypeProto objects. This loses us some transparency, and generally represents a hole in our otherwise impeccable object graph.

I just wrote some patches to make this all work. Coming right up.
Created attachment 576659 [details] [diff] [review]
part 1 - Introduce CType::IsCTypeProto() and CData::isCDataProto(). v1
Attachment #576659 - Flags: review?(jorendorff)
Created attachment 576660 [details] [diff] [review]
part 2 - Make toSource() and toString() work on sCTypeProtoClass and sCDataProtoClass objects. v1
Attachment #576660 - Flags: review?(jorendorff)
Created attachment 576661 [details] [diff] [review]
part 3 - Allow access to {C,Pointer,Struct,Array,Function}Type.prototype.prototype. v1
Assignee: nobody → bobbyholley+bmo
Status: NEW → ASSIGNED
Attachment #576661 - Flags: review?(jorendorff)
Gah. My git-to-bugzilla patch uploader broke with a recent bugzilla change, so I just manually generated and uploaded these patches. But, I forgot to do -U8.

Rather than doing a bunch of churn, here's the link to the patches on github in case you need any more context: https://github.com/bholley/mozilla-central/commits/ctypeproto
Also, the reason I sunk time into this today was because I thought it would be good to support jorendorff's little prototype.prototype hack, a la bug 704147. But I just remembered that we actually share those objects across compartments, and thus don't want people to modify the prototypes. The only reason they're modifiable right now is that the Freeze call is commented out:

http://mxr.mozilla.org/mozilla-central/source/js/src/ctypes/CTypes.cpp#697

So this patch is less useful, but what the heck.

Also, it looks like the bug blocking the Freeze call has been fixed. Maybe we should take care of that while we're at it?
Attachment #576659 - Flags: review?(jorendorff) → review+
Comment on attachment 576660 [details] [diff] [review]
part 2 - Make toSource() and toString() work on sCTypeProtoClass and sCDataProtoClass objects. v1

Review of attachment 576660 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=me.

::: js/src/ctypes/CTypes.cpp
@@ +3159,5 @@
>    JSObject* obj = JS_THIS_OBJECT(cx, vp);
> +  if (!obj ||
> +      !(CType::IsCType(cx, obj) || CType::IsCTypeProto(cx, obj)))
> +  {
> +    JS_ReportError(cx, "not a CType or a CTypeProto");

Please change all three error messages back. The user isn't expected to know what a CTypeProto is, so the error message "not a CType" is better.

::: toolkit/components/ctypes/tests/unit/test_jsctypes.js.in
@@ +299,5 @@
> +
> +  // toString and toSource are called by the web console during inspection,
> +  // so we don't want them to throw.
> +  do_check_eq(ctypes.CType.prototype.toString(), '[CType proto object]');
> +  do_check_eq(ctypes.CType.prototype.toSource(), '[CType proto object]');

When I make a change like this where I really don't care what the output is, as long as the function doesn't fall over and die, I generally don't make the test check for the specific arbitrarily chosen behavior I implemented--so for example, this could just check that these methods return strings. Just a thought.
Attachment #576660 - Flags: review?(jorendorff) → review+
Comment on attachment 576661 [details] [diff] [review]
part 3 - Allow access to {C,Pointer,Struct,Array,Function}Type.prototype.prototype. v1

In the test, please use Object.getPrototypeOf(obj) instead of obj.__proto__.
Attachment #576661 - Flags: review?(jorendorff) → review+
Addressed jorendorff's comments and pushed to try:

https://tbpl.mozilla.org/?tree=Try&rev=90e36f26720a
This had a bit of a rocky try run, but all of the failures seemed to be known intermittent (maybe the machine was bogged or something)?

Anyway, appears to be OK. Pushed to mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c716afd37a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1488f748ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d2b7e489b20
https://hg.mozilla.org/mozilla-central/rev/3c716afd37a7
https://hg.mozilla.org/mozilla-central/rev/7d1488f748ce
https://hg.mozilla.org/mozilla-central/rev/4d2b7e489b20
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.