[webkit-reviews] review denied: [Bug 58064] Web Inspector: Network events don't preserves, when inspector frontend closed and open again : [Attachment 89009] Add new files in WebCore/WebCore.pro file

[bugzilla-daemon at webkit.org]

Yury Semikhatsky <yurys at chromium.org> has denied Sergey Vorobyev
<sergeyvorobyev at google.com>'s request for review:
Bug 58064: Web Inspector: Network events don't preserves, when inspector
frontend closed and open again
https://bugs.webkit.org/show_bug.cgi?id=58064

Attachment 89009: Add new files in WebCore/WebCore.pro file
https://bugs.webkit.org/attachment.cgi?id=89009&action=review

------- Additional Comments from Yury Semikhatsky <yurys at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=89009&action=review

> Source/WebCore/ChangeLog:9
> +	   No new tests. (OOPS!)

Would be really nice to have a test for this change.

> Source/WebCore/inspector/EventsCollector.cpp:38
> +{ }

style: closing bracket should go on a new line.

> Source/WebCore/inspector/EventsCollector.cpp:42
> +    totalLength += (int)message.length();

we don't use c-style casts in WebCore code. You could use size_t as
m_totalLength type.

> Source/WebCore/inspector/EventsCollector.h:42
> +    void collect(const String& message);

I'd rename it to addEvent

> Source/WebCore/inspector/EventsCollector.h:43
> +    void pushData(InspectorFrontendChannel*);

Consider renaming pushData to sendCollectedEvents?

> Source/WebCore/inspector/EventsCollector.h:45
> +    static const int CAPACITY = 1048576; // 1 Mb

WebCore constant names should be in camelCase, like maxCapacity. Also please
use 1024*1024 instead of the number, compiler will do this evaluation for you.

> Source/WebCore/inspector/EventsCollector.h:46
> +    int totalLength;

style: private fields should start with m_
prefix(http://www.webkit.org/coding/coding-style.html)

> Source/WebCore/inspector/InspectorFrontendProxy.h:40
> +    InspectorFrontendProxy(EventsCollector*);

Constructors with single argument should be marked as "explicit".

> Source/WebCore/inspector/InspectorResourceAgent.cpp:549
> +    m_frontend = new
InspectorFrontend::Network(m_inspectorFrontendProxy.get());

You're leaking the Network object here. Instead it should be owned by the agent
and created only when background resource tracking is enabled.