[webkit-reviews] review granted: [Bug 101956] We should trigger a console warning when we encounter invalid sandbox flags. : [Attachment 173713] Take 2.

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Mike West <mkwst at chromium.org>'s
request for review:
Bug 101956: We should trigger a console warning when we encounter invalid
sandbox flags.
https://bugs.webkit.org/show_bug.cgi?id=101956

Attachment 173713: Take 2.
https://bugs.webkit.org/attachment.cgi?id=173713&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173713&action=review


> Source/WebCore/dom/SecurityContext.cpp:125
> +		   tokenErrors.appendLiteral("'");

I gave you bad advice.	You can actually just make this
tokenErrors.append('\''), which is slightly more efficient.

> Source/WebCore/dom/SecurityContext.cpp:127
> +	       tokenErrors.appendLiteral("'");

Ditto

> Source/WebCore/dom/SecurityContext.cpp:135
> +	   tokenErrors.append((numberOfTokenErrors > 1) ? " are invalid sandbox
flags." : " is an invalid sandbox flag.");

This would be more efficient if you had a real if statement and called
appendLiteral separately for the two different constants.

What's going on here is that appendLiteral uses a compiler trick to figure out
the length of the string at compile-time, which means we don't need to call
strlen on these constants.  The way you've written this code, we can't figure
out the length at compile time, so we fall back to strlen.

Above, it's better to use character constants rather than strings because then
we don't need loops or other nonsense to traverse a one-character string.

None of this really matters in this patch.  I just mention it so that you know
for future patches where it might matter more.