[webkit-reviews] review granted: [Bug 86307] Simplify SourceProvider's subclasses : [Attachment 141591] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 86307: Simplify SourceProvider's subclasses
https://bugs.webkit.org/show_bug.cgi?id=86307

Attachment 141591: Patch
https://bugs.webkit.org/attachment.cgi?id=141591&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.