[v8] More correct and faster error handling when converting v8 objects to various WebCore strings
Created attachment 65550 [details] Patch
Comment on attachment 65550 [details] Patch Aside from the comment below, this looks reasonable. I'm a bit too tired to think it through entirely though, so I'm leaving it r? for now. WebCore/bindings/v8/V8Binding.cpp:375 + static AtomicString lowNumbers[kLowNumbers + 1]; Is this threadsafe? What if a worker and a normal document both use the number 42?
Created attachment 65706 [details] Patch
(In reply to comment #2) > (From update of attachment 65550 [details]) > Aside from the comment below, this looks reasonable. I'm a bit too tired to think it through entirely though, so I'm leaving it r? for now. > > WebCore/bindings/v8/V8Binding.cpp:375 > + static AtomicString lowNumbers[kLowNumbers + 1]; > Is this threadsafe? What if a worker and a normal document both use the number 42? Good catch. I don't think it's thread safe. However AFAIK we are still single threaded. So I've added an ASSERT on top of the function with some clarification. Do you think it's enough for now? And are you fine with the wording?
Comment on attachment 65706 [details] Patch > WebCore/bindings/v8/V8Binding.cpp:377 > + // Most numbers used are <= 100. Even if they aren't used there's very little in using the space. very little cost? Seems we're missing a word here. > WebCore/bindings/v8/V8Binding.cpp:379 > + static AtomicString lowNumbers[kLowNumbers + 1]; Shouldn't we use DECLARE_STATIC_LOCAL here? > WebCore/bindings/v8/V8Binding.cpp:384 > + AtomicString valueString = AtomicString(String::number(value)); Is there an advantage to using atomic strings here? It seems like you're already doing the atomization. > WebCore/bindings/v8/V8Binding.cpp:397 > + int32ToWebCoreString(object->Int32Value()); Why don't we use the return value here? Is this code tested??? > WebCore/bindings/v8/V8Binding.h:218 > + operator AtomicString() { return toString<AtomicString>(); } I think we'd be better off if we had explicit conversion operators. In general, WebKit frown on these implicit operators. > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:114 > + STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(V8Parameter<>, type, args[0]); Why this particular callsite? Is this the only custom one?
Created attachment 65934 [details] Patch
(In reply to comment #5) > (From update of attachment 65706 [details]) > > WebCore/bindings/v8/V8Binding.cpp:377 > > + // Most numbers used are <= 100. Even if they aren't used there's very little in using the space. > very little cost? Seems we're missing a word here. Added 'cost' > > > WebCore/bindings/v8/V8Binding.cpp:379 > > + static AtomicString lowNumbers[kLowNumbers + 1]; > Shouldn't we use DECLARE_STATIC_LOCAL here? Adam, the code was copied verbatim. Could we postpone this DECLARE_STATIC_LOCAL for a next commit? I'd like to minimize changes to the code which existed before my change. > > > WebCore/bindings/v8/V8Binding.cpp:384 > > + AtomicString valueString = AtomicString(String::number(value)); > Is there an advantage to using atomic strings here? It seems like you're already doing the atomization. I think yes, as AtomicString to String conversion is cheaper then conversion back if I understand it correctly. Again, I don't change this code. > > > WebCore/bindings/v8/V8Binding.cpp:397 > > + int32ToWebCoreString(object->Int32Value()); > Why don't we use the return value here? Is this code tested??? Very good catch, thank you very much. Alas, it's not obvious how to test it: it's only an optimization: we fall through to heavyweight toString conversion which would do the trick anyway. > > > WebCore/bindings/v8/V8Binding.h:218 > > + operator AtomicString() { return toString<AtomicString>(); } > I think we'd be better off if we had explicit conversion operators. In general, WebKit frown on these implicit operators. Those operators are crucial to solve the following problem: some WebCore DOM methods take Strings and some AtomicStrings. Alas, in IDL there is no such information (we used to have AtomicHint). So we need to find the best conversion method and those C++ operators fit very nicely. I drafted a solution with metaprogramming (using SFINAE to find out exact argument type), but it's more verbose and rumors have it VC used to be weak in templates with pointers-to-members parameters (just read it, didn't check if it's indeed the case). > > > WebCore/bindings/v8/custom/V8DeviceMotionEventCustom.cpp:114 > > + STRING_TO_V8PARAMETER_EXCEPTION_BLOCK(V8Parameter<>, type, args[0]); > Why this particular callsite? Is this the only custom one? Yes. I double checked and found another one. I'll have to check again before I submit it. BTW, if in review you could ask people to use the macro when converting parameters, that would be most appreciated. Thank you very much for a next round of comments.
Comment on attachment 65934 [details] Patch Thanks. > WebCore/bindings/scripts/CodeGeneratorV8.pm:3333 > + my $macro = "STRING_TO_V8PARAMETER_EXCEPTION_BLOCK"; four-space indent.
Created attachment 66018 [details] Patch
(In reply to comment #8) > (From update of attachment 65934 [details]) > Thanks. > > > WebCore/bindings/scripts/CodeGeneratorV8.pm:3333 > > + my $macro = "STRING_TO_V8PARAMETER_EXCEPTION_BLOCK"; > four-space indent. Sorry, Adam, what do you mean? Is it Perl statement which requires additional indentation or generated C++ string? Apparently both are fine: perl's at right indent and C++ doesn't require indentation---it's indented at call site. Anyway, if I manage to submit it before you reply, I'll fix it in a separate change. And many thanks for review.
Comment on attachment 66018 [details] Patch > WebCore/bindings/scripts/CodeGeneratorV8.pm:3339 > + if ($signature->type eq "DOMString") { > + my $macro = "STRING_TO_V8PARAMETER_EXCEPTION_BLOCK"; > + $macro .= "_$suffix" if $suffix; > + return "$macro($nativeType, $variableName, $value);" > + } else { > + # Don't know how to properly check for conversion exceptions when $parameter->type is "DOMUserData" > + return "$nativeType $variableName($value, true);"; > + } Notice that the contents of the if are only indented with two spaces instead of four.
Comment on attachment 66018 [details] Patch Clearing flags on attachment: 66018 Committed r66521: <http://trac.webkit.org/changeset/66521>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/66521 might have broken Chromium Linux Release
(In reply to comment #14) > http://trac.webkit.org/changeset/66521 might have broken Chromium Linux Release This broke the chromium compile: third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.cpp: In static member function ‘static v8::Handle<v8::Value> WebCore::V8DeviceOrientationEvent::initDeviceOrientationEventCallback(const v8::Arguments&)’: third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.cpp:72:error: expected primary-expression before ‘,’ token third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.cpp:72:error: ‘type’ was not declared in this scope third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.cpp:72:error: ‘STRING_TO_V8PARAMETER_EXCEPTION_BLOCK’ was not declared in this scope make: *** [out/Release/obj.target/webcore_remaining/third_party/WebKit/WebCore/bindings/v8/custom/V8DeviceOrientationEventCustom.o] Error 1
Looks like you didn't update the custom bindings.
(In reply to comment #16) > Looks like you didn't update the custom bindings. I have a compile fix coming.
http://trac.webkit.org/changeset/66530
(In reply to comment #11) > (From update of attachment 66018 [details]) > > WebCore/bindings/scripts/CodeGeneratorV8.pm:3339 > > + if ($signature->type eq "DOMString") { > > + my $macro = "STRING_TO_V8PARAMETER_EXCEPTION_BLOCK"; > > + $macro .= "_$suffix" if $suffix; > > + return "$macro($nativeType, $variableName, $value);" > > + } else { > > + # Don't know how to properly check for conversion exceptions when $parameter->type is "DOMUserData" > > + return "$nativeType $variableName($value, true);"; > > + } > Notice that the contents of the if are only indented with two spaces instead of four. Sorry, didn't notice that. Fixing ASAP.
(In reply to comment #18) > http://trac.webkit.org/changeset/66530 Thank you very much, Tony.
Created attachment 66209 [details] Patch
(In reply to comment #21) > Created an attachment (id=66209) [details] > Patch It's ok to commit whitespace changes like this without review.
Comment on attachment 66209 [details] Patch thanks a lot, Tony
to kick cq.
Comment on attachment 66209 [details] Patch Clearing flags on attachment: 66209 Committed r66664: <http://trac.webkit.org/changeset/66664>