Bug 153688 - Modern IDB: Some tests crash with specific odd database names
Summary: Modern IDB: Some tests crash with specific odd database names
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: Safari 9
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks: 149117 153021
  Show dependency treegraph
 
Reported: 2016-01-29 16:43 PST by Brady Eidson
Modified: 2016-02-02 16:05 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (3.98 KB, patch)
2016-01-29 16:45 PST, Brady Eidson
darin: review+
Details | Formatted Diff | Diff
Patch for landing (3.97 KB, patch)
2016-01-30 13:48 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-01-29 16:43:15 PST
Modern IDB: Some tests crash with specific odd database names
Comment 1 Brady Eidson 2016-01-29 16:45:06 PST
Created attachment 270271 [details]
Patch v1
Comment 2 Darin Adler 2016-01-29 18:18:12 PST
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.
Comment 3 Brady Eidson 2016-01-30 13:48:39 PST
Created attachment 270318 [details]
Patch for landing
Comment 4 WebKit Commit Bot 2016-01-30 14:25:29 PST
Comment on attachment 270318 [details]
Patch for landing

Clearing flags on attachment: 270318

Committed r195913: <http://trac.webkit.org/changeset/195913>