[Webkit-unassigned] [Bug 91850] add 7 bit strings capabilities to the v8 binding layer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=91850





--- Comment #30 from Adam Barth <abarth at webkit.org>  2012-10-18 15:12:52 PST ---
(From update of attachment 169406)
View in context: https://bugs.webkit.org/attachment.cgi?id=169406&action=review

> Source/WebCore/bindings/v8/V8PerIsolateData.cpp:116
> +            if (encoding == v8::String::ASCII_ENCODING)
> +                base = static_cast<WebCoreStringResource8*>(resource);
> +            else
> +                base = static_cast<WebCoreStringResource16*>(resource);

This code looks very strange.  Why is it important to use the right static cast?  Presumably WebCoreStringResourceBase is the only base class for WebCoreStringResource8 and WebCoreStringResource16, which means these static casts should be zero offsets.  We can add an ASSERT to that effect.

> Source/WebCore/bindings/v8/V8StringResource.cpp:82
> +    static const int inlineBufferSize = 16;

Is this constant related to something intrinsic to AtomicString?  if so, can we get the constant from AtomicString rather than having to duplicate it here?

> Source/WebCore/bindings/v8/V8StringResource.cpp:100
> +    v8String->WriteAscii(reinterpret_cast<char*>(buffer), 0, length, v8::String::PRESERVE_ASCII_NULL);

The NULL in this constant got me thinking.  Are we getting things correct w.r.t. null termination?  For example, the buffer in String isn't null terminated.  If V8 is trying to write a null terminator, the lengths will be off by one.  I guess that would show up in tests if we were goofing that up.

> Source/WebCore/bindings/v8/V8StringResource.cpp:125
> +        v8::String::Encoding encoding;
> +        v8::String::ExternalStringResourceBase* resource = v8String->GetExternalStringResourceBase(&encoding);
> +        if (LIKELY(!!resource)) {
> +            WebCoreStringResourceBase* base;
> +            if (encoding == v8::String::ASCII_ENCODING)
> +                base = static_cast<WebCoreStringResource8*>(resource);
> +            else
> +                base = static_cast<WebCoreStringResource16*>(resource);
> +            return StringTraits<StringType>::fromStringResource(base);
> +        }

This code looks largely duplicated from V8PerIsolateData.cpp.  Is there no way to share this logic?

I also don't understand why we need this tricky static_cast logic (same question as above)

> Source/WebCore/bindings/v8/V8StringResource.cpp:129
> -    if (!length)
> +    if (UNLIKELY(!length))

Did you measure this UNLIKELY as having a performance benefit?  It's most likely a no-op.

> Source/WebCore/bindings/v8/V8ValueCache.h:151
> +        return static_cast<WebCoreStringResource16*>(v8String->GetExternalStringResource());

Should we add an ASSERT about the encoding of v8String?

> Source/WebCore/bindings/v8/V8ValueCache.h:154
> +    explicit WebCoreStringResource16(const String& string): WebCoreStringResourceBase(string) { }
> +    explicit WebCoreStringResource16(const AtomicString& string): WebCoreStringResourceBase(string) { }

nit: Space before :

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.