Bug 48316 – Web Inspector: rewrite InspectorInstrumenation using macro

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Web Inspector (Deprecated)

[Pavel Podivilov]

Web Inspector: rewrite InspectorInstrumenation using macro

[Pavel Podivilov]

Created attachment 71855 [details]
Patch.

[Pavel Podivilov]

Created attachment 71856 [details]
Patch.

[Early Warning System Bot]

Attachment 71856 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4755012

[Eric Seidel (no email)]

Attachment 71856 [details] did not build on mac:
Build output: http://queues.webkit.org/results/4787009

[Eric Seidel (no email)]

Attachment 71856 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/4816018

[Pavel Podivilov]

Created attachment 71872 [details]
Build fix.

[Alexander Pavlov (apavlov)]

Comment on attachment 71872 [details]
Build fix.

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

> WebCore/ChangeLog:5
> +        Web Inspector: rewrite InspectorInstrumenation using macro

Bad "InspectorInstrumenation" spelling (from the bug summary)

> WebCore/inspector/InspectorInstrumentation.cpp:462
> +    if (page) {

Converted into a series of guards (if (...) return 0;), it would read better

> WebCore/inspector/InspectorInstrumentation.cpp:471
> +InspectorController* InspectorInstrumentation::retrieveInspectorController(ScriptExecutionContext* context)

we usually name getters without a leading verb (e.g. InspectorInstrumentation::inspectorController(..)). Ditto for the overloads.

[Joseph Pecoraro]

I don't find this easier to read at all. There is too much magic going on, I don't think
all of it is necessary. I would say its easier to read generated code than this. I don't
think this makes it easier to understand and write inspector code for newcomers.

[Joseph Pecoraro]

Thats a lot of "I don'ts". I am just worried about this change making it more difficult
for people learn and work on the inspector.

[Pavel Feldman]

(In reply to comment #9)
> Thats a lot of "I don'ts". I am just worried about this change making it more difficult
> for people learn and work on the inspector.

I can see your concerns. We'd like to have a nice facade for the instrumentation calls in WebCore and this class serves the purpose. But we've learned that it is error-prone since api methods call corresponding impl methods and those delegate further. So one can easily confuse names and such. This change kills code and that's why I tend to like it. It makes things no so clear, but it definitely allows adding instrumentation with less pain. Like inspector.idl was a great thing to have instead of the InspectorFrontend class.

What are your suggestions wrt killing code and making it less error-prone?

[Joseph Pecoraro]

> It makes things not so clear, but it definitely allows adding instrumentation with less pain.
> Like inspector.idl was a great thing to have instead of the InspectorFrontend class.

I think Inspector.idl was a nice improvement. It is much easier to follow how the idl function
generates the .cpp, .h, and .js files. And, if there is any confusion you can see the actual
generated file to see what it output. Here, there is no generated file. Maybe you can run the
pre-processor to see what it produces, but it still feels more difficult. This looks like it
is trying to provide a new special syntax whereas the idl approach has a cleaner,
more standard syntax.

It could just be that the macros are so spread out in the patch that I have to jump around a
lot to follow what is happening. Would you mind putting up a patch of what
InspectorInstrumentation.h looks like after your patch?


> What are your suggestions wrt killing code and making it less error-prone?

I'm a big fan of DRY (Don't Repeat Yourself) removing duplicated code.

[Pavel Podivilov]

Created attachment 72047 [details]
Patch.

[Pavel Podivilov]

Created attachment 72048 [details]
InspectorInstrumentation.h

[Pavel Podivilov]

(In reply to comment #7)
> (From update of attachment 71872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71872&action=review
> 
> > WebCore/ChangeLog:5
> > +        Web Inspector: rewrite InspectorInstrumenation using macro
> 
> Bad "InspectorInstrumenation" spelling (from the bug summary)
done

> 
> > WebCore/inspector/InspectorInstrumentation.cpp:462
> > +    if (page) {
> 
> Converted into a series of guards (if (...) return 0;), it would read better
done

> 
> > WebCore/inspector/InspectorInstrumentation.cpp:471
> > +InspectorController* InspectorInstrumentation::retrieveInspectorController(ScriptExecutionContext* context)
> 
> we usually name getters without a leading verb (e.g. InspectorInstrumentation::inspectorController(..)). Ditto for the overloads.
Actually it's not a getter, since InspectorInstrumentation doesn't store pointers to InspectorController. I think retrieveInspectorController name better reflects what method does.

[Maciej Stachowiak]

Comment on attachment 72047 [details]
Patch.

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.