[webkit-reviews] review denied: [Bug 59114] Move makeSecure from WTF::String to renderText : [Attachment 90740] fix patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied Chang Shu <cshu at webkit.org>'s
request for review:
Bug 59114: Move makeSecure from WTF::String to renderText
https://bugs.webkit.org/show_bug.cgi?id=59114

Attachment 90740: fix patch
https://bugs.webkit.org/attachment.cgi?id=90740&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=90740&action=review

A good cleanup, but the isNull() check in makeSecure is wrong.

> Source/JavaScriptCore/JavaScriptCore.order:-1686
> -__ZN7WebCore10StringImpl6secureEt

You don't need to change order files. It's no problem if you do, but it's a
waste of time.

> Source/WebCore/rendering/RenderText.cpp:95
> +void makeSecure(String* string, UChar mask, LastCharacterBehavior behavior)

I suggest making the string argument a reference instead of a pointer, to make
it obvious that it can't be null.

> Source/WebCore/rendering/RenderText.cpp:98
> +    if (string->isNull())
> +	   return;

You iterate up to length - 1 below, which will be no good if the string is
empty. Why didn't you get crashes in testing?

This should check for isEmpty() - or if there is some reason why empty strings
can't get here, we should assert that, and/or restructure the code to make it
more clear.

> Source/WebCore/rendering/RenderText.h:211
> +void makeSecure(String*, UChar mask, LastCharacterBehavior =
ObscureLastCharacter);

I don't know if this is the best place for this function - RenderText.h doesn't
currently have utility functions like this. Seems acceptable, but you could ask
a rendering expert.