XML initialization simplifications

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(4 attachments)

It's a bit hard to follow how the E4X classes are set up, due to their relying on a large, complex workhorse method; a little clarification will be helpful.  Patches to follow.
Created attachment 549928 [details] [diff] [review]
Clarify js_InitNamespaceClass
Attachment #549928 - Flags: review?(igor)
Created attachment 549930 [details] [diff] [review]
Clarify js_InitQNameClass

Note that (as in the previous patch) these changes mean the JSCLASS_CONSTRUCT_PROTOTYPE class flag is no longer needed for this class.
Attachment #549930 - Flags: review?(igor)
Created attachment 549932 [details] [diff] [review]
Clarify js_InitXMLClass

The XML classes in particular have some really hairy special-casing code which is dramatically simpler with these sorts of cleanups.
Attachment #549932 - Flags: review?(igor)
Created attachment 549933 [details] [diff] [review]
Remove JSCLASS_CONSTRUCT_PROTOTYPE from a couple XML classes that don't need it

These two classes aren't constructed through the js_InitClass pinch point, so the flag is unnecessary and only makes the code less clear.
Attachment #549933 - Flags: review?(igor)

Comment 5

6 years ago
(In reply to comment #1)
> Created attachment 549928 [details] [diff] [review] [diff] [details] [review]
> Clarify js_InitNamespaceClass

I do not see how replacing single method call with 26 lines of code can be a cleanup. Could you explain what is wrong with js_InitClass?

(In reply to comment #3)
> Created attachment 549932 [details] [diff] [review] [diff] [details] [review]
> Clarify js_InitXMLClass

Here AFAICS the only simplification is avoiding calling js_LookupProperty to get the constructor. But that can be avoided if js_InitClass would return the constructed class object.
I'm working to remove use of js_InitClass for all the built-in classes.  This will simplify initialization of standard classes, and of the global object more generally.  I find the control flow in js_InitClass and DefineConstructorAndPrototype quite confusing, and I would like to reduce dependencies on it because I am not smart enough to understand it, not when applied across a dozen different uses, all slightly different.

Also, by removing the dependency on JSCLASS_CONSTRUCT_PROTOTYPE, the use of which for XML I find quite confusing, I enable removing that code from js_InitClass as well.  I could merge the removal of that into these patches, if you wanted, to offset some of the lines of code added here.
And in case it was unclear, making js_InitClass return the constructor seems to me to be doubling down on js_InitClass's complexity, not making it any simpler and more understandable.

Comment 8

6 years ago
Comment on attachment 549928 [details] [diff] [review]
Clarify js_InitNamespaceClass

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

::: js/src/jsxml.cpp
@@ +7121,5 @@
> +    namespaceProto->setNameURI(empty);
> +    namespaceProto->syncSpecialEquality();
> +
> +    JSFunction *ctor = global->createConstructor(cx, Namespace, &js_NamespaceClass,
> +                                                 CLASS_ATOM(cx, Namespace), 2);

Define a constant like NAMESPACE_DEFINED_ARGS = 2 before the constructor function and use it here  instead of unclear 2.
Attachment #549928 - Flags: review?(igor) → review+

Comment 9

6 years ago
Comment on attachment 549930 [details] [diff] [review]
Clarify js_InitQNameClass

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

::: js/src/jsxml.cpp
@@ +7152,5 @@
> +        return NULL;
> +    qnameProto->syncSpecialEquality();
> +
> +    JSFunction *ctor = global->createConstructor(cx, QName, &js_QNameClass,
> +                                                 CLASS_ATOM(cx, QName), 2);

As with Namespace, define an explicit constant like QNAME_DEFINED_ARGS and use it, not 2, here.
Attachment #549930 - Flags: review?(igor) → review+

Comment 10

6 years ago
Comment on attachment 549932 [details] [diff] [review]
Clarify js_InitXMLClass

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

Again, add those constants.
Attachment #549932 - Flags: review?(igor) → review+

Updated

6 years ago
Attachment #549933 - Flags: review?(igor) → review+

Comment 11

6 years ago
(In reply to comment #6)
> I enable removing that code from
> js_InitClass as well.  I could merge the removal of that into these patches,
> if you wanted, to offset some of the lines of code added here.

I trust you that the removal would not be forgotten :)
http://hg.mozilla.org/integration/mozilla-inbound/rev/0f1422d3acc9
http://hg.mozilla.org/integration/mozilla-inbound/rev/97c6b5502d05
http://hg.mozilla.org/integration/mozilla-inbound/rev/0e60d45bcb1c
http://hg.mozilla.org/integration/mozilla-inbound/rev/9a24057c1f5d
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/0f1422d3acc9
http://hg.mozilla.org/mozilla-central/rev/97c6b5502d05
http://hg.mozilla.org/mozilla-central/rev/0e60d45bcb1c
http://hg.mozilla.org/mozilla-central/rev/9a24057c1f5d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.