[webkit-reviews] review granted: [Bug 72404] Could save a lot of memory in CharacterData by not always storing a String : [Attachment 117712] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Erik Arvidsson <arv at chromium.org>'s
request for review:
Bug 72404: Could save a lot of memory in CharacterData by not always storing a
String
https://bugs.webkit.org/show_bug.cgi?id=72404

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=117712&action=review


> Source/WebCore/html/parser/HTMLConstructionSite.cpp:350
> +    if (whitespaceInfo == WhitespaceUnknown)
> +	   whitespaceInfo = isAllWhitespace(characters) ? AllWhitespace :
NotAllWhitespace;

I think you should write this:

    // Strings composed entirely of whitespace are likely to be repeated.
    // Turn them into AtomicString so we share a single string for each.
    bool shouldUseAtomicString = whitespaceInfo == AllWhitespace
	|| (whitespaceInfo == WhitespaceUnknown &&
isAllWhitespace(characters));

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:366
> -	   RefPtr<Text> textNode =
Text::createWithLengthLimit(site.parent->document(), characters,
currentPosition);
> +	   RefPtr<Text> textNode =
Text::createWithLengthLimit(site.parent->document(), whitespaceInfo ==
AllWhitespace ? AtomicString(characters).string() : characters,
currentPosition);

This code needs a comment explaining the policy. Or better, it could just use a
boolean named shouldUseAtomicString and the comment could be above.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:369
> -	       textNode = Text::create(site.parent->document(),
characters.substring(currentPosition));
> +	       textNode = Text::create(site.parent->document(), whitespaceInfo
== AllWhitespace ? AtomicString(characters.substring(currentPosition)).string()
: characters.substring(currentPosition));

It would be nice to put the substring into a local String so we don’t have to
repeat that subexpression.

> Source/WebCore/html/parser/HTMLConstructionSite.h:55
> +    enum WhitespaceInfo {
> +	   AllWhitespace,
> +	   NotAllWhitespace,
> +	   WhitespaceUnknown
> +    };

Since this is software, everything is “info”, so that seems like a non-helpful
suffix, and abbreviated to boot! I didn’t think of a better name, though, and I
did try for a few minutes.

I think it would be fine to have this at the namespace level instead of using a
class member. Would make the call sites more readable.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:276
> -    String takeLeadingWhitespace()
> +    AtomicString takeLeadingWhitespace()

Why is it helpful for this to return an AtomicString? It seems like extra
unnecessary work and extra unnecessary change.

> Source/WebCore/html/parser/HTMLTreeBuilder.cpp:300
> -    String takeRemainingWhitespace()
> +    AtomicString takeRemainingWhitespace()

Why is it helpful for this to return an AtomicString?