Use v8AtomicString instead of v8::String::NewFromUtf8

Use v8AtomicString instead of v8::String::NewFromUtf8

We should use utility methods as much as possible.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164510

[haraken, 2013-12-29 12:09:54]

PTAL

[sof, 2013-12-29 13:20:11]

https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
File Source/bindings/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
Source/bindings/v8/V8DOMConfiguration.cpp:72:
functionDescriptor->Set(v8AtomicString(isolate, constant->name),
v8::Integer::New(isolate, constant->value),
static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete));
Possible to avoid duplicate calls ?

[haraken, 2013-12-29 13:47:56]

https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
File Source/bindings/v8/V8DOMConfiguration.cpp (right):

https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
Source/bindings/v8/V8DOMConfiguration.cpp:72:
functionDescriptor->Set(v8AtomicString(isolate, constant->name),
v8::Integer::New(isolate, constant->value),
static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete));
On 2013/12/29 13:20:12, sof wrote:
> Possible to avoid duplicate calls ?

Done.

[sof, 2013-12-29 20:35:46]

On 2013/12/29 13:47:56, haraken wrote:
>
https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
> File Source/bindings/v8/V8DOMConfiguration.cpp (right):
> 
>
https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
> Source/bindings/v8/V8DOMConfiguration.cpp:72:
> functionDescriptor->Set(v8AtomicString(isolate, constant->name),
> v8::Integer::New(isolate, constant->value),
> static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete));
> On 2013/12/29 13:20:12, sof wrote:
> > Possible to avoid duplicate calls ?
> 
> Done.

Thanks, looks fine from here.

However, I do notice that some exception messages are now internalized that
weren't before -- should I infer from that that the barrier to internalizing
strings should be kept really low & frequency of use not be considered so much
when deciding for/against?

[dcarney, 2013-12-29 21:43:14]

On 2013/12/29 20:35:46, sof wrote:
> On 2013/12/29 13:47:56, haraken wrote:
> >
>
https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
> > File Source/bindings/v8/V8DOMConfiguration.cpp (right):
> > 
> >
>
https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
> > Source/bindings/v8/V8DOMConfiguration.cpp:72:
> > functionDescriptor->Set(v8AtomicString(isolate, constant->name),
> > v8::Integer::New(isolate, constant->value),
> > static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete));
> > On 2013/12/29 13:20:12, sof wrote:
> > > Possible to avoid duplicate calls ?
> > 
> > Done.
> 
> Thanks, looks fine from here.
> 
> However, I do notice that some exception messages are now internalized that
> weren't before -- should I infer from that that the barrier to internalizing
> strings should be kept really low & frequency of use not be considered so much
> when deciding for/against?

Internalized strings can be costly.  They are pretenured and are only cleared on
major gc.  If the string table is filled with short lived objects, it will be
often resized, which is highly wasteful.

They should be used, for instance, in named property setter/getters, and any
other place a string will anyway be internalized.  Also, any place where you
need a static string often.

Error messages are likely short lived and don't need to be internalized.

[haraken, 2013-12-30 04:44:50]

On 2013/12/29 21:43:14, dcarney wrote:
> On 2013/12/29 20:35:46, sof wrote:
> > On 2013/12/29 13:47:56, haraken wrote:
> > >
> >
>
https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
> > > File Source/bindings/v8/V8DOMConfiguration.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
> > > Source/bindings/v8/V8DOMConfiguration.cpp:72:
> > > functionDescriptor->Set(v8AtomicString(isolate, constant->name),
> > > v8::Integer::New(isolate, constant->value),
> > > static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete));
> > > On 2013/12/29 13:20:12, sof wrote:
> > > > Possible to avoid duplicate calls ?
> > > 
> > > Done.
> > 
> > Thanks, looks fine from here.
> > 
> > However, I do notice that some exception messages are now internalized that
> > weren't before -- should I infer from that that the barrier to internalizing
> > strings should be kept really low & frequency of use not be considered so
much
> > when deciding for/against?
> 
> Internalized strings can be costly.  They are pretenured and are only cleared
on
> major gc.  If the string table is filled with short lived objects, it will be
> often resized, which is highly wasteful.
> 
> They should be used, for instance, in named property setter/getters, and any
> other place a string will anyway be internalized.  Also, any place where you
> need a static string often.
> 
> Error messages are likely short lived and don't need to be internalized.

Then, as a general rule, how should we distinguish:

- v8::String::NewFromUtf8(kNormalString)
- v8::String::NewFromUtf8(kInternalizedString)
- v8String()

?

[sof, 2013-12-30 11:53:41]

On 2013/12/30 04:44:50, haraken wrote:
> On 2013/12/29 21:43:14, dcarney wrote:
> > On 2013/12/29 20:35:46, sof wrote:
> > > On 2013/12/29 13:47:56, haraken wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
> > > > File Source/bindings/v8/V8DOMConfiguration.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/116983005/diff/1/Source/bindings/v8/V8DOMConf...
> > > > Source/bindings/v8/V8DOMConfiguration.cpp:72:
> > > > functionDescriptor->Set(v8AtomicString(isolate, constant->name),
> > > > v8::Integer::New(isolate, constant->value),
> > > > static_cast<v8::PropertyAttribute>(v8::ReadOnly | v8::DontDelete));
> > > > On 2013/12/29 13:20:12, sof wrote:
> > > > > Possible to avoid duplicate calls ?
> > > > 
> > > > Done.
> > > 
> > > Thanks, looks fine from here.
> > > 
> > > However, I do notice that some exception messages are now internalized
that
> > > weren't before -- should I infer from that that the barrier to
internalizing
> > > strings should be kept really low & frequency of use not be considered so
> much
> > > when deciding for/against?
> > 
> > Internalized strings can be costly.  They are pretenured and are only
cleared
> on
> > major gc.  If the string table is filled with short lived objects, it will
be
> > often resized, which is highly wasteful.
> > 
> > They should be used, for instance, in named property setter/getters, and any
> > other place a string will anyway be internalized.  Also, any place where you
> > need a static string often.
> > 
> > Error messages are likely short lived and don't need to be internalized.
> 
> Then, as a general rule, how should we distinguish:
> 
> - v8::String::NewFromUtf8(kNormalString)
> - v8::String::NewFromUtf8(kInternalizedString)
> - v8String()
> 
> ?

Strawman:

- property names (and perhaps all IDL named entities) gets an internalized
string.
- const char*s of rare, temporary, use maps to kNormalString
- WTF::Strings (and constant strings that are used "more often") use v8 external
strings.

[haraken, 2013-12-30 17:42:51]

> > Then, as a general rule, how should we distinguish:
> > 
> > - v8::String::NewFromUtf8(kNormalString)
> > - v8::String::NewFromUtf8(kInternalizedString)
> > - v8String()
> > 
> > ?
> 
> Strawman:
> 
> - property names (and perhaps all IDL named entities) gets an internalized
> string.
> - const char*s of rare, temporary, use maps to kNormalString
> - WTF::Strings (and constant strings that are used "more often") use v8
external
> strings.

I agree that we should distinguish v8::String::NewFromUtf8(kInternalizedString)
and v8String(), but would it be worth distinguishing
v8::String::NewFromUtf8(kNormalString) and v8String()?

[eseidel, 2013-12-30 18:38:05]

v8LongLivedAtom(String)
v8TemporaryAtom(String)
?

[haraken, 2013-12-30 18:59:30]

On 2013/12/30 18:38:05, eseidel wrote:
> v8LongLivedAtom(String)
> v8TemporaryAtom(String)
> ?

Yeah, I agree with the point that two types would be enough (instead of three
types).

[sof, 2013-12-30 19:14:47]

On 2013/12/30 18:59:30, haraken wrote:
> On 2013/12/30 18:38:05, eseidel wrote:
> > v8LongLivedAtom(String)
> > v8TemporaryAtom(String)
> > ?
> 
> Yeah, I agree with the point that two types would be enough (instead of three
> types).

v8String() strikes me as a heavy primop, but perhaps there's not many uses of
NewFromUtf8(kNormalString) around. ?

[haraken, 2014-01-01 02:41:53]

Either way, I reverted the controversial part from this CL. I just replaced
v8::String::NewFromUtf8(kInternalizedString) with v8AtomicString.

PTAL?

[dcarney, 2014-01-01 13:13:48]

lgtm

https://codereview.chromium.org/116983005/diff/310001/Source/bindings/v8/Scri...
File Source/bindings/v8/ScriptDebugServer.cpp (right):

https://codereview.chromium.org/116983005/diff/310001/Source/bindings/v8/Scri...
Source/bindings/v8/ScriptDebugServer.cpp:546: return
m_isolate->ThrowException(v8::String::NewFromUtf8(m_isolate, "Debugging is not
enabled.", v8::String::kNormalString));
kNormalString is the default, and it even sounds like the default, so i'd say
drop the explicit use of it here

https://codereview.chromium.org/116983005/diff/310001/Source/bindings/v8/V8DO...
File Source/bindings/v8/V8DOMConfiguration.h (right):

https://codereview.chromium.org/116983005/diff/310001/Source/bindings/v8/V8DO...
Source/bindings/v8/V8DOMConfiguration.h:77: (attribute.onPrototype ? prototype :
instanceTemplate)->SetAccessor(v8::String::NewFromUtf8(isolate, attribute.name,
v8::String::kInternalizedString),
should be v8AtomicString here

https://codereview.chromium.org/116983005/diff/310001/Source/bindings/v8/cust...
File Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp (right):

https://codereview.chromium.org/116983005/diff/310001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:344:
isolate->ThrowException(v8::Exception::Error(v8::String::NewFromUtf8(isolate,
"One argument expected.", v8::String::kNormalString)));
kNormalString, as previously mentioned

https://codereview.chromium.org/116983005/diff/310001/Source/bindings/v8/cust...
Source/bindings/v8/custom/V8InjectedScriptHostCustom.cpp:350:
isolate->ThrowException(v8::Exception::Error(v8::String::NewFromUtf8(isolate,
"The argument must be a string.", v8::String::kNormalString)));
kNormalString, as previously mentioned

[haraken, 2014-01-06 14:34:13]

Thanks Dan, all comments addressed.

[commit-bot: I haz the power, 2014-01-06 14:34:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/116983005/390001

[commit-bot: I haz the power, 2014-01-06 14:34:32]

Failed to apply patch for Source/bindings/templates/interface.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/templates/interface.cpp
  Hunk #2 succeeded at 500 (offset -9 lines).
  Hunk #3 FAILED at 520.
  Hunk #4 succeeded at 533 (offset -10 lines).
  Hunk #5 succeeded at 544 (offset -10 lines).
  Hunk #6 succeeded at 642 (offset -10 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
Source/bindings/templates/interface.cpp.rej

Patch:       Source/bindings/templates/interface.cpp
Index: Source/bindings/templates/interface.cpp
diff --git a/Source/bindings/templates/interface.cpp
b/Source/bindings/templates/interface.cpp
index
440c327d7e16753ff5888b986f042a6a6e2783ab..ff57b50e7af4e873e0b823fb3988f9339f91be9a
100644
--- a/Source/bindings/templates/interface.cpp
+++ b/Source/bindings/templates/interface.cpp
@@ -154,7 +154,7 @@ v8::Handle<v8::FunctionTemplate>
{{v8_class}}Constructor::domTemplate(v8::Isolat
 
     v8::Local<v8::ObjectTemplate> instanceTemplate =
result->InstanceTemplate();
     instanceTemplate->SetInternalFieldCount({{v8_class}}::internalFieldCount);
-    result->SetClassName(v8::String::NewFromUtf8(isolate, "{{cpp_class}}",
v8::String::kInternalizedString));
+    result->SetClassName(v8AtomicString(isolate, "{{cpp_class}}"));
     result->Inherit({{v8_class}}::domTemplate(isolate, currentWorldType));
     data->setPrivateTemplate(currentWorldType, &privateTemplateUniqueKey,
result);
 
@@ -509,7 +509,7 @@ static v8::Handle<v8::FunctionTemplate>
Configure{{v8_class}}Template(v8::Handle
     {% for attribute in attributes if attribute.is_static %}
     {% set getter_callback = '%sV8Internal::%sAttributeGetterCallback' %
            (interface_name, attribute.name) %}
-    functionTemplate->SetNativeDataProperty(v8::String::NewFromUtf8(isolate,
"{{attribute.name}}", v8::String::kInternalizedString), {{getter_callback}},
{{attribute.setter_callback}}, v8::External::New(isolate, 0),
static_cast<v8::PropertyAttribute>(v8::None),
v8::Handle<v8::AccessorSignature>(),
static_cast<v8::AccessControl>(v8::DEFAULT));
+    functionTemplate->SetNativeDataProperty(v8AtomicString(isolate,
"{{attribute.name}}"), {{getter_callback}}, {{attribute.setter_callback}},
v8::External::New(isolate, 0), static_cast<v8::PropertyAttribute>(v8::None),
v8::Handle<v8::AccessorSignature>(),
static_cast<v8::AccessControl>(v8::DEFAULT));
     {% endfor %}
     {% if has_custom_legacy_call_as_function %}
    
functionTemplate->InstanceTemplate()->SetCallAsFunctionHandler({{v8_class}}::legacyCallCustom);
@@ -520,7 +520,7 @@ static v8::Handle<v8::FunctionTemplate>
Configure{{v8_class}}Template(v8::Handle
     {% endif %}
 
     // Custom toString template
-    functionTemplate->Set(v8::String::NewFromUtf8(isolate, "toString",
v8::String::kInternalizedString),
V8PerIsolateData::current()->toStringTemplate());
+    functionTemplate->Set(v8AtomicString(isolate, "toString"),
V8PerIsolateData::current()->toStringTemplate());
     return functionTemplate;
 }
 
@@ -543,7 +543,7 @@ static v8::Handle<v8::FunctionTemplate>
Configure{{v8_class}}Template(v8::Handle
 {% set property_attribute =
     'static_cast<v8::PropertyAttribute>(%s)' %
     ' | '.join(method.property_attributes or ['v8::DontDelete']) %}
-{{method.function_template}}->SetAccessor(v8::String::NewFromUtf8(isolate,
"{{method.name}}", v8::String::kInternalizedString), {{getter_callback}},
{{setter_callback}}, v8Undefined(), v8::ALL_CAN_READ, {{property_attribute}});
+{{method.function_template}}->SetAccessor(v8AtomicString(isolate,
"{{method.name}}"), {{getter_callback}}, {{setter_callback}}, v8Undefined(),
v8::ALL_CAN_READ, {{property_attribute}});
 {%- endmacro %}
 
 
@@ -554,7 +554,7 @@ static v8::Handle<v8::FunctionTemplate>
Configure{{v8_class}}Template(v8::Handle
                          (interface_name, method.name, world_suffix) %}
 {% set property_attribute = 'static_cast<v8::PropertyAttribute>(%s)' %
                             ' | '.join(method.property_attributes) %}
-{{method.function_template}}->Set(v8::String::NewFromUtf8(isolate,
"{{method.name}}", v8::String::kInternalizedString),
v8::FunctionTemplate::New(isolate, {{method_callback}}, v8Undefined(),
{{method.signature}}, {{method.number_of_required_or_variadic_arguments}}){% if
method.property_attributes %}, {{property_attribute}}{% endif %});
+{{method.function_template}}->Set(v8AtomicString(isolate, "{{method.name}}"),
v8::FunctionTemplate::New(isolate, {{method_callback}}, v8Undefined(),
{{method.signature}}, {{method.number_of_required_or_variadic_arguments}}){% if
method.property_attributes %}, {{property_attribute}}{% endif %});
 {%- endmacro %}
 
 
@@ -652,7 +652,7 @@ void
{{v8_class}}::installPerContextEnabledMethods(v8::Handle<v8::Object> protot
     ExecutionContext* context =
toExecutionContext(prototypeTemplate->CreationContext());
     {% for method in methods if method.per_context_enabled_function %}
     if (context && context->isDocument() &&
{{method.per_context_enabled_function}}(toDocument(context)))
-        prototypeTemplate->Set(v8::String::NewFromUtf8(isolate,
"{{method.name}}", v8::String::kInternalizedString),
v8::FunctionTemplate::New(isolate,
{{cpp_class}}V8Internal::{{method.name}}MethodCallback, v8Undefined(),
defaultSignature, {{method.number_of_required_arguments}})->GetFunction());
+        prototypeTemplate->Set(v8AtomicString(isolate, "{{method.name}}"),
v8::FunctionTemplate::New(isolate,
{{cpp_class}}V8Internal::{{method.name}}MethodCallback, v8Undefined(),
defaultSignature, {{method.number_of_required_arguments}})->GetFunction());
     {% endfor %}
 }

[commit-bot: I haz the power, 2014-01-06 14:49:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/116983005/480001

[commit-bot: I haz the power, 2014-01-06 15:08:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/116983005/540002

[commit-bot: I haz the power, 2014-01-06 15:17:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/116983005/620001

[commit-bot: I haz the power, 2014-01-06 16:17:28]

Change committed as 164510