Bug 94571 – [V8] Move String related code in V8Binding to a separate file

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Kentaro Hara]

We can move V8Parameter, V8ParameterBase and String related code in V8Binding to a separate file.

[Kentaro Hara]

Created attachment 159618 [details]
Patch

[Kentaro Hara]

I'll do the following things in follow-up patches:

- Remove V8ParameterBase. It's redundant. Only V8Parameter will work.
- Rename V8Parameter to V8GenericString, because it's an adapter class for String and AtomicString.

[Adam Barth]

Comment on attachment 159618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159618&action=review

> Source/WebCore/bindings/v8/V8StringResource.cpp:35
> +

I'd remove this blank line.

> Source/WebCore/bindings/v8/V8StringResource.cpp:52
> +        String result = String::createUninitialized(length, buffer);

Does this really use int as the type for length?

> Source/WebCore/bindings/v8/V8StringResource.cpp:75
> +        String tmp = String::createUninitialized(length, buffer);

tmp -> string ?

> Source/WebCore/bindings/v8/V8StringResource.cpp:88
> +    int length = v8String->Length();

Oh, V8 uses int for the length of a string?

> Source/WebCore/bindings/v8/V8StringResource.cpp:91
> +        return StringImpl::empty();

Is this ok for background threads?  Typically we can't use these sorts of functions if we're not on the main thread.

[Kentaro Hara]

Created attachment 159640 [details]
patch for landing

[Kentaro Hara]

Committed r126150: <http://trac.webkit.org/changeset/126150>

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 159618 [details])
> > Source/WebCore/bindings/v8/V8StringResource.cpp:35
> > +
> 
> I'd remove this blank line.
> 
> > Source/WebCore/bindings/v8/V8StringResource.cpp:75
> > +        String tmp = String::createUninitialized(length, buffer);
> 
> tmp -> string ?
> 
> > Source/WebCore/bindings/v8/V8StringResource.cpp:91
> > +        return StringImpl::empty();
> 
> Is this ok for background threads?  Typically we can't use these sorts of functions if we're not on the main thread.

Done. Replaced with String().

[WebKit Review Bot]

Re-opened since this is blocked by 94605

[Kentaro Hara]

Created attachment 159852 [details]
patch for landing

[Kentaro Hara]

(In reply to comment #6)
> > > Source/WebCore/bindings/v8/V8StringResource.cpp:91
> > > +        return StringImpl::empty();
> > 
> > Is this ok for background threads?  Typically we can't use these sorts of functions if we're not on the main thread.
> 
> Replaced with String().

This was the cause of the crashes.

I replaced it with String(""). Given that this is not a fast path of v8String(), it would be OK to create String("") every time.

[WebKit Review Bot]

Comment on attachment 159852 [details]
patch for landing

Clearing flags on attachment: 159852

Committed r126262: <http://trac.webkit.org/changeset/126262>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.