[webkit-reviews] review denied: [Bug 205902] KeyedDecoderGeneric craches when it acesses data with non-existing key. : [Attachment 387078] Patch

[bugzilla-daemon at webkit.org]

Fujii Hironori <Hironori.Fujii at sony.com> has denied Takashi Komori
<Takashi.Komori at sony.com>'s request for review:
Bug 205902: KeyedDecoderGeneric craches when it acesses data with non-existing
key.
https://bugs.webkit.org/show_bug.cgi?id=205902

Attachment 387078: Patch

https://bugs.webkit.org/attachment.cgi?id=387078&action=review




--- Comment #3 from Fujii Hironori <Hironori.Fujii at sony.com> ---
Comment on attachment 387078
  --> https://bugs.webkit.org/attachment.cgi?id=387078
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387078&action=review

> Source/WebCore/ChangeLog:3
> +	   KeyedDecoderGeneric craches when it acesses data with non-existing
key.

acesses → accesses

> Source/WebCore/platform/generic/KeyedDecoderGeneric.cpp:46
> +    bool exists(const String& key) { return !!m_map.get(key); }

It's inefficient to call 'm_map.get(key)' twice.
Make 'get' method return Node*.

Node* get(const String& key) { return m_map.get(key); }

> Source/WebCore/platform/generic/KeyedDecoderGeneric.cpp:193
> +    return WTF::get_if<T>(dictionary->get(key));

auto node = dictionary->get(key)
if (!node)
  return nullptr;
return WTF::get_if<T>(node);

> Source/WebCore/platform/generic/KeyedDecoderGeneric.cpp:197
> +bool KeyedDecoderGeneric::getValueFromDictionaryStack(const String& key, T&
result)

I think decodeSimpleValue is a better name. KeyedDecoderGlib is using the name.

> Tools/TestWebKitAPI/PlatformWin.cmake:65
> +	   Tests/WebCore/KeyedCodingGeneric.cpp

I think this tests should be run on all ports.
Rename KeyedCodingGeneric.cpp KeyedCoding.cpp.
Add KeyedCoding.cpp for all ports.

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:42
> +	       return false;

Let's use std::memcmp.

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:48
> +TEST(KeyedCodingGeneric, SetAndGetBytes)

KeyedCodingGeneric → KeyedCoding

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:51
> +    const size_t dataLength = sizeof(data) / sizeof(uint8_t);

Use WTF_ARRAY_LENGTH.

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:67
> +    result = decoder->decodeBytes("data", buffer, size);

Rename 'result' to 'ok' or 'success'.

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:133
> +    EXPECT_EQ(INT32_MAX, value);

I think you should add one more test case checking unavailable to decode int32
as other type, for example, bool.
    ok = decoder->decodeBool("zero", value);
    EXPECT_FALSE(ok);

> Tools/TestWebKitAPI/Tests/WebCore/KeyedCodingGeneric.cpp:263
> +    encoder->encodeDouble("double-max", DBL_MAX);

I think std::numeric_limits<double>::min() and
std::numeric_limits<double>::max() are better in C++ code.