Bug 86307 – Simplify SourceProvider's subclasses

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Misc.

[Benjamin Poulain]

Simplify SourceProvider's subclasses

[Benjamin Poulain]

Created attachment 141591 [details]
Patch

[Darin Adler]

Comment on attachment 141591 [details]
Patch

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

This is OK as-is, but we might be able to do a bit better.

> Source/JavaScriptCore/parser/SourceProvider.h:61
> +        const StringImpl* data() const
> +        {
> +            return stringImpl();
> +        }

Why not leave this as a pure virtual function and call it instead of adding a new stringImpl() function? Or maybe this could all be done with the source() function? I am unclear on why there need to be three different functions that all return the source string.

Also, all the callers seem to be calling impl() so maybe this should return a const UString& or const String&. That would be one way of saving a bit of reference count churn in the getRange function. I guess the source() function already works that way.

> Source/JavaScriptCore/parser/SourceProvider.h:65
> +            int length = end - start;

Why a local variable for this? Doesn’t seem better than just saying end - start in the function call.

> Source/JavaScriptCore/parser/SourceProvider.h:66
> +            return UString(stringImpl()).substringSharingImpl(start, length);

Would be nice to add a version of substringSharingImpl that works directly on a StringImpl* so we don’t have to churn the reference count an extra time each time this function is called.

> Source/JavaScriptCore/parser/SourceProvider.h:72
> +            StringImpl* impl = stringImpl();
> +            return impl ? impl->length() : 0;

There must be a better name than “impl” for this. Maybe “data” or “string”. Words are better than word fragments.

[Benjamin Poulain]

The method source() is actually WebCore specific. Generalizing it to JSC is a good idea, I will look into that.

[Benjamin Poulain]

With the UString change, this problem has radically changed.

I close this bug as it is now  superseded by https://bugs.webkit.org/show_bug.cgi?id=95635