Bug 53169

Summary: Web Inspector: move InspectorController's methods from InspectorAgent to InspectorController
Product: WebKit Reporter: Ilya Tikhonovsky <loislo>
Component: Web Inspector (Deprecated)Assignee: Ilya Tikhonovsky <loislo>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, apavlov, buildbot, bweinstein, dglazkov, eric, gustavo.noronha, gustavo, joepeck, keishi, loislo, ossy, pfeldman, pmuellr, rik, timothy, webkit-ews, webkit.review.bot, xan.lopez, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 53580    
Bug Blocks:    
Attachments:
Description Flags
[patch] initial version. for trybots
none
[patch] second version
none
[patch] third version version
none
[patch] fourth version
none
patch I'm going to land none

Description Ilya Tikhonovsky 2011-01-26 07:41:32 PST
%subj%
Comment 1 Ilya Tikhonovsky 2011-01-26 07:50:25 PST
Created attachment 80192 [details]
[patch] initial version. for trybots
Comment 2 Pavel Feldman 2011-01-26 08:17:04 PST
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.
Comment 3 Build Bot 2011-01-26 08:25:02 PST
Attachment 80192 [details] did not build on win:
Build output: http://queues.webkit.org/results/7523362
Comment 4 WebKit Review Bot 2011-01-26 08:51:55 PST
Attachment 80192 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7592372
Comment 5 Ilya Tikhonovsky 2011-02-02 10:14:36 PST
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.
Comment 6 Early Warning System Bot 2011-02-02 10:51:12 PST
Attachment 80927 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7503449
Comment 7 Yury Semikhatsky 2011-02-03 02:31:20 PST
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.
Comment 8 Yury Semikhatsky 2011-02-03 04:44:52 PST
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.
Comment 9 Yury Semikhatsky 2011-02-03 04:54:43 PST
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.
Comment 10 Yury Semikhatsky 2011-02-03 04:56:34 PST
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.
Comment 11 Ilya Tikhonovsky 2011-02-08 02:31:45 PST
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
Comment 12 WebKit Review Bot 2011-02-08 02:33:56 PST
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.
Comment 13 Yury Semikhatsky 2011-02-08 05:16:50 PST
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?
Comment 14 Ilya Tikhonovsky 2011-02-08 06:45:25 PST
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
Comment 15 WebKit Review Bot 2011-02-08 06:51:24 PST
Attachment 81629 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7797080
Comment 16 Collabora GTK+ EWS bot 2011-02-08 06:54:07 PST
Attachment 81629 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7847070
Comment 17 Early Warning System Bot 2011-02-08 07:03:11 PST
Attachment 81629 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7797087
Comment 18 Build Bot 2011-02-08 07:15:40 PST
Attachment 81629 [details] did not build on win:
Build output: http://queues.webkit.org/results/7791080
Comment 19 WebKit Review Bot 2011-02-08 07:31:17 PST
Attachment 81629 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7801087
Comment 20 WebKit Review Bot 2011-02-08 07:31:24 PST
Attachment 81629 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7763099
Comment 21 Ilya Tikhonovsky 2011-02-08 09:33:57 PST
Created attachment 81652 [details]
patch I'm going to land
Comment 22 WebKit Review Bot 2011-02-08 10:15:28 PST
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
Comment 23 Csaba Osztrogonác 2011-02-08 10:22:55 PST
(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.