Bug 128110 – Web Replay: upstream base input classes and the input cursor interface

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Blaze Burg]

The first patch of many..

This one includes the base classes for captured nondeterministic inputs, and the machinery to set the input cursor (which mediates storing and loading inputs) on a JSGlobalObject or Document.

[Blaze Burg]

Created attachment 223013 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/replay/NondeterministicInput.h:69:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/replay/NondeterministicInput.h:74:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/replay/EmptyInputCursor.h:52:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/replay/EmptyInputCursor.h:58:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/replay/EmptyInputCursor.h:64:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 5 in 15 files


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

[Blaze Burg]

Filed bug against style checker: https://bugs.webkit.org/show_bug.cgi?id=128119

[Joseph Pecoraro]

Comment on attachment 223013 [details]
patch

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

r=me, I only have style comments and nits. I may have more questions once these get used and when ReplayController is added.

> Source/JavaScriptCore/replay/EmptyInputCursor.h:32
> +#include <replay/InputCursor.h>

Nit: You should be able to just do #include "InputCursor.h" and all ports should build fine.

> Source/JavaScriptCore/replay/EmptyInputCursor.h:38
> +class EmptyInputCursor : public InputCursor {

Nit: Easier and cleaner to put "final" here in the class definition then on every single method.

    class EmptyInputCursor final : public InputCursor

> Source/JavaScriptCore/replay/EmptyInputCursor.h:45
> +        return adoptRef(new EmptyInputCursor());

Style: Parens not needed. "new EmptyInputCursor"

> Source/JavaScriptCore/replay/InputCursor.h:48
> +    virtual bool isCapturing() const =0;
> +    virtual bool isReplaying() const =0;

Style: "= 0;", here and elsewhere.

> Source/JavaScriptCore/replay/InputCursor.h:53
> +        return storeInput(std::unique_ptr<NondeterministicInputBase>(WTF::safeCast<InputType*>(new InputType(std::forward<Args>(args)...))));

Maybe break this up into 2 lines so it is easier to read?

    InputType* inputType = WTF::safeCast<InputType*>(blah);
    return storeInput(std::unique_ptr<NondeterministicInputBase>(inputType));

> Source/JavaScriptCore/replay/NondeterministicInput.h:67
> +class NondeterministicInput : public NondeterministicInputBase {

Nit: final in class

> Source/WebCore/WebCore.xcodeproj/project.pbxproj:17859
> +			name = replay;

Nit: This should be "path = replay". That way, if you want to add files to this group in Xcode, Xcode automatically opens up the Source/WebCore/replay folder instead of the project base Source/WebCore folder.

You can change this in Xcode by selecting the Group, showing the right sidebar, and setting the Relative to Group path. You'll need to re-add the files inside the group. Better to do this now while there is 1 file then when there are 10+!

> Source/WebCore/replay/EventLoopInput.h:38
> +namespace JSC {
> +class EncodedValue;
> +}

Nit: This forward declaration is unneeded, as it is guaranteed by the base header.

> Source/WebCore/replay/EventLoopInput.h:46
> +    explicit ReplayPosition()

Nit: explicit not needed here.

> Source/WebCore/replay/EventLoopInput.h:61
> +class EventLoopInputBase : public NondeterministicInputBase {

Nit: final in class.

> Source/WebCore/replay/EventLoopInput.h:80
> +    virtual const AtomicString& type() const override final {

Style: brace on its own line.

[Blaze Burg]

Comment on attachment 223013 [details]
patch

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

>> Source/JavaScriptCore/replay/EmptyInputCursor.h:32
>> +#include <replay/InputCursor.h>
> 
> Nit: You should be able to just do #include "InputCursor.h" and all ports should build fine.

Oops, nice catch. This class grew up in WebCore.

>> Source/JavaScriptCore/replay/EmptyInputCursor.h:45
>> +        return adoptRef(new EmptyInputCursor());
> 
> Style: Parens not needed. "new EmptyInputCursor"

Filed against check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=128134

>> Source/JavaScriptCore/replay/InputCursor.h:48
>> +    virtual bool isReplaying() const =0;
> 
> Style: "= 0;", here and elsewhere.

Filed against check-webkit-style: https://bugs.webkit.org/show_bug.cgi?id=128134

>> Source/JavaScriptCore/replay/InputCursor.h:53
>> +        return storeInput(std::unique_ptr<NondeterministicInputBase>(WTF::safeCast<InputType*>(new InputType(std::forward<Args>(args)...))));
> 
> Maybe break this up into 2 lines so it is easier to read?
> 
>     InputType* inputType = WTF::safeCast<InputType*>(blah);
>     return storeInput(std::unique_ptr<NondeterministicInputBase>(inputType));

OK

>> Source/WebCore/WebCore.xcodeproj/project.pbxproj:17859
>> +			name = replay;
> 
> Nit: This should be "path = replay". That way, if you want to add files to this group in Xcode, Xcode automatically opens up the Source/WebCore/replay folder instead of the project base Source/WebCore folder.
> 
> You can change this in Xcode by selecting the Group, showing the right sidebar, and setting the Relative to Group path. You'll need to re-add the files inside the group. Better to do this now while there is 1 file then when there are 10+!

I could never figure out how to make it do this. Thanks! I wrote up some of the steps details in the wiki, since I seem to forget frequently.

https://trac.webkit.org/wiki/AddingFiles

>> Source/WebCore/replay/EventLoopInput.h:61
>> +class EventLoopInputBase : public NondeterministicInputBase {
> 
> Nit: final in class.

dispatch() is overridden by every subclass of EventLoopInput.

[Blaze Burg]

http://trac.webkit.org/changeset/163346