Bug 53169 – Web Inspector: move InspectorController's methods from InspectorAgent to InspectorController

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

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

Created attachment 80192 [details]
[patch] initial version. for trybots

[Pavel Feldman]

Comment on attachment 80192 [details]
[patch] initial version. for trybots

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

A bunch of nits, otherwise looks good.

> Source/WebCore/ChangeLog:12
> +        https://bugs.webkit.org/show_bug.cgi?id=53169

ChangeLog structure should be:

Web Inspector: <Bug title>
http://bug
<blank line>
Description.
<blank line>

> Source/WebCore/inspector/InspectorController.cpp:167
> +    m_inspectorAgent->evaluateForTestInFrontend(callId, script);

This can go straight into the front-end.

> Source/WebCore/inspector/InspectorController.h:65
> +    InspectorAgent* inspectorAgent() const { return m_inspectorAgent.get(); }

You can't expose agent from inspector controller.

> Source/WebCore/inspector/InspectorController.h:66
> +    InspectorBackendDispatcher* inspectorBackendDispatcher() { return m_inspectorBackendDispatcher.get(); }

replace with wrapper method: dispatchMessageFromFrontend

> Source/WebCore/inspector/InspectorController.h:67
> +    InspectorClient* inspectorClient() const { return m_inspectorClient; }

Should not be exposed. Or at least put a FIXME here.

> Source/WebCore/inspector/InspectorController.h:82
> +    void reuseFrontend();

I don't think we have reuseFrontend.

> Source/WebCore/inspector/InspectorController.h:87
> +    void highlight(Node*);

This code should move to HighlightUtilities class.

> Source/WebCore/inspector/InspectorController.h:94
> +    // FIXME: these methods are necessary for LayoutTestController but we can do that with help of script injected into WebInspector page.

These should stay here.

> Source/WebCore/loader/FrameLoader.cpp:2981
> +    InspectorInstrumentation::resume(m_frame->page());

resume is not a part of the instrumentation, this is a control statement, should be called on InspectorController.

[Build Bot]

Attachment 80192 [details] did not build on win:
Build output: http://queues.webkit.org/results/7523362

[WebKit Review Bot]

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

[Ilya Tikhonovsky]

Created attachment 80927 [details]
[patch] second version

(In reply to comment #2)
> (From update of attachment 80192 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80192&action=review
> 
> A bunch of nits, otherwise looks good.
> 
> > Source/WebCore/ChangeLog:12
> > +        https://bugs.webkit.org/show_bug.cgi?id=53169
> 
> ChangeLog structure should be:
> 
> Web Inspector: <Bug title>
> http://bug
> <blank line>
> Description.
> <blank line>

done.

> 
> > Source/WebCore/inspector/InspectorController.cpp:167
> > +    m_inspectorAgent->evaluateForTestInFrontend(callId, script);
> 
> This can go straight into the front-end.

done.

> 
> > Source/WebCore/inspector/InspectorController.h:65
> > +    InspectorAgent* inspectorAgent() const { return m_inspectorAgent.get(); }
> 
> You can't expose agent from inspector controller.

done.

> 
> > Source/WebCore/inspector/InspectorController.h:66
> > +    InspectorBackendDispatcher* inspectorBackendDispatcher() { return m_inspectorBackendDispatcher.get(); }
> 
> replace with wrapper method: dispatchMessageFromFrontend

done.

> > Source/WebCore/inspector/InspectorController.h:67
> > +    InspectorClient* inspectorClient() const { return m_inspectorClient; }
> 
> Should not be exposed. Or at least put a FIXME here.
> 
> > Source/WebCore/inspector/InspectorController.h:82
> > +    void reuseFrontend();
> 
> I don't think we have reuseFrontend.

done.

> 
> > Source/WebCore/inspector/InspectorController.h:87
> > +    void highlight(Node*);
> 
> This code should move to HighlightUtilities class.

I'll do this in a separate patch.


> 
> > Source/WebCore/loader/FrameLoader.cpp:2981
> > +    InspectorInstrumentation::resume(m_frame->page());
> 
> resume is not a part of the instrumentation, this is a control statement, should be called on InspectorController.

done.

[Early Warning System Bot]

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

[Yury Semikhatsky]

Comment on attachment 80927 [details]
[patch] second version

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

> Source/WebCore/inspector/InspectorAgent.cpp:-161
> -    ASSERT(!m_highlightedNode);

Please revert this line.

> Source/WebCore/inspector/InspectorAgent.cpp:189
> +        m_inspectorController->connectFrontend();

InspectorAgent shouldn't know about InspectorController. You can break this dependency in a separate change though. Please file a bug and add a FIXME.

> Source/WebCore/inspector/InspectorAgent.cpp:289
> +        m_inspectorController->inspect(node.get());

You shouldn't go into IC here.

[Yury Semikhatsky]

Comment on attachment 80927 [details]
[patch] second version

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

> Source/WebCore/loader/FrameLoader.cpp:2977
> +    m_frame->page()->inspectorController()->resume();

You may want to assign m_frame->page() to a local variable since it is used in several places in the method.

[Yury Semikhatsky]

Comment on attachment 80927 [details]
[patch] second version

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

> Source/WebCore/inspector/InspectorController.h:37
> +#include <wtf/PassOwnPtr.h>

You don't need PassOwnPtr and RefPtr includes here, wtf/Forward should be enough.

[Yury Semikhatsky]

Comment on attachment 80927 [details]
[patch] second version

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

> Source/WebCore/inspector/InspectorController.h:111
> +    friend class InspectorAgent;

We could introduce InspectorAgentClient and have InspectorController implement it. Then you wouldn't need this friendship.

[Ilya Tikhonovsky]

Created attachment 81617 [details]
[patch] third version version

(In reply to comment #7)
> (From update of attachment 80927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80927&action=review
> 
> > Source/WebCore/inspector/InspectorAgent.cpp:-161
> > -    ASSERT(!m_highlightedNode);
> 
> Please revert this line.
done


> > Source/WebCore/inspector/InspectorAgent.cpp:189
> > +        m_inspectorController->connectFrontend();
> 
> InspectorAgent shouldn't know about InspectorController. You can break this dependency in a separate change though. Please file a bug and add a FIXME.

done

> > Source/WebCore/inspector/InspectorAgent.cpp:289
> > +        m_inspectorController->inspect(node.get());
> 
> You shouldn't go into IC here.

done

[WebKit Review Bot]

Attachment 81617 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/inspector/InspectorController.h:36:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Yury Semikhatsky]

Comment on attachment 81617 [details]
[patch] third version version

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

> Source/WebCore/inspector/InspectorAgent.cpp:295
> +void InspectorAgent::didClearWindowObjectInWorld(Frame* frame)

This method should be renamed since it doesn't accept any world anymore.

> Source/WebCore/inspector/InspectorAgent.cpp:454
> +    if (!m_requiredPanel) {

!m_requiredPanel -> m_requiredPanel

> Source/WebCore/loader/FrameLoader.cpp:2984
> +    m_frame->page()->inspectorController()->resume();

Why has the main frame check gone?

[Ilya Tikhonovsky]

Created attachment 81629 [details]
[patch] fourth version

(In reply to comment #13)
> (From update of attachment 81617 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81617&action=review
> 
> > Source/WebCore/inspector/InspectorAgent.cpp:295
> > +void InspectorAgent::didClearWindowObjectInWorld(Frame* frame)
> 
> This method should be renamed since it doesn't accept any world anymore.

done


> > Source/WebCore/inspector/InspectorAgent.cpp:454
> > +    if (!m_requiredPanel) {
> 
> !m_requiredPanel -> m_requiredPanel

done

> > Source/WebCore/loader/FrameLoader.cpp:2984
> > +    m_frame->page()->inspectorController()->resume();
> 
> Why has the main frame check gone?

done

[WebKit Review Bot]

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

[Collabora GTK+ EWS bot]

Attachment 81629 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7847070

[Early Warning System Bot]

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

[Build Bot]

Attachment 81629 [details] did not build on win:
Build output: http://queues.webkit.org/results/7791080

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Ilya Tikhonovsky]

Created attachment 81652 [details]
patch I'm going to land

[WebKit Review Bot]

http://trac.webkit.org/changeset/77950 might have broken Qt Linux Release
The following tests are not passing:
http/tests/websocket/tests/workers/close-in-onmessage-crash.html

[Csaba Osztrogonác]

(In reply to comment #22)
> http://trac.webkit.org/changeset/77950 might have broken Qt Linux Release
> The following tests are not passing:
> http/tests/websocket/tests/workers/close-in-onmessage-crash.html

It is a false positive alarm: https://bugs.webkit.org/show_bug.cgi?id=53912
Sorry for the SPAM.