Summary: | Web Inspector: [refacotring] merge InspectorAgent::willSendRequest() into InspectorResourceAgnet | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrey Kosyakov <caseq> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Andrey Kosyakov <caseq> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, apavlov, bweinstein, dglazkov, eric, gustavo, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, xan.lopez, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Andrey Kosyakov
2011-02-04 10:45:56 PST
Created attachment 81247 [details]
patch
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. 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
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.
Attachment 81481 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7702898 Attachment 81481 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7700918 (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. Created attachment 81486 [details]
patch
- Fixed style & gcc errors
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 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 Attachment 81481 [details] did not build on mac: Build output: http://queues.webkit.org/results/7709051 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)
Manually committed r77925: http://trac.webkit.org/changeset/77925 http://trac.webkit.org/changeset/77925 might have broken GTK Linux 32-bit Release |