[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 #26 from Eric Seidel <eric at webkit.org>  2012-10-17 14:18:04 PST ---
(From update of attachment 169187)
View in context: https://bugs.webkit.org/attachment.cgi?id=169187&action=review

> Source/WebCore/ChangeLog:11
> +        No new tests. Test coverage already extensive.

Since this should be a perf win, it seems appropriate to comment on how much of a win it is (in whatever example benchmarks you like) in the ChangeLog.

> Source/WebCore/bindings/v8/V8StringResource.cpp:42
> +    inline static String fromStringResource(WebCoreStringResourceBase* resource)

Should this return const String&?  it seems the resource always owns the String.

> Source/WebCore/bindings/v8/V8StringResource.cpp:135
>          return String("");

String::empty() is faster. :)

> Source/WebCore/bindings/v8/V8ValueCache.cpp:44
> +    if (string.is8Bit() && string.containsOnlyASCII()) {
> +        WebCoreStringResource8* stringResource = new WebCoreStringResource8(string);
> +        v8::Local<v8::String> newString = v8::String::NewExternal(stringResource);
> +        if (newString.IsEmpty())
> +            delete stringResource;
> +        return newString;
> +    }
> +
> +    WebCoreStringResource16* stringResource = new WebCoreStringResource16(string);

Can't we share this block by using a WebCoreStringResourceBase* ?  Or does it not have a virtual destructor?

> Source/WebCore/bindings/v8/V8ValueCache.h:87
> +    inline explicit WebCoreStringResourceBase(const AtomicString& string)

I was under the impression that marking things "inline" in headers was meanlingless?  I thought 'inline' was implied for anything which was listed as part of the class definition?

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