[webkit-reviews] review granted: [Bug 128633] Web Replay: capture and replay nondeterminism of Date.now() and Math.random() : [Attachment 223920] patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 128633: Web Replay: capture and replay nondeterminism of Date.now() and
Math.random()
https://bugs.webkit.org/show_bug.cgi?id=128633

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

------- Additional Comments from Filip Pizlo <fpizlo at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=223920&action=review


R=me but please consider encapsulating the #if ENABLE(WEB_REPLAY) inside a
clever macro.

> Source/JavaScriptCore/runtime/DateConstructor.cpp:128
> +#if ENABLE(WEB_REPLAY)
> +	   value = deterministicCurrentTime(globalObject);
> +#else
>	   value = jsCurrentTime();
> +#endif

Can't you have a macro like REPLAY(jsCurrentTime(),
deterministicCurrentTime(globalObject))?

The reason why I would like that more is that #if's can often disturb our
ability to visualize the nesting of control flow.  The definition of REPLAY
would literally be:

#if ENABLE(WEB_REPLAY)
#define REPLAY(a, b) (b)
#else
#define REPLAY(a, b) (a)
#endif

You can call REPLAY() whatever you like...

> Source/JavaScriptCore/runtime/DateConstructor.cpp:214
> +#if ENABLE(WEB_REPLAY)
> +    return
JSValue::encode(jsNumber(deterministicCurrentTime(exec->lexicalGlobalObject()))
);
> +#else
> +    UNUSED_PARAM(exec);
>      return JSValue::encode(jsNumber(jsCurrentTime()));
> +#endif

Another fine example of a place where that REPLAY() macro would simplify
things.