Bug 91196 – Web Inspector: refactor InspectorController::connectFrontend() to accept InspectorFrontendChannel.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

We need to refactor InspectorController::connectFrontend() so that it could receive the InspectorFrontendChannel* and use it when creating the front-end. That way, we would be able to plug custom channels into it and bypass platform-specific InspectorClient. As a result, testing harness can inject its own transport channel there.

[Pavel Feldman]

As a result of the refactoring, InspectorClient should stop being inherited from InspectorFrontendChannel.

[Vivek Galatage]

For this refactoring, as now we are planning to remove InspectorFrontendChannel from being inherited by InspectorClient, all the clients must be refactored as well. Here I propose the following approaches:

Approach 1:
-----------
All the clients will implement InspectorClient now without InspectorFrontendChannel. As a result of this, all of the clients(e.g. InspectorClientQt) needs to implement InspectorFrontendChannel separately. Now in this approach, the required objects within InspectorFrontendChannel::sendMessageToFrontend(), can be made members of the class who is implementing InspectorFrontendChannel lets say InspectorFrontendChannelQt. In turn, InspectorClientQt can own this new channel object InspectorFrontendChannelQt(InspectorClientQt::m_frontendChannel). And provide method within InspectorClient to return this channel object as: InspectorFrontendChannel* InspectorClient::frontendChannel()

class InspectorClientQt : public InspectorClient {
   ...
private:
    InspectorFrontendChannelQt* m_frontendChannel;
};

class InspectorFrontendChannelQt : public InspectorFrontendChannel {
public:
    InspectorFrontendChannelQt(QWebPage*, InspectorServerRequestHandlerQt* m_remoteFrontEndChannel);
    bool sendMessageToFrontend(const String& message);

private:
    QWebPage* m_frontendWebPage;
    InspectorServerRequestHandlerQt* m_remoteFrontEndChannel;
};


Approach 2:
-----------
This would be same as above except that the InspectorFrontendChannelQt can be declared as friend of InspectorClientQt. The class InspectorFrontendChannelQt can have just the reference to InspectorClientQt. So while we migrate the sendMessageToFrontend() to this new class InspectorFrontendChannelQt, we can access all the objects owned by InspectorClientQt.

class InspectorClientQt : public InspectorClient {
   ...
private:
    InspectorFrontendChannelQt* m_frontendChannel;
    friend class InspectorFrontendChannelQt;
};

class InspectorFrontendChannelQt : public InspectorFrontendChannel {
public:
    InspectorFrontendChannelQt(InspectorClientQt* m_inspectorClient);
    bool sendMessageToFrontend(const String& message);

private:
    InspectorClientQt* m_inspectorClient;
};


Please let me know your comments/suggestion on these approaches.

[Pavel Feldman]

I was more thinking that:

class InspectorClientQt : public InspectorClient,
                        : public InspectorFrontendChannel {
}

and would pass this into the connectFrontend in order to minimize the amount of changes.

[Vivek Galatage]

(In reply to comment #3)
> I was more thinking that:
> 
> class InspectorClientQt : public InspectorClient,
>                         : public InspectorFrontendChannel {
> }
> 
> and would pass this into the connectFrontend in order to minimize the amount of changes.

Thank you Pavel. I was thinking of the same earlier to minimize the changes. But again I thought, these approaches might separate the classes logically as well. Hence proposed these solutions.

Sure, I will go ahead with these changes as per your proposal.

[Vivek Galatage]

(In reply to comment #4)
> (In reply to comment #3)
> > I was more thinking that:
> > 
> > class InspectorClientQt : public InspectorClient,
> >                         : public InspectorFrontendChannel {
> > }
> > 
> > and would pass this into the connectFrontend in order to minimize the amount of changes.
> 
> Thank you Pavel. I was thinking of the same earlier to minimize the changes. But again I thought, these approaches might separate the classes logically as well. Hence proposed these solutions.
> 
> Sure, I will go ahead with these changes as per your proposal.

We need to also introduce method like frontendChannel() in InspectorClient so as to refer this from InspectorController:

class InspectorClient {
public:
    virtual InspectorFrontendChannel* frontendChannel() const = 0;
}

The method would typically return "this" from the implementation(e.g. InspectorClientQt)

[Pavel Feldman]

> We need to also introduce method like frontendChannel() in InspectorClient so as to refer this from InspectorController:
> 
> class InspectorClient {
> public:
>     virtual InspectorFrontendChannel* frontendChannel() const = 0;
> }
> 
> The method would typically return "this" from the implementation(e.g. InspectorClientQt)

That would be wrong. The point of this refactoring is to separate the channel from the client. Your testing harness will be setting the generic channel, but the clients will be port-specific.

connectFrontend will receive frontend channel instance as a parameter and will store a raw pointer to it in the InspectorController. disconnectFrontend will clear that internal pointer.

[Vivek Galatage]

(In reply to comment #6)
> > We need to also introduce method like frontendChannel() in InspectorClient so as to refer this from InspectorController:
> > 
> > class InspectorClient {
> > public:
> >     virtual InspectorFrontendChannel* frontendChannel() const = 0;
> > }
> > 
> > The method would typically return "this" from the implementation(e.g. InspectorClientQt)
> 
> That would be wrong. The point of this refactoring is to separate the channel from the client. Your testing harness will be setting the generic channel, but the clients will be port-specific.
> 
> connectFrontend will receive frontend channel instance as a parameter and will store a raw pointer to it in the InspectorController. disconnectFrontend will clear that internal pointer.

The method InspectorController::connectFrontend() is mostly called by InspectorController::show() method except WebInspector::remoteFrontendConnected(). So in order for this refactoring, we need to call the connectFrontend(frontendChannel) before every call to InspectorController::show(). But this would bring a lot of changes and also might be erroneous. Hence added the method frontendChannel() in InspectorClient to give the frontendChannel*. This would convey that the InspectorFrontendChannel is owned by InspectorClient object.

[Pavel Feldman]

> The method InspectorController::connectFrontend() is mostly called by InspectorController::show() method except WebInspector::remoteFrontendConnected(). So in order for this refactoring, we need to call the connectFrontend(frontendChannel) before every call to InspectorController::show(). But this would bring a lot of changes and also might be erroneous. Hence added the method frontendChannel() in InspectorClient to give the frontendChannel*. This would convey that the InspectorFrontendChannel is owned by InspectorClient object.

I don't think an API that receives InspectorFrontendChannel on connection and returns it at any time makes sense. What if we change the controller code to:

InspectorFrontencChannel* channel = m_inspectorClient->openInspectorFrontend(this);
connectFrontend(channel);

This would mean that controller can use its client to create front-ends and upon front-end creation it receives its channel. Then you make all InspectorClientXX implementations inherit from InspectorClient and InspectorFrontendChannel and you return "this" in the openInspectorFrontend.

Note that chromium's openInspectorFrontend will return 0 which is Ok.

[Pavel Feldman]

It sounds like there are more connectFrontend() calls btw:

http://code.google.com/p/chromium/source/search?q=connectFrontend%5C%28%5C%29&origq=connectFrontend%5C%28%5C%29&btnG=Search+Trunk

[Vivek Galatage]

(In reply to comment #8)
> > The method InspectorController::connectFrontend() is mostly called by InspectorController::show() method except WebInspector::remoteFrontendConnected(). So in order for this refactoring, we need to call the connectFrontend(frontendChannel) before every call to InspectorController::show(). But this would bring a lot of changes and also might be erroneous. Hence added the method frontendChannel() in InspectorClient to give the frontendChannel*. This would convey that the InspectorFrontendChannel is owned by InspectorClient object.
> 
> I don't think an API that receives InspectorFrontendChannel on connection and returns it at any time makes sense. What if we change the controller code to:
> 
> InspectorFrontencChannel* channel = m_inspectorClient->openInspectorFrontend(this);
> connectFrontend(channel);
> 
> This would mean that controller can use its client to create front-ends and upon front-end creation it receives its channel. Then you make all InspectorClientXX implementations inherit from InspectorClient and InspectorFrontendChannel and you return "this" in the openInspectorFrontend.
> 
> Note that chromium's openInspectorFrontend will return 0 which is Ok.

This sounds perfect. 

(In reply to comment #9)
> It sounds like there are more connectFrontend() calls btw:
> 
> http://code.google.com/p/chromium/source/search?q=connectFrontend%5C%28%5C%29&origq=connectFrontend%5C%28%5C%29&btnG=Search+Trunk

Yes there are more number of calls but these are again from WorkerInspectorController. Also some of the port-specific calls I didn't mention earlier.

Are we targeting the WorkerInspectorController::connectFrontend method as well for refactoring?

[Pavel Feldman]

> Are we targeting the WorkerInspectorController::connectFrontend method as well for refactoring?

We should target it for refactoring, but it should be done in a separate change if possible.

[Vivek Galatage]

Looks like InspectorController::restoreInspectorStateFromCookie() method is being used only by chromium. This method has a call to connectFrontend. What do you think about this call?

I have declared a member m_inspectorFrontendChannel in InspectorController. This would be set to the pointer passed to connectFrontend method and will be cleared upon disconnect. So would passing this member m_inspectorFrontendChannel to the above place make sense?

[Vivek Galatage]

Created attachment 152510 [details]
Work in progress patch. Not ready for port specific changes.

[Pavel Feldman]

Comment on attachment 152510 [details]
Work in progress patch. Not ready for port specific changes.

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

So far looks good!

> Source/WebCore/inspector/InspectorController.cpp:263
> +        m_inspectorFrontendChannel = m_inspectorClient->openInspectorFrontend(this);

The call below will do this for you, store it in a temporary variable for now.

> Source/WebCore/inspector/InspectorController.cpp:279
> +    connectFrontend(m_inspectorFrontendChannel);

I would rename and extend "restoreInspectorStateFromCookie" to

InspectorController::reconnectFrontend(InspectorFrontendChannel* channel, const String& inspectorStateCookie)

> Source/WebCore/loader/EmptyClients.h:565
> +class EmptyInspectorClient : public InspectorClient, public InspectorFrontendChannel {

This should not change, you should instead remove sendMessageToFrontend from it.

> Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp:174
> +        return 0;

We should make sure call site is ready to accept 0 (it should not crash).

[Vivek Galatage]

Created attachment 152598 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152598 [details]
Patch

Attachment 152598 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13265050

[Early Warning System Bot]

Comment on attachment 152598 [details]
Patch

Attachment 152598 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13265068

[Vivek Galatage]

Created attachment 152693 [details]
Patch

[Early Warning System Bot]

Comment on attachment 152693 [details]
Patch

Attachment 152693 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13278046

[Vivek Galatage]

Created attachment 152699 [details]
Patch

[Pavel Feldman]

Comment on attachment 152699 [details]
Patch

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

I only have two small comments, otherwise it looks good!

> Source/WebCore/inspector/InspectorController.cpp:216
> +    m_inspectorFrontendChannel = frontendChannel;

You no longer need to store it locally, just create the front-end off the channel that is passed!

> Source/WebKit2/WebProcess/WebPage/WebPage.h:193
> +    WebInspectorClient* inspectorClient() const { return m_inspectorClient; };

Instead of exposing it, I would make WebInspector constructor receive the InspectorFrontendChannel* in the constructor to minimize the API surface.

> Source/WebKit/blackberry/Api/WebPage.cpp:493
> +    m_inspectorClient = new WebCore::InspectorClientBlackBerry(this);

It is using namespace WebCore at the top, no need for WebCore.

> Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:392
> +    inspectorController()->connectFrontend(this);

FYI: You should have passed InspectorClientImpl instance here, but I actually think this should work since the agent has greater lifespan.

[Vivek Galatage]

(In reply to comment #21)
> (From update of attachment 152699 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=152699&action=review
> 
> I only have two small comments, otherwise it looks good!
> 
> > Source/WebCore/inspector/InspectorController.cpp:216
> > +    m_inspectorFrontendChannel = frontendChannel;
> 
> You no longer need to store it locally, just create the front-end off the channel that is passed!
> 

Sure. Will do.

> > Source/WebKit2/WebProcess/WebPage/WebPage.h:193
> > +    WebInspectorClient* inspectorClient() const { return m_inspectorClient; };
> 
> Instead of exposing it, I would make WebInspector constructor receive the InspectorFrontendChannel* in the constructor to minimize the API surface.
> 

Will do. 

Now that WebInspector will have a member to the channel, this must be set to null in WebInspector::destoryInspectorPage(). Is this correct?

> > Source/WebKit/blackberry/Api/WebPage.cpp:493
> > +    m_inspectorClient = new WebCore::InspectorClientBlackBerry(this);
> 
> It is using namespace WebCore at the top, no need for WebCore.
> 

Will do.

> > Source/WebKit/chromium/src/WebDevToolsAgentImpl.cpp:392
> > +    inspectorController()->connectFrontend(this);
> 
> FYI: You should have passed InspectorClientImpl instance here, but I actually think this should work since the agent has greater lifespan.

I am not quite sure, if I understand this correctly.

[Vivek Galatage]

Created attachment 152721 [details]
Patch

[WebKit Review Bot]

Comment on attachment 152721 [details]
Patch

Clearing flags on attachment: 152721

Committed r122838: <http://trac.webkit.org/changeset/122838>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.