[Webkit-unassigned] [Bug 205902] KeyedDecoderGeneric craches when it acesses data with non-existing key.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=205902

Fujii Hironori <Hironori.Fujii at sony.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #387078|review?                     |review-
              Flags|                            |

--- 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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200108/b336010c/attachment.htm>