Bug 52282 – Web Inspector: extract console related functionality into InspectorConsoleAgent

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

[Yury Semikhatsky]

Web Inspector: extract console related functionality into InspectorConsoleAgent. There are several methods and fields in InspectorController that deal with console messages and they should be moved into their own class.

[Yury Semikhatsky]

Created attachment 78808 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 78808 [details]
Patch

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


> Source/WebCore/inspector/Inspector.idl:83
>          [domain=Inspector] void clearConsoleMessages();

I think it also should be a member of Console domain.


other looks good to me

[Yury Semikhatsky]

Created attachment 78813 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #2)
> (From update of attachment 78808 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78808&action=review
> 
> 
> > Source/WebCore/inspector/Inspector.idl:83
> >          [domain=Inspector] void clearConsoleMessages();
> 
> I think it also should be a member of Console domain.
> 
Done.

[Pavel Feldman]

Comment on attachment 78813 [details]
Patch

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

> Source/WebCore/inspector/front-end/ConsoleView.js:252
> +    addConsoleMessage: function(payload)

I know Ilya has been doing it earlier, but I think it is wrong. We should dispatch against private closure.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Early Warning System Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Pavel Feldman]

Comment on attachment 78813 [details]
Patch

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

Please fix these two comments before landing.

> Source/WebCore/inspector/Inspector.idl:79
> +        [domain=Console] void setConsoleMessagesEnabled(in boolean enabled, out boolean newState);

I think this should belong to the "Inspector" domain. Upon this call, inspector controller should provide console agent with the InspectorFrontend instance.

> Source/WebCore/inspector/InspectorConsoleAgent.cpp:188
> +    m_state->setBoolean(InspectorState::consoleMessagesEnabled, enabled);

If you do what I suggest above, you would no longer need this property (it'll be m_frontend != 0).

[Martin Robinson]

(In reply to comment #12)
> (From update of attachment 78813 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78813&action=review

This patch broke every EWS bot. It might be better to post another iteration.

[Yury Semikhatsky]

Comment on attachment 78813 [details]
Patch

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

>> Source/WebCore/inspector/InspectorConsoleAgent.cpp:188

> 
> If you do what I suggest above, you would no longer need this property (it'll be m_frontend != 0).

This won't work since we need to restore console state after navigation.

[Yury Semikhatsky]

Created attachment 78926 [details]
Patch that I'm going to land

[Yury Semikhatsky]

Committed r75792