Bug 124246 – Web Inspector: Extract InspectorFrontendDispatchers from InspectorFrontend

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

FrontendDispatchers are generated from Domains with "events". E.g. for Console.messageAdded.

Currently generated InspectorFrontend.h includes a InspectorFrontend::Foo nested class for each of these domains, which means class InspectorFrontend knows about all of the different possible domains that need to send events. That will not be the case when JavaScriptCore has some domains and WebCore has other domains.

All that is necessary for a domain to generate events is an InspectorFrontendChannel. We should split up InspectorFrontend into individual InspectorFrontendDispatchers.

This will be the first major part of Agent refactoring to make it possible to push some into JSC.

[Radar WebKit Bug Importer]

<rdar://problem/15454285>

[Joseph Pecoraro]

It is not possible to make this a simple patch, just because it touches all of the agents. So rather then do a series of very large patches, I'm going to bundle a few changes together in this one. Namely:

  - Remove "class InspectorFrontend" that currently does nothing but hold other frontends. This also catches some stale code.
  - Generate individual "class InspectorFooFrontendDispatcher" classes only for each domain with events. This catches some unnecessary classes.
  - In each agent, convert "InspectorFrontend::Foo* m_frontend" to "unique_ptr<InspectorFooFrontendDispatcher> m_frontendDispatcher" and update uses
  - Simplify the Base Agent interface from optional setFrontend/clearFrontend/registerInDispatcher to required didCreateFrontendAndBackend/willDestroyFrontendAndBackend
  - Base Agent no longer needs to be templated<> or have an "InspectorBaseAgentInterface" class, so remove those
  - since Base Agent subclass constructor calls have to change, pass ASCIILiteral agent names since the string constants turn into WTFString
  - individual Agent's with events should create their frontend dispatchers in didCreateFrontendAndBackend, and clear in willDestroyFrontendAndBackend

Really, splitting up any of these into individual patches would just make unnecessarily large individual patches.

This also paves the way to simplify splitting up InspectorBackend; didCreateFrontendAndBackend will be a perfect time to register a backend dispatcher for each domain.

[Radar WebKit Bug Importer]

<rdar://problem/15454438>

[Joseph Pecoraro]

Created attachment 216748 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 216749 [details]
[BEFORE] InspectorFrontend.h

[Joseph Pecoraro]

Created attachment 216750 [details]
[AFTER] InspectorFrontend.h

[Joseph Pecoraro]

InspectorFrontend.h is really the only thing that changed significantly in generated code. The cpp barely changed (function names, includes, and a bit less unnecessarily generated ifdefs).

[Timothy Hatcher]

Comment on attachment 216748 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/CodeGeneratorInspector.py:1904
> +                    domainClassName="Inspector%sFrontendDispatcher" % domain_name,
> +                    frontendDomainMethodDeclarations="".join(flatten_list(frontend_method_declaration_lines))))

Should add spaces around the assignment = while you are touching this horrid code. Should do a find and replace globally at some point… or just find a way to rm *.py someday. :P

> Source/WebCore/inspector/InspectorCSSAgent.cpp:695
> -    m_frontend->namedFlowCreated(buildObjectForNamedFlow(&errorString, namedFlow, documentNodeId));
> +    m_frontendDispatcher->namedFlowCreated(buildObjectForNamedFlow(&errorString, namedFlow, documentNodeId));

Oh how much a single word helps.

> Source/WebCore/inspector/InspectorDOMStorageAgent.h:84
> -    InspectorFrontend* m_frontend;
> +    std::unique_ptr<InspectorDOMStorageFrontendDispatcher> m_frontendDispatcher;

And at last I see the light! It's like the fog has lifted.

> Source/WebCore/inspector/InspectorWorkerAgent.cpp:131
> +    disable(nullptr);

Can disable() default errorString to nullptr? This looks funny to me.

[Timothy Hatcher]

InspectorFrontend.h is so clean you can eat off of it now!

[Timothy Hatcher]

One nit about the generated InspectorFrontend.h around #ifdefs:

};

#endif // ENABLE(JAVASCRIPT_DEBUGGER)
class InspectorRuntimeFrontendDispatcher {

The blank line and the #endif should swap. This happens around all the #endifs.

[Joseph Pecoraro]

(In reply to comment #8)
> (From update of attachment 216748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=216748&action=review
> 
> > Source/WebCore/inspector/CodeGeneratorInspector.py:1904
> > +                    domainClassName="Inspector%sFrontendDispatcher" % domain_name,
> > +                    frontendDomainMethodDeclarations="".join(flatten_list(frontend_method_declaration_lines))))
> 
> Should add spaces around the assignment = while you are touching this horrid code. Should do a find and replace globally at some point… or just find a way to rm *.py someday. :P

Yeah, I'm not very comfortable with Python. It is okay, but I stuck with consistent styling in this patch. Eventually I'd like to look into rewriting the generator altogether. It isn't hard at all.


> > Source/WebCore/inspector/InspectorWorkerAgent.cpp:131
> > +    disable(nullptr);
> 
> Can disable() default errorString to nullptr? This looks funny to me.

There are all kinds of differences between Agents here. The enable/disable methods are generated, and its up to each agent to choose whether or not it could send an ErrorString in disable. I would really like to make some consistency here eventually. I will look into this eventually!


(In reply to comment #10)
> One nit about the generated InspectorFrontend.h around #ifdefs:
> 
> };
> 
> #endif // ENABLE(JAVASCRIPT_DEBUGGER)
> class InspectorRuntimeFrontendDispatcher {
> 
> The blank line and the #endif should swap. This happens around all the #endifs.

Yep, and this bugs me quite a bit too =(. I know where the newline comes from, but I didn't think of a simple way to clean it up. t want to change how guards are output altogether in the generators, so I'll look into addressing this eventually when I do more work in the generator.

[Joseph Pecoraro]

Lots of "eventually" in that comment. Really you've pointed out all the things that annoyed me with the code as I was writing this patch. So they are all on my list of issues to fix.

[Joseph Pecoraro]

Created attachment 216850 [details]
[BOTS] Updated patch for bots

[Joseph Pecoraro]

Committed <http://trac.webkit.org/changeset/159243>.