Last Comment Bug 675745 - XML initialization simplifications
: XML initialization simplifications
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-01 14:35 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-08-04 03:14 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Clarify js_InitNamespaceClass (2.16 KB, patch)
2011-08-01 14:36 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
igor: review+
Details | Diff | Splinter Review
Clarify js_InitQNameClass (1.88 KB, patch)
2011-08-01 14:40 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
igor: review+
Details | Diff | Splinter Review
Clarify js_InitXMLClass (3.74 KB, patch)
2011-08-01 14:41 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
igor: review+
Details | Diff | Splinter Review
Remove JSCLASS_CONSTRUCT_PROTOTYPE from a couple XML classes that don't need it (1022 bytes, patch)
2011-08-01 14:43 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
igor: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-01 14:35:11 PDT
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.
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-01 14:36:59 PDT
Created attachment 549928 [details] [diff] [review]
Clarify js_InitNamespaceClass
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-01 14:40:21 PDT
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.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-01 14:41:43 PDT
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.
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-01 14:43:08 PDT
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.
Comment 5 Igor Bukanov 2011-08-02 00:15:12 PDT
(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.
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-02 08:36:56 PDT
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.
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-08-02 10:17:52 PDT
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 Igor Bukanov 2011-08-02 12:50:38 PDT
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.
Comment 9 Igor Bukanov 2011-08-02 12:52:44 PDT
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.
Comment 10 Igor Bukanov 2011-08-02 12:56:21 PDT
Comment on attachment 549932 [details] [diff] [review]
Clarify js_InitXMLClass

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

Again, add those constants.
Comment 11 Igor Bukanov 2011-08-02 12:58:18 PDT
(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 :)

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