Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(9)

Issue 2285743003: Use StringView for v8String(). (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 months, 4 weeks ago by esprehn
Modified:
10 months, 4 weeks ago
CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use StringView for v8String(). By using StringView here we can avoid allocating externalized strings for all of the literal strings used like v8String(isolate, "example") where we would first allocate a WTF::String then externalize it into a v8::String. Instead we can just go directly to a v8::String which is faster and uses less memory. BUG=615174 Committed: https://crrev.com/d4a17e1863ab232b718caeb4afdd4252af99cef0 Cr-Commit-Position: refs/heads/master@{#414920}

Patch Set 1 #

Total comments: 3

Messages

Total messages: 19 (10 generated)
esprehn
10 months, 4 weeks ago (2016-08-27 01:16:40 UTC) #5
haraken
https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/bindings/core/v8/V8Binding.h File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/bindings/core/v8/V8Binding.h#newcode409 third_party/WebKit/Source/bindings/core/v8/V8Binding.h:409: return v8::String::NewFromOneByte(isolate, reinterpret_cast<const uint8_t*>(string.characters8()), v8::NewStringType::kNormal, static_cast<int>(string.length())).ToLocalChecked(); Don't you want ...
10 months, 4 weeks ago (2016-08-27 01:21:46 UTC) #6
esprehn
https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/bindings/core/v8/V8Binding.h File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/bindings/core/v8/V8Binding.h#newcode409 third_party/WebKit/Source/bindings/core/v8/V8Binding.h:409: return v8::String::NewFromOneByte(isolate, reinterpret_cast<const uint8_t*>(string.characters8()), v8::NewStringType::kNormal, static_cast<int>(string.length())).ToLocalChecked(); On 2016/08/27 at ...
10 months, 4 weeks ago (2016-08-27 02:18:57 UTC) #9
haraken
On 2016/08/27 02:18:57, esprehn wrote: > https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/bindings/core/v8/V8Binding.h > File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): > > https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/bindings/core/v8/V8Binding.h#newcode409 > ...
10 months, 4 weeks ago (2016-08-27 02:21:52 UTC) #10
esprehn
https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/bindings/core/v8/V8Binding.h File third_party/WebKit/Source/bindings/core/v8/V8Binding.h (right): https://codereview.chromium.org/2285743003/diff/1/third_party/WebKit/Source/bindings/core/v8/V8Binding.h#newcode409 third_party/WebKit/Source/bindings/core/v8/V8Binding.h:409: return v8::String::NewFromOneByte(isolate, reinterpret_cast<const uint8_t*>(string.characters8()), v8::NewStringType::kNormal, static_cast<int>(string.length())).ToLocalChecked(); ex. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/ScriptString.cpp?q=%22v8String(%22&sq=package:chromium&l=68&dr=C that ...
10 months, 4 weeks ago (2016-08-27 02:24:03 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2285743003/1
10 months, 4 weeks ago (2016-08-27 02:25:12 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2285743003/1
10 months, 4 weeks ago (2016-08-27 08:26:32 UTC) #16
commit-bot: I haz the power
Committed patchset #1 (id:1)
10 months, 4 weeks ago (2016-08-27 11:05:25 UTC) #17
commit-bot: I haz the power
10 months, 4 weeks ago (2016-08-27 11:08:39 UTC) #19
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/d4a17e1863ab232b718caeb4afdd4252af99cef0
Cr-Commit-Position: refs/heads/master@{#414920}
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 25c286973