Change all JSClass' name back to the correct name instead of "Object"
Categories
(Core :: JavaScript: Standard Library, task, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox80 | --- | fixed |
People
(Reporter: evilpie, Assigned: anba)
References
Details
Attachments
(7 files)
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review | |
|
47 bytes,
text/x-phabricator-request
|
Details | Review |
Before bug 1277801 Object.prototype.toString would return the wrong result without a @@toStringTag. That is why we changed some JSClass name members to "Object". We should search for all those instances. Searching for "bug 1277801" already finds this one.
|
||
Updated•4 years ago
|
|
Assignee | |
Comment 1•4 years ago
|
||
I'm currently looking into this bug and it seems like we're not really consistent when it comes to naming proto-classes. For some proto-classes we're using "Object" (and also have tests for this behaviour [1]), but for others we're using the class name plus "Prototype", e.g. "Int32ArrayPrototype". And as three special cases. there's also "PromiseProto" for Promise.prototype JSClass, "object" for the streams proto-classes [2], and just the class name without "Prototype" for Iterator.prototype and AsyncIterator.prototype [3].
Tom, do you know if there's any preference here?
[1] https://searchfox.org/mozilla-central/source/js/src/jit-test/tests/basic/plain-object-prototypes-error.js
[2] https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/js/src/builtin/streams/ClassSpecMacro.h#36 and https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/js/src/builtin/streams/ReadableStream.cpp#553
[3] https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/js/src/vm/Iteration.cpp#1663 and https://searchfox.org/mozilla-central/rev/5a4aaccb28665807a6fd49cf48367d47fbb5a19a/js/src/vm/AsyncIteration.cpp#628
|
Reporter | |
Comment 2•4 years ago
|
||
I like something along the lines of Object / ObjectPrototype, so e.g. "Map" / "MapPrototype". Maybe it would be even better to use "Map.prototype" for nicer results in [1]. Not really sure, everything is better than just always using Object. I think we shouldn't worry to much about the error messages right now, those should be improved anyway. InformalValueName using the JSClass name is not great.
|
|
Assignee | |
Comment 3•4 years ago
|
||
Great, let's go for that! :-)
|
|
Assignee | |
Comment 4•4 years ago
|
||
The Intl objects were already changed in bug 1642962 to get a more descriptive
@@toStringTag.
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 5•4 years ago
|
||
Depends on D82042
|
|
Assignee | |
Comment 6•4 years ago
|
||
Depends on D82043
|
|
Assignee | |
Comment 7•4 years ago
|
||
Depends on D82045
|
|
Assignee | |
Comment 8•4 years ago
|
||
Depends on D82046
|
|
Assignee | |
Comment 9•4 years ago
|
||
Depends on D82048
|
|
Assignee | |
Comment 10•4 years ago
|
||
Depends on D82049
|
|
Reporter | |
Comment 11•4 years ago
|
||
Nice \o/ I think those names are much better. Have you pushed this to try? This might potentially cause test failures for some devtools tests.
|
|
Assignee | |
Comment 12•4 years ago
|
||
Ah, good call! I've forgot about devtools, only pushed to try with jsreftest, jit-tests, and xpc-shell tests.
|
|
||
Updated•4 years ago
|
|
|
Assignee | |
Comment 13•4 years ago
|
||
Changing Error objects to use a more specific class-name didn't work out for devtools, because they are testing object.class === "Error" to test for Error objects, so I had to revert that part.
|
|
Reporter | |
Comment 14•4 years ago
|
||
(In reply to André Bargull [:anba] from comment #13)
Changing Error objects to use a more specific class-name didn't work out for devtools, because they are testing
object.class === "Error"to test for Error objects, so I had to revert that part.
Okay. We should probably introduce Debugger.Object.isError and change devtools to use that.
|
||
Comment 15•4 years ago
|
||
Pushed by dluca@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8fb77958bcda Part 1: Change class-name from "Object" to the actual name of the class. r=evilpie https://hg.mozilla.org/integration/autoland/rev/f19da76c9c1e Part 2: Change class-name for prototype objects from "Object" to "Thing.prototype". r=evilpie https://hg.mozilla.org/integration/autoland/rev/a576109df99c Part 3: Expand comment why class-name for error objects is "Error". r=evilpie https://hg.mozilla.org/integration/autoland/rev/7958608bfaba Part 4: Change class-name for Streams prototype objects from "object" to "Thing.prototype". r=evilpie https://hg.mozilla.org/integration/autoland/rev/e9795038bab7 Part 5: Change class-name for (Async)Iterator prototypes from "(Async)Iterator" to "(Async)Iterator.prototype". r=evilpie https://hg.mozilla.org/integration/autoland/rev/3fe60ca27593 Part 6: Change class-name for Promise.prototype from "PromiseProto" to "Promise.prototype". r=evilpie https://hg.mozilla.org/integration/autoland/rev/fb6df25c3a13 Part 7: Change class-name for prototype objects from "ThingPrototype" to "Thing.prototype". r=evilpie
|
|
Assignee | |
Comment 16•4 years ago
|
||
(In reply to Tom Schuster [:evilpie] from comment #14)
Okay. We should probably introduce
Debugger.Object.isErrorand change devtools to use that.
Filed as bug 1650776.
|
||
Comment 17•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/8fb77958bcda
https://hg.mozilla.org/mozilla-central/rev/f19da76c9c1e
https://hg.mozilla.org/mozilla-central/rev/a576109df99c
https://hg.mozilla.org/mozilla-central/rev/7958608bfaba
https://hg.mozilla.org/mozilla-central/rev/e9795038bab7
https://hg.mozilla.org/mozilla-central/rev/3fe60ca27593
https://hg.mozilla.org/mozilla-central/rev/fb6df25c3a13
Description
•