[webkit-reviews] review granted: [Bug 129395] Web Replay: capture and replay mouse events : [Attachment 226862] the patch (no tests yet)

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 129395: Web Replay: capture and replay mouse events
https://bugs.webkit.org/show_bug.cgi?id=129395

Attachment 226862: the patch (no tests yet)
https://bugs.webkit.org/attachment.cgi?id=226862&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=226862&action=review


> Source/WebCore/replay/SerializationMethods.cpp:54
> +    _encodedValue.put<_type>(ASCIILiteral("_key"), _value)

I don't think this does what you think it does. This always uses the key
"_key".

test:

    #include <stdio.h>
    #define MACRO(_key) "_key"
    int main() {
	printf(">>> macro: %s\n", MACRO(test));
	return 0;
    }

outputs:

    >>> macro: _key

I think you should avoid the quotes in the macro entirely, and instead put them
at the macro call site. I think that would be easier to read, and see that,
"positionX" is a string key and not a variable.

> Source/WebCore/replay/SerializationMethods.cpp:59
> +#define DECODE_SCALAR_TYPE_WITH_KEY(_encodedValue, _type, _key) \
> +    _type _key; \
> +    if (!_encodedValue.get<_type>(ASCIILiteral("_key"), _key)) \
> +	   return false

Likewise. Yes, there will be some duplication (key and variable name), but I
think that is fine. If you really want to avoid the duplication then I think
you can do:

    ...(ASCIILiteral("" #_key), _key)

> Source/WebCore/replay/UserInputBridge.cpp:52
> +    do { \
> +    if (inputSource == InputSource::User && m_state ==
UserInputBridge::State::Replaying) \
> +	   return true; \
> +    } while (false)

Style: Weird indentation. And would this really be an unexpected input, or just
an ignored input while replaying?