Closed Bug 1521141 Opened 10 months ago Closed 10 months ago

Stop pretending js::InitClass can be used to initialise standard classes

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(3 files, 3 obsolete files)

js::InitClass has some code/comments which refer to initialising standard classes, but standard classes are no longer initialised through js::InitClass.

js::InitClass is no longer used for initialising standard classes, so remove everything from it (and from DefineConstructorAndPrototype) which handles the case when the JSProtoKey is not JSProto_Null.

Also:

  • Switch from NewNativeObjectWithClassProto to GlobalObject::createBlankPrototypeInheriting so the prototype objects are directly marked as delegates.
  • And change js::InitClass to treat a null protoProto argument as an request to use Object.prototype, which enables us to remove a bit of extra code in js/src/vm/Debugger.cpp.
Attachment #9037637 - Flags: review?(jorendorff)

Removes a bit of unused code (and inlines GlobalObject::[array,regexp]ClassInitialized into their sole callers).

Attachment #9037638 - Flags: review?(jorendorff)

Use js::Throw in more places to reduce code duplication. Also add missing includes for existing js::Throw callers.

Attachment #9037640 - Flags: review?(jorendorff)

Update: Forgot to remove one line when rebasing and splitting the patch.

Attachment #9037637 - Attachment is obsolete: true
Attachment #9037637 - Flags: review?(jorendorff)
Attachment #9037644 - Flags: review?(jorendorff)
Priority: -- → P2
Comment on attachment 9037644 [details] [diff] [review]
bug1521141-part1-slim-down-initclass.patch

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

Thank you so much.

::: js/src/vm/JSObject.cpp
@@ +2031,3 @@
>       * Optionally construct the prototype object, before the class has
>       * been fully initialized.  Allow the ctor to replace proto with a
>       * different object, as is done for operator new.

This comment has been obsolete since JSCLASS_CONSTRUCT_PROTOTYPE was removed in 2011: https://hg.mozilla.org/mozilla-central/rev/a7b658e309b9

Please delete it!
Attachment #9037644 - Flags: review?(jorendorff) → review+
Comment on attachment 9037638 [details] [diff] [review]
bug1521141-part2-remove-unused-code.patch

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

Great.
Attachment #9037638 - Flags: review?(jorendorff) → review+
Attachment #9037640 - Flags: review?(jorendorff) → review+

(In reply to Jason Orendorff [:jorendorff] from comment #5)

::: js/src/vm/JSObject.cpp
@@ +2031,3 @@

  * Optionally construct the prototype object, before the class has
  * been fully initialized.  Allow the ctor to replace proto with a
  * different object, as is done for operator new.

This comment has been obsolete since JSCLASS_CONSTRUCT_PROTOTYPE was removed
in 2011: https://hg.mozilla.org/mozilla-central/rev/a7b658e309b9

Please delete it!

Ah, good to know. I thought this was some obscure way to comment that a JavaScript function can return a different object when the JavaScript new operator is called.

Updated per review comments, carrying r+.

Attachment #9037644 - Attachment is obsolete: true
Attachment #9037644 - Attachment is obsolete: true
Attachment #9038791 - Flags: review+
Attachment #9038792 - Flags: review+

Updated per review comments, carrying r+.

Comment on attachment 9038792 [details] [diff] [review]
bug1521141-part1-slim-down-initclass.patch

Bugzilla apparently decided to duplicate the patch for unknown reasons, so hide one copy of it...
Attachment #9038792 - Attachment is obsolete: true

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6d3f60fc172
Part 2: Remove unused code. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/e166c6ae9e1e
Part 3: Use js::Throw helper in a few more places. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/250c2923583d
Part 1: Remove code to initialise standard classes from js::InitClass. r=jorendorff

Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.