Bug 53789 – Web Inspector: [refacotring] merge InspectorAgent::willSendRequest() into InspectorResourceAgnet

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

[Andrey Kosyakov]

- Merge InspectorAgent::willSendRequest() into InspectorResourceAgent::willSendRequest()
- Move extra request handling to InspectorResourceAgent
- Perform UserAgent override via InspectorInstrumentation method
- Remove redudant stuff from InspectorState

[Andrey Kosyakov]

Created attachment 81247 [details]
patch

[Pavel Feldman]

Comment on attachment 81247 [details]
patch

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.

[Andrey Kosyakov]

Created attachment 81481 [details]
patch

- Changed inspector state to use string property names instead of enum
- Moved state property names definitions to agents that need them

[WebKit Review Bot]

Attachment 81481 [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/InspectorDebuggerAgent.cpp:46:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/inspector/InspectorBrowserDebuggerAgent.h:54:  The parameter name "inspectorAgent" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/inspector/InspectorAgent.cpp:133:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/inspector/InspectorBrowserDebuggerAgent.cpp:65:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/inspector/InspectorConsoleAgent.cpp:53:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Source/WebCore/inspector/InspectorResourceAgent.cpp:71:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 6 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

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

[Gustavo Noronha (kov)]

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

[Andrey Kosyakov]

(In reply to comment #2)
> (From update of attachment 81247 [details])
> 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.

I'd rather take care of default values when/if we ever need them -- otherwise that would be a dead code. The registerXXX methods I dropped were literally were almost like setXXX methods (except that latter used to check for the property being present) -- so we could always had default values set in the constructor. Alternatively, we could have an optional default value in getXXX methods, e.g. getLong(const String& propertyName, long defaultValue = 0);
 
> > 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.

Technically, we apply it within FrameLoader, but I thing logically it's more general than network, since it also affects the value returned by navigator.userAgent -- so I'd propose to leave it on InspectorAgent.

> > Source/WebCore/inspector/InspectorInstrumentation.cpp:386
> > +    ia->applyUserAgentOverride(userAgent);
> 
> ia -> inspectorAgent. ic confuses you, but we do not abbreviate in other places.

done.

> > 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?

Do we need this now? I'd rather implement it on demand.

> > 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).

Gone with the numeric ids.

> > 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.

I'm just fixing obviously inconsistent copyright -- this had only Google on top (copyright section) but Apple in endorsement/no warranty sections.

> > 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.

Done -- and I also moved property name definitions to the agents that use them.

[Andrey Kosyakov]

Created attachment 81486 [details]
patch

- Fixed style & gcc errors

[WebKit Review Bot]

Attachment 81486 [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/InspectorBrowserDebuggerAgent.h:54:  The parameter name "inspectorAgent" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 81486 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +        https://bugs.webkit.org/show_bug.cgi?id=53789

Web Inspector: bug title
http://bugs...

Description

*

Agnet -> Agent

[WebKit Review Bot]

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

[Andrey Kosyakov]

Created attachment 81625 [details]
landed patch

A variation of the previous patch landed (moved clearing of breakpoints from inspector state from factories to agent constructors)

[Andrey Kosyakov]

Manually committed r77925: http://trac.webkit.org/changeset/77925

[WebKit Review Bot]

http://trac.webkit.org/changeset/77925 might have broken GTK Linux 32-bit Release