Bug 46802 – Web Inspector: extract consoleMessages related stuff from populateScriptObjects into separate function.

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

[Ilya Tikhonovsky]

this is a part of Inspector protocol sanitization activity.
We want to populate console messages only if it is required by frontend.

[Ilya Tikhonovsky]

Created attachment 69195 [details]
[patch] initial version.

[Ilya Tikhonovsky]

Comment on attachment 69195 [details]
[patch] initial version.

the idea was changed.

[Ilya Tikhonovsky]

Created attachment 69605 [details]
[patch] second iteration

[Yury Semikhatsky]

Comment on attachment 69605 [details]
[patch] second iteration

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

> WebCore/inspector/InspectorController.cpp:127
> +static const char* const consoleMessagesNotificationsStateName = "consoleMessagesNotificationsEnabled";

Should we move this constants into a class/namespace InspectorSettingNames instead of using *StateName suffix?

> WebCore/inspector/InspectorController.cpp:162
> +    , m_consoleMessagesNotifications(false)

m_consoleMessagesNotifications -> m_consoleMessagesNotificationsEnabled

> WebCore/inspector/InspectorController.cpp:603
> +    m_consoleMessagesNotifications = false;

This will be called only on close of the detached window. When attached window is closed only disconnectFrontend will be invoked. r- for this.

> WebCore/inspector/InspectorController.h:150
> +    void setConsoleMessagesNotificationsEnabled(bool enabled);

should be private

[Ilya Tikhonovsky]

Created attachment 69635 [details]
[patch] third iteration.

[Pavel Feldman]

Comment on attachment 69635 [details]
[patch] third iteration.

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

> WebCore/inspector/InspectorController.h:362
> +    bool m_consoleMessagesNotificationsEnabled;

This seems to be a new kind of flag that reflects the front-end state. It should vanish on front-end disconnect. We already have way too many settings, states for sessions, application, etc. Should we create structs with descriptive names for them already?

[Ilya Tikhonovsky]

Created attachment 70855 [details]
[patch] next iteration.

[Yury Semikhatsky]

Comment on attachment 70855 [details]
[patch] next iteration.

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

> WebCore/inspector/InspectorController.cpp:625
> +    unsigned messageCount = m_consoleMessages.size();

unsigned -> size_t

> WebCore/inspector/InspectorState.cpp:54
> +    registerBoolean(consoleMessagesNotificationsEnabled, false, "consoleMessagesNotifications", (const char*)0);

There should be a single place where constants like "consoleMessagesNotifications" are declared.

[Ilya Tikhonovsky]

Created attachment 70865 [details]
[patch] next iteration.

New version of patch which is less generic. 
It'd be better to implement the generic version later with help of InspectorGenerator just for strict arguments check.

[Eric Seidel (no email)]

Comment on attachment 70855 [details]
[patch] next iteration.

Cleared Yury Semikhatsky's review+ from obsolete attachment 70855 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[WebKit Commit Bot]

Comment on attachment 70865 [details]
[patch] next iteration.

Clearing flags on attachment: 70865

Committed r69853: <http://trac.webkit.org/changeset/69853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.