[webkit-reviews] review denied: [Bug 48316] Web Inspector: rewrite InspectorInstrumenation using macro : [Attachment 72047] Patch.

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Pavel Podivilov
<podivilov at chromium.org>'s request for review:
Bug 48316: Web Inspector: rewrite InspectorInstrumenation using macro
https://bugs.webkit.org/show_bug.cgi?id=48316

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

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72047&action=review

> WebCore/inspector/InspectorInstrumentation.h:106
> +#define ARGS0()
> +#define ARGS1(Arg1) , Arg1 arg1
> +#define ARGS2(Arg1, Arg2) ARGS1(Arg1), Arg2 arg2
> +#define ARGS3(Arg1, Arg2, Arg3) ARGS2(Arg1, Arg2), Arg3 arg3
> +#define ARGS4(Arg1, Arg2, Arg3, Arg4) ARGS3(Arg1, Arg2, Arg3), Arg4 arg4
> +
> +#define ARGNAMES0
> +#define ARGNAMES1 , arg1
> +#define ARGNAMES2 , arg1, arg2
> +#define ARGNAMES3 , arg1, arg2, arg3
> +#define ARGNAMES4 , arg1, arg2, arg3, arg4
> +
> +#define EXPAND(macro, name, Origin, argc, args) macro(name, Origin,
ARGS##argc args, ARGNAMES##argc)
> +#define EXPAND1(macro, name, Origin, argc, args) macro(name, Origin,
ARGS##argc args, ARGNAMES##argc, , )
> +#define EXPAND2(macro, name, Origin, argc1, args1, argc2, args2) macro(name,
Origin, ARGS##argc1 args1, ARGNAMES##argc1, ARGS##argc2 args2, ARGNAMES##argc2)

> +
> +#define INSPECTOR_INSTRUMENTATION_METHODS(macro, pairedMacro) \
> +    EXPAND(macro, willInsertDOMNode, Document*, 2, (Node*, Node*)) \
> +    EXPAND(macro, didInsertDOMNode, Document*, 1, (Node*)) \
> +    EXPAND(macro, willRemoveDOMNode, Document*, 1, (Node*)) \
> +    EXPAND(macro, didRemoveDOMNode, Document*, 1, (Node*)) \
> +    EXPAND(macro, willModifyDOMAttr, Document*, 1, (Element*)) \
> +    EXPAND(macro, didModifyDOMAttr, Document*, 1, (Element*)) \
> +    EXPAND(macro, characterDataModified, Document*, 1, (CharacterData*)) \
> +    EXPAND(macro, willSendXMLHttpRequest, ScriptExecutionContext*, 1, (const
String&)) \
> +    EXPAND(macro, didScheduleResourceRequest, Document*, 1, (const String&))
\
> +    EXPAND(macro, didInstallTimer, ScriptExecutionContext*, 3, (int, int,
bool)) \
> +    EXPAND(macro, didRemoveTimer, ScriptExecutionContext*, 1, (int)) \
> +    EXPAND(macro, didCreateWebSocket, ScriptExecutionContext*, 3, (unsigned
long, const KURL&, const KURL&)) \
> +    EXPAND(macro, willSendWebSocketHandshakeRequest,
ScriptExecutionContext*, 2, (unsigned long, const WebSocketHandshakeRequest&))
\
> +    EXPAND(macro, didReceiveWebSocketHandshakeResponse,
ScriptExecutionContext*, 2, (unsigned long, const WebSocketHandshakeResponse&))
\
> +    EXPAND(macro, didCloseWebSocket, ScriptExecutionContext*, 1, (unsigned
long)) \
> +    EXPAND1(pairedMacro, CallFunction, Frame*, 2, (const String&, int)) \
> +    EXPAND1(pairedMacro, ChangeXHRReadyState, ScriptExecutionContext*, 1,
(XMLHttpRequest*)) \
> +    EXPAND1(pairedMacro, DispatchEvent, Document*, 4, (const Event&,
DOMWindow*, Node*, const Vector<RefPtr<ContainerNode> >&)) \
> +    EXPAND1(pairedMacro, DispatchEventOnWindow, Frame*, 2, (const Event&,
DOMWindow*)) \
> +    EXPAND1(pairedMacro, EvaluateScript, Frame*, 2, (const String&, int)) \
> +    EXPAND1(pairedMacro, FireTimer, ScriptExecutionContext*, 1, (int)) \
> +    EXPAND1(pairedMacro, Layout, Frame*, 0, ()) \
> +    EXPAND1(pairedMacro, LoadXHR, ScriptExecutionContext*, 1,
(XMLHttpRequest*)) \
> +    EXPAND1(pairedMacro, Paint, Frame*, 1, (const IntRect&)) \
> +    EXPAND1(pairedMacro, RecalculateStyle, Document*, 0, ()) \
> +    EXPAND1(pairedMacro, ReceiveResourceData, Frame*, 1, (unsigned long)) \
> +    EXPAND1(pairedMacro, ReceiveResourceResponse, Frame*, 2, (unsigned long,
const ResourceResponse&)) \
> +    EXPAND2(pairedMacro, WriteHTML, Document*, 2, (unsigned int, unsigned
int), 1, (unsigned int)) \
> +// end of INSPECTOR_INSTRUMENTATION_METHODS

This seems like a bad change. It makes it harder to see what inspector methods
exist and what the method signatures are, and it's very hard to follow in
general. It also makes for more difficult debugging. For nontrivial code
generation we prefer to write code-generator scripts to excess use of macros.

r- because this is a refactoring change but seems to make readability worse
instead of better.