Bug 129395 – Web Replay: capture and replay mouse events

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Blaze Burg]

Patch forthcoming

[Blaze Burg]

Splitting this into separate patches for mouse, keyboard, wheel, and scroll. The latter two require support elsewhere in WebCore.

[Blaze Burg]

Created attachment 226862 [details]
the patch (no tests yet)

[WebKit Commit Bot]

Attachment 226862 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/PlatformMouseEvent.h:47:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/replay/SerializationMethods.cpp:57:  _key is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 226862 [details]
the patch (no tests yet)

Attachment 226862 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5953556595081216

New failing tests:
mathml/wbr-in-mroot-crash.html

[Build Bot]

Created attachment 226866 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Blaze Burg]

(In reply to comment #4)
> (From update of attachment 226862 [details])
> Attachment 226862 [details] did not pass mac-wk2-ews (mac-wk2):
> Output: http://webkit-queues.appspot.com/results/5953556595081216
> 
> New failing tests:
> mathml/wbr-in-mroot-crash.html

This is unrelated.

[Blaze Burg]

Regarding the windows build failure, it seems to have trouble finding operator== for MouseButton when it has an explicit storage type. Not sure how to fix.

[Joseph Pecoraro]

Comment on attachment 226862 [details]
the patch (no tests yet)

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?

[Blaze Burg]

Comment on attachment 226862 [details]
the patch (no tests yet)

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

>> Source/WebCore/replay/SerializationMethods.cpp:59
>> +        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)

Just #_key should suffice to quote the token.

>> Source/WebCore/replay/UserInputBridge.cpp:52
>> +    } while (false)
> 
> Style: Weird indentation. And would this really be an unexpected input, or just an ignored input while replaying?

The fact that it's unexpected means we should ignore it.

[Joseph Pecoraro]

(In reply to comment #9)
> >> Source/WebCore/replay/UserInputBridge.cpp:52
> >> +    } while (false)
> > 
> > Style: Weird indentation. And would this really be an unexpected input, or just an ignored input while replaying?
> 
> The fact that it's unexpected means we should ignore it.

But its not unexpected. Its realistically possible, and if it happens we just ignore it. It is not unexpected that the user might click on the page during a replay. I just find the word "unexpected" here to inaccurate.

[Joseph Pecoraro]

to be*

[Blaze Burg]

Created attachment 227320 [details]
revised patch with test.

[Joseph Pecoraro]

Comment on attachment 227320 [details]
revised patch with test.

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

r=me

> ManualTests/inspector/replay-mouse-events.html:44
> +    var block = createBlock(hex_md5(JSON.stringify(obj)));
> +    block.textContent = glyphForType(event.type);
> +    var blocksContainer = document.getElementById("blocks");
> +    blocksContainer.appendChild(block);
> +    
> +    var hashLabel = document.getElementById("hash");
> +    hash.textContent = hex_md5(JSON.stringify(dumpedEvents));

Nit: Probably should be indented.

> ManualTests/inspector/replay-mouse-events.html:80
> +    </style>

Nit: Probably should match indentation of <style>.

[Blaze Burg]

Committed <http://trac.webkit.org/changeset/166006>