[webkit-reviews] review granted: [Bug 128110] Web Replay: upstream base input classes and the input cursor interface : [Attachment 223013] patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 128110: Web Replay: upstream base input classes and the input cursor
interface
https://bugs.webkit.org/show_bug.cgi?id=128110

Attachment 223013: patch
https://bugs.webkit.org/attachment.cgi?id=223013&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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.