[webkit-reviews] review denied: [Bug 53789] Web Inspector: [refacotring] merge InspectorAgent::willSendRequest() into InspectorResourceAgnet : [Attachment 81247] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Andrey Kosyakov
<caseq at chromium.org>'s request for review:
Bug 53789: Web Inspector: [refacotring] merge InspectorAgent::willSendRequest()
into InspectorResourceAgnet
https://bugs.webkit.org/show_bug.cgi?id=53789

Attachment 81247: patch
https://bugs.webkit.org/attachment.cgi?id=81247&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=81247&action=review

I think you can nuke even more code here (all these cycles, etc.) Plus we need
good way of defining default values.

> Source/WebCore/inspector/InspectorAgent.h:270
>      void setUserAgentOverride(const String& userAgent);

I am in doubts on whether this one should got to the network domain or not.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:386
> +    ia->applyUserAgentOverride(userAgent);

ia -> inspectorAgent. ic confuses you, but we do not abbreviate in other
places.

> Source/WebCore/inspector/InspectorInstrumentation.h:606
> +    if (InspectorAgent* ic = inspectorAgentWithFrontendForFrame(frame))

inspectorAgent. I know, there has been a massive rename
InspectorController->InspectorAgent and these ics are now all over the place.
We should clean them up.

> Source/WebCore/inspector/InspectorState.cpp:-42
> -    registerBoolean(userInitiatedProfiling, false);

How do I make property value 'true' (or '5') by default?

> Source/WebCore/inspector/InspectorState.cpp:55
>	   ASSERT(id > 0 && id < lastPropertyId);

There is no persistence for these - they are only managed at runtime. This
assert does not make much sense (as InspectorPropertyId itself).

> Source/WebCore/inspector/InspectorState.h:-8
> - * 1.  Redistributions of source code must retain the above copyright

You should not do things like this post-factum.

> Source/WebCore/inspector/InspectorState.h:60
>	   lastPropertyId

We don't seem to iterate over values anymore, so lastPropertyId is of no use,
right? Why not to use const char[] names for these instead of
InspectorPropertyId class? Then entire store would be a simple InspectorObject
and cookie serialization would be trivial.