Closed
Bug 1014114
Opened 10 years ago
Closed 10 years ago
Self-host string HTML extensions
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(1 file, 1 obsolete file)
12.73 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
These functions like "foo".bold() => "<b>foo</b>" are simple to self-host. The attached patch does this, so that I don't have to refactor the C++ code to handle latin1 strings. 2 files changed, 68 insertions(+), 177 deletions(-) (And most of the added lines are JS instead of C++.) These functions are not standardized, so I just followed the C++ implementation and V8's self-hosted implementation. I also added a jit-test and made sure it passes without the other changes.
Attachment #8426391 -
Flags: review?(till)
Comment 1•10 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #0) > These functions are not standardized, so I just followed the C++ > implementation and V8's self-hosted implementation. I also added a jit-test > and made sure it passes without the other changes. ES6 adds a specification for these functions: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-additional-properties-of-the-string.prototype-object (As far as I can see, ) The proposed changes will give a different result for: --- String.prototype.fixed.call({toString: () => 1, valueOf: () => 2}) --- Before: "<tt>1</tt>" After: "<tt>2</tt>" (V8 also returns this string)
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8426391 [details] [diff] [review] Patch Thanks André, that's good to know. I'll update the patch. There's also a difference in argument evaluation order. I was aware of that, but if these functions are being standardized we should fix it.
Attachment #8426391 -
Flags: review?(till)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8426391 -
Attachment is obsolete: true
Attachment #8426457 -
Flags: review?(till)
Comment 4•10 years ago
|
||
Comment on attachment 8426457 [details] [diff] [review] Patch Review of attachment 8426457 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/String.js @@ +254,5 @@ > + return "<sup>" + ToString(this) + "</sup>"; > +} > + > +function EscapeAttributeValue(v) { > + return ToString(v).replace(/"/g, """); removeUnicodeExtensions() in builtin/Intl.js contains a note that directly calling String.prototype.replace with a RegExp object is not allowed in self-hosted code, because it sets alters the RegExp statics. Apart from that issue, shouldn't that line read `callFunction(std_String_replace, ...)`?
Assignee | ||
Comment 5•10 years ago
|
||
Good points again. I'm not very familiar with our self-hosting infrastructure. Till, can you tell me what's the best way to do this?
Comment 6•10 years ago
|
||
Comment on attachment 8426457 [details] [diff] [review] Patch Review of attachment 8426457 [details] [diff] [review]: ----------------------------------------------------------------- Nice :) One thing, though: as jorendorff recently discovered, ToString as defined in Utilities.js uses std_String for the coercion. That is valid, but wasteful, as it causes the entire String object and its prototype to be cloned over into the using compartment. Can you simply replace that implementation with `return v + ''`? That should have exactly the same result, but without the overhead. ::: js/src/builtin/String.js @@ +254,5 @@ > + return "<sup>" + ToString(this) + "</sup>"; > +} > + > +function EscapeAttributeValue(v) { > + return ToString(v).replace(/"/g, """); Correct: this would need to use `callFunction`. However, as André also correctly says, .replace can't be used here at all. The harder solution would be implement a .replace equivalent to the `regexp_exec_no_statics` intrinsic used in Intl.js. However, it shouldn't be needed here, really, as this should do the trick, too: var inputStr = ToString(v); var outputStr = ''; var chunkStart = 0; for (var i = 0; i < inputStr.length; i++) { if (inputStr[i] === '"') { outputStr += callFunction(std_String_substring, inputStr, chunkStart, i); outputStr += inputStr.substring(chunkStart, i) + '"'; chunkStart = i + 1; } } if (chunkStart < inputStr.length) { outputStr += callFunction(std_String_substring, inputStr, chunkStart); } return outputStr; (Also, you can either leave out the ToString here or in the callers. Maybe replace one with an assert.)
Attachment #8426457 -
Flags: review?(till) → review+
Comment 7•10 years ago
|
||
(In reply to Till Schneidereit [:till] from comment #6) > Can you simply replace that implementation with > `return v + ''`? That should have exactly the same result, but without the > overhead. ToString(ToPrimitive(v)) is not the same as ToString(v), see comment #1 ;-)
Comment 8•10 years ago
|
||
(In reply to André Bargull from comment #7) > (In reply to Till Schneidereit [:till] from comment #6) > > Can you simply replace that implementation with > > `return v + ''`? That should have exactly the same result, but without the > > overhead. > > ToString(ToPrimitive(v)) is not the same as ToString(v), see comment #1 ;-) Urgh. :( Ok, I'll change ToString to an intrinsic tomorrow. We can land that before this patch and should be good on that front. (In reply to Jan de Mooij [:jandem] from comment #5) > Good points again. I'm not very familiar with our self-hosting > infrastructure. > > Till, can you tell me what's the best way to do this? In general, I try to keep our self-hosting wiki page[1] up to date. Let me know if some important information is missing from that. [1]: https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/self-hosting
Assignee | ||
Comment 9•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/184fd695b135 Green Linux64 Try run: https://tbpl.mozilla.org/?tree=Try&rev=43c50ee89dc8 (In reply to Till Schneidereit [:till] from comment #6) > The harder solution would be implement a .replace equivalent to the > `regexp_exec_no_statics` intrinsic used in Intl.js. However, it shouldn't be > needed here, really, as this should do the trick, too: Great, that works nicely :) (In reply to Till Schneidereit [:till] from comment #8) > In general, I try to keep our self-hosting wiki page[1] up to date. Let me > know if some important information is missing from that. > > [1]: > https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/self-hosting Thanks, I'll take a look.
Comment 10•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/184fd695b135
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•