Summary: | [V8Bindings] Implement generalised method to validates that the passed object is a sequence type. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||
Component: | New Bugs | Assignee: | Vineet Chaudhary (vineetc) <code.vineet> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, haraken, japhet, jochen, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Vineet Chaudhary (vineetc)
2012-07-12 01:15:09 PDT
Created attachment 151882 [details]
Patch
I thought of removing custom code from V8Utilities.cpp in another patch. Please let me know if we want that in one patch. Comment on attachment 151882 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151882&action=review > Source/WebCore/bindings/v8/V8Binding.h:444 > + inline v8::Handle<v8::Object> toV8Sequence(v8::Handle<v8::Value> value, uint32_t& length) I think the current toJSSequence() is conformed to the spec. Would you implement toV8Sequence() so that it exactly corresponds to toJSSequence()? (toV8Sequence() in this patch is not conformed to the spec and has a couple of bugs.) > Source/WebCore/bindings/v8/V8Binding.h:447 > + V8Proxy::throwTypeError("TransferArray argument must be an object"); We want to make this message the same as the toJSSequence()'s one. > Source/WebCore/bindings/v8/V8Binding.h:455 > + v8::Local<v8::Array> array = v8::Local<v8::Array>::Cast(v8Value); > + length = array->Length(); You are doing something like: array = ...; length = ...; return object; which would be wrong. (In reply to comment #2) > I thought of removing custom code from V8Utilities.cpp in another patch. > Please let me know if we want that in one patch. Another patch is fine. Created attachment 151891 [details]
Updated Patch
Updated patch with incorporating review comments.
Comment on attachment 151891 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151891&action=review > Source/WebCore/bindings/v8/V8Binding.h:447 > + V8Proxy::throwTypeError("Type error"); To align with toJSSequence(), this could be V8Proxy::throwTypeError(). (though it would just print "Type error".) > Source/WebCore/bindings/v8/V8Binding.h:448 > + return v8::Local<v8::Object>(); You can simply write 'return V8Proxy::throwTypeError()'. > Source/WebCore/bindings/v8/V8Binding.h:453 > + v8::Local<v8::Value> lengthValue = object->Get(v8::String::New("length")); Isn't there any possibility that this code throws exception? Then we need to use TryCatch. (See macros in V8BindingMacros.h) > Source/WebCore/bindings/v8/V8Binding.h:457 > + V8Proxy::throwTypeError("Type error"); > + return v8::Local<v8::Object>(); return V8Proxy::throwTypeError(); > Source/WebCore/bindings/v8/V8Binding.h:463 > + if (!lengthValue->IsNumber()) { > + V8Proxy::throwTypeError("Type error"); > + return v8::Local<v8::Object>(); > + } Is this check needed? toJSSequence() does not have the check. > Source/WebCore/bindings/v8/V8Binding.h:465 > + length = lengthValue->Uint32Value(); Isn't there any possibility that this code throws exception? Then we need to use TryCatch. (See macros in V8BindingMacros.h) Comment on attachment 151891 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151891&action=review >> Source/WebCore/bindings/v8/V8Binding.h:448 >> + return v8::Local<v8::Object>(); > > You can simply write 'return V8Proxy::throwTypeError()'. actually throwTypeError() returns v8::Value. Then may be might have Cast it to v8::Object >> Source/WebCore/bindings/v8/V8Binding.h:453 >> + v8::Local<v8::Value> lengthValue = object->Get(v8::String::New("length")); > > Isn't there any possibility that this code throws exception? Then we need to use TryCatch. (See macros in V8BindingMacros.h) Oke I will do. Comment on attachment 151891 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151891&action=review >>> Source/WebCore/bindings/v8/V8Binding.h:448 >>> + return v8::Local<v8::Object>(); >> >> You can simply write 'return V8Proxy::throwTypeError()'. > > actually throwTypeError() returns v8::Value. Then may be might have Cast it to v8::Object What if we change the return type from v8::Handle<v8::Object> to v8::Handle<v8::Value>? Would it cause a problem in the caller side? (In reply to comment #8) > (From update of attachment 151891 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151891&action=review > > >>> Source/WebCore/bindings/v8/V8Binding.h:448 > >>> + return v8::Local<v8::Object>(); > >> > >> You can simply write 'return V8Proxy::throwTypeError()'. > > > > actually throwTypeError() returns v8::Value. Then may be might have Cast it to v8::Object > > What if we change the return type from v8::Handle<v8::Object> to v8::Handle<v8::Value>? Would it cause a problem in the caller side? The original code is v8::Local<v8::Object> transferrables = v8::Local<v8::Object>::Cast(value); The caller code with Object return type would look like uint32_t length = 0; v8::Hand<v8::Object> transferrables = toV8Sequence(value,length); if (transferrables.IsEmpty()) return false; And this "transferrables" further used. With return type v8::Value along with the original code we need to add check what throwTypeError() returns. BTW toJSSequence() returns JSObject. I was trying to use EXCEPTION_BLOCK(v8::Local<v8::Value>, lengthValue, object->Get(v8::String::New("length"))); and EXCEPTION_BLOCK(uint32_t, sequenceLength, lengthValue->Uint32Value()); But getting some compile errors like Source/WebKit/chromium/v8/include/v8.h: In constructor ‘v8::Handle<T>::Handle(v8::Handle<S>) [with S = v8::Value, T = v8::Object]’: Source/WebCore/bindings/v8/V8Binding.h:462:9: instantiated from here Source/WebKit/chromium/v8/include/v8.h:202:5: error: invalid conversion from ‘v8::Value*’ to ‘v8::Object*’ [-fpermissive] Created attachment 151949 [details]
test_patch
Created attachment 151958 [details]
Updated Patch
Updated patch.
Comment on attachment 151958 [details] Updated Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151958&action=review > Source/WebCore/bindings/v8/V8Binding.h:445 > + inline v8::Handle<v8::Value> toV8Sequence(v8::Handle<v8::Value> value, uint32_t& length) inline is to avoid multiple definition error in multiple header inclusion. > Source/WebCore/bindings/v8/V8Binding.h:462 > + EXCEPTION_BLOCK(uint32_t, sequenceLength, lengthValue->Int32Value()); using length gives error of multiple declaration error. Comment on attachment 151958 [details]
Updated Patch
OK, thank you very much for the detailed investigation!
Comment on attachment 151958 [details] Updated Patch Clearing flags on attachment: 151958 Committed r122555: <http://trac.webkit.org/changeset/122555> All reviewed patches have been landed. Closing bug. |