WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
153688
Modern IDB: Some tests crash with specific odd database names
https://bugs.webkit.org/show_bug.cgi?id=153688
Summary
Modern IDB: Some tests crash with specific odd database names
Brady Eidson
Reported
2016-01-29 16:43:15 PST
Modern IDB: Some tests crash with specific odd database names
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
View All
Add attachment
proposed patch, testcase, etc.
Brady Eidson
Comment 1
2016-01-29 16:45:06 PST
Created
attachment 270271
[details]
Patch v1
Darin Adler
Comment 2
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.
Brady Eidson
Comment 3
2016-01-30 13:48:39 PST
Created
attachment 270318
[details]
Patch for landing
WebKit Commit Bot
Comment 4
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
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug