Bug 153688

Created attachment 270271 [details] Patch v1

Comment on attachment 270271 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=270271&action=review > Source/WebCore/platform/FileSystem.cpp:89 > +enum class UCharEscapeMode { > + NoEscape, > + Escape1Byte, > + Escape2Bytes, > +}; Could just have kept the shouldEscape function returning a boolean. It’s really easy (and logical) for the caller to check if the character fits in a byte after the fact and decide which kind of escaping to do. > Source/WebCore/platform/FileSystem.cpp:91 > +static inline UCharEscapeMode shouldEscapeUChar(UChar c, UChar prev, UChar next) I would have named these character, previousCharacter, and nextCharacter. > Source/WebCore/platform/FileSystem.cpp:96 > + if (U16_IS_LEAD(c) && (!next || !U16_IS_TRAIL(next))) No need for the !next check here. 0 is not a trailing surrogate. > Source/WebCore/platform/FileSystem.cpp:99 > + if (U16_IS_TRAIL(c) && (!prev || !U16_IS_LEAD(prev))) No need for the !prev check here. 0 is not a leading surrogate. > Source/WebCore/platform/FileSystem.cpp:109 > unsigned length = inputString.length(); Normally we would reserve capacity here for the string builder to slightly improve memory use. > Source/WebCore/platform/FileSystem.cpp:111 > UChar character = (*stringImpl)[i]; Kind of strange to use stringImpl instead of inputString here. Seems to be a preexisting unnecessary attempt at optimization. > Source/WebCore/platform/FileSystem.cpp:112 > + UChar prev = !i ? 0 : (*stringImpl)[i - 1]; I would have written: UChar previousCharacter = i ? inputString[i] : 0; And if I really cared so much about optimization I would have tried to keep the previous character from the last time through the loop instead of re-fetching it. > Source/WebCore/platform/FileSystem.cpp:113 > + UChar next = i + 1 == length ? 0 : (*stringImpl)[i + 1]; I would have written: UChar nextCharacter = i + 1 < length ? inputString[i + 1] : 0; > Source/WebCore/platform/FileSystem.cpp:115 > + auto escapeMode = shouldEscapeUChar(character, prev, next); Seems like this should be a switch rather than cascading if statements. If we keep the enum type at all. > Source/WebCore/platform/FileSystem.cpp:120 > + result.append("%+"); Should use appendLiteral.

Created attachment 270318 [details] Patch for landing

Comment on attachment 270318 [details] Patch for landing Clearing flags on attachment: 270318 Committed r195913: <http://trac.webkit.org/changeset/195913>