Bug 53789 - Web Inspector: [refacotring] merge InspectorAgent::willSendRequest() into InspectorResourceAgnet
Summary: Web Inspector: [refacotring] merge InspectorAgent::willSendRequest() into Ins...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Kosyakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-04 10:45 PST by Andrey Kosyakov
Modified: 2011-02-08 07:07 PST (History)
16 users (show)

See Also:


Attachments
patch (21.97 KB, patch)
2011-02-04 11:09 PST, Andrey Kosyakov
pfeldman: review-
Details | Formatted Diff | Diff
patch (42.53 KB, patch)
2011-02-07 09:28 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff
patch (42.48 KB, patch)
2011-02-07 10:17 PST, Andrey Kosyakov
pfeldman: review+
Details | Formatted Diff | Diff
landed patch (44.20 KB, patch)
2011-02-08 05:48 PST, Andrey Kosyakov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Kosyakov 2011-02-04 10:45:56 PST
- Merge InspectorAgent::willSendRequest() into InspectorResourceAgent::willSendRequest()
- Move extra request handling to InspectorResourceAgent
- Perform UserAgent override via InspectorInstrumentation method
- Remove redudant stuff from InspectorState
Comment 1 Andrey Kosyakov 2011-02-04 11:09:36 PST
Created attachment 81247 [details]
patch
Comment 2 Pavel Feldman 2011-02-04 11:31:54 PST
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.
Comment 3 Andrey Kosyakov 2011-02-07 09:28:29 PST
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
Comment 4 WebKit Review Bot 2011-02-07 09:30:34 PST
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.
Comment 5 WebKit Review Bot 2011-02-07 09:38:15 PST
Attachment 81481 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7702898
Comment 6 Gustavo Noronha (kov) 2011-02-07 09:40:00 PST
Attachment 81481 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7700918
Comment 7 Andrey Kosyakov 2011-02-07 10:16:20 PST
(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.
Comment 8 Andrey Kosyakov 2011-02-07 10:17:06 PST
Created attachment 81486 [details]
patch

- Fixed style & gcc errors
Comment 9 WebKit Review Bot 2011-02-07 10:19:52 PST
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.
Comment 10 Pavel Feldman 2011-02-07 10:24:02 PST
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
Comment 11 WebKit Review Bot 2011-02-07 10:32:02 PST
Attachment 81481 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7709051
Comment 12 Andrey Kosyakov 2011-02-08 05:48:12 PST
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)
Comment 13 Andrey Kosyakov 2011-02-08 05:49:27 PST
Manually committed r77925: http://trac.webkit.org/changeset/77925
Comment 14 WebKit Review Bot 2011-02-08 07:07:50 PST
http://trac.webkit.org/changeset/77925 might have broken GTK Linux 32-bit Release