Summary: | Web Inspector: remove some unnecessary Inspector prefixes from class names in Inspector namespace | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Brian Burg <burg> | ||||||||||||||||
Component: | Web Inspector | Assignee: | Brian Burg <burg> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | commit-queue, graouts, japhet, joepeck, jonowells, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 141608 | ||||||||||||||||||
Attachments: |
|
Description
Brian Burg
2015-02-08 12:32:12 PST
Created attachment 246246 [details]
Proposed Fix
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`) Attachment 246246 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/ConsoleMessage.cpp:180: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:85: [CppBackendDispatcherHeaderGenerator._generate_alternate_handler_forward_declarations_for_domains] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5]
Total errors found: 2 in 112 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246273 [details]
Proposed Fix
Attachment 246273 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/ConsoleMessage.cpp:180: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:85: [CppBackendDispatcherHeaderGenerator._generate_alternate_handler_forward_declarations_for_domains] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5]
Total errors found: 2 in 111 files
If any of these errors are false positives, please file a bug against check-webkit-style.
> /Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphoneos/DerivedSources/JavaScriptCore/InspectorBackendDispatchers.cpp:39:10: fatal error: 'Inspector::AlternateBackendDispatchers.h' file not found
> #include "Inspector::AlternateBackendDispatchers.h"
> ^
> 1 error generated.
Hmm, weird name for an include seen on the iOS bot.
Comment on attachment 246273 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=246273&action=review Seems this could go another round to clean up Inspector::'s. Overall I think its a good step forward tho. r- so I can see another patch. And I'll likely need to try a patch in advance to make sure Augmentation Agents stuff still works correct. > Source/JavaScriptCore/inspector/InspectorAgentBase.h:49 > + virtual void didCreateFrontendAndBackend(Inspector::FrontendChannel*, Inspector::BackendDispatcher*) = 0; > + virtual void willDestroyFrontendAndBackend(Inspector::DisconnectReason) = 0; No need for the "Inspector::" namespace here. > Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.h:75 > + void connectFrontend(Inspector::FrontendChannel*, bool isAutomaticInspection); > + void disconnectFrontend(Inspector::DisconnectReason); Likewise. Seems the FrontendChannel > Source/JavaScriptCore/inspector/agents/InspectorAgent.h:56 > + virtual void didCreateFrontendAndBackend(Inspector::FrontendChannel*, Inspector::BackendDispatcher*) override; > + virtual void willDestroyFrontendAndBackend(Inspector::DisconnectReason) override; Ditto. Seems like move places with FrontendChannel / DisconnectReason can avoid it. > Source/JavaScriptCore/inspector/agents/JSGlobalObjectRuntimeAgent.cpp:46 > + m_frontendDispatcher = std::make_unique<Inspector::RuntimeFrontendDispatcher>(frontendChannel); > + m_backendDispatcher = Inspector::RuntimeBackendDispatcher::create(backendDispatcher, this); Hmm, lots of unnecessary Inspector::'s actually. Maybe that was intentional? I would expect FooBackendDispatcher classes to always be unique. > Source/JavaScriptCore/inspector/augmentable/AlternateDispatchableAgent.h:31 > +#include "Inspector::AlternateBackendDispatchers.h" This looks bad! > Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_implementation.py:59 > + secondary_includes.append('#include "Inspector::AlternateBackendDispatchers.h"') This also looks bad. > Source/JavaScriptCore/inspector/scripts/tests/expected/commands-with-async-attribute.json-result:1 > +### Begin File: Inspector::AlternateBackendDispatchers.h Eek. > Source/WebCore/WebCore.order:6337 > +__ZN7WebCore19Inspector::Controller18disconnectFrontendEv We shouldn't change the order file, and these changes wouldn't make sense. > Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.h:56 > - WebCore::InspectorFrontendChannel* openInspectorFrontend(WebCore::InspectorController*) override; > + Inspector::FrontendChannel* openInspectorFrontend(WebCore::InspectorController*) override; Did we remove WebCore::InspectorFrontendChannel? I thought it was a typedef, that would prevent WebKit clients from having issue. > Source/WebKit2/mac/WebKit2.order:1676 > +__ZN6WebKit12WebInspector6createEPNS_7WebPageEPN7WebCore15FrontendChannelE Ditto. Comment on attachment 246273 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=246273&action=review >> Source/JavaScriptCore/inspector/InspectorAgentBase.h:49 >> + virtual void willDestroyFrontendAndBackend(Inspector::DisconnectReason) = 0; > > No need for the "Inspector::" namespace here. I opted to include it in headers, to make mass searching easier. But I think I'll remove it for code in JavaScriptCore. >> Source/JavaScriptCore/inspector/agents/JSGlobalObjectRuntimeAgent.cpp:46 >> + m_backendDispatcher = Inspector::RuntimeBackendDispatcher::create(backendDispatcher, this); > > Hmm, lots of unnecessary Inspector::'s actually. Maybe that was intentional? I would expect FooBackendDispatcher classes to always be unique. I think I prefer keeping the namespace for the per-agent classes, and removing it otherwise (DisconnectReason, etc). >> Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.h:56 >> + Inspector::FrontendChannel* openInspectorFrontend(WebCore::InspectorController*) override; > > Did we remove WebCore::InspectorFrontendChannel? I thought it was a typedef, that would prevent WebKit clients from having issue. it is re-exported from the WebCore namespace. This hasn't changed, but if WebKit clients are like to use this type, I'll change this line back. Created attachment 246290 [details]
Proposed Fix .2
Attachment 246290 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/ConsoleMessage.cpp:180: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:85: [CppBackendDispatcherHeaderGenerator._generate_alternate_handler_forward_declarations_for_domains] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5]
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 3 in 99 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246310 [details]
Patch (fix Windows)
Attachment 246310 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/ConsoleMessage.cpp:180: Code inside a namespace should not be indented. [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:85: [CppBackendDispatcherHeaderGenerator._generate_alternate_handler_forward_declarations_for_domains] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5]
Total errors found: 2 in 99 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246586 [details]
[PATCH] ObjC Generation Changes (needs test rebaseline)
These are the changes to get the ObjC Portion building properly.
- AlternateDispatchableAgent.h the template typename "BackendDispatcher" hilariously conflicted with the actual Inspector::BackendDispatcher. Make the typename clearer and avoid the conflict with "TBackendDispatcher".
- many cases: s/AlternateInspector${domainName}/Alternate${domainName}/
- one case: s/Inspector${domainName}BackendDispatcher/${domainName}BackendDispatcher/
Back over to you Brian!
Comment on attachment 246310 [details] Patch (fix Windows) View in context: https://bugs.webkit.org/attachment.cgi?id=246310&action=review r=me with the ObjC fixes. Careful with the bots (Windows still looks red) > Source/JavaScriptCore/inspector/ConsoleMessage.cpp:180 > -void ConsoleMessage::addToFrontend(InspectorConsoleFrontendDispatcher* consoleFrontendDispatcher, Inspector::InjectedScriptManager* injectedScriptManager, bool generatePreview) > + void ConsoleMessage::addToFrontend(ConsoleFrontendDispatcher* consoleFrontendDispatcher, InjectedScriptManager* injectedScriptManager, bool generatePreview) Style: Unexpected leading whitespace. > Source/JavaScriptCore/inspector/ConsoleMessage.cpp:233 > +void ConsoleMessage::updateRepeatCountInConsole(Inspector::ConsoleFrontendDispatcher* consoleFrontendDispatcher) Meh, I really don't think the Inspector::'s are needed anymore for FrontendDispatchers / BackendDispatchers. You removed them in some places (see above). But I won't belabor the point. > Source/JavaScriptCore/inspector/JSGlobalObjectInspectorController.cpp:105 > - ASSERT(!m_inspectorFrontendChannel); > - ASSERT(!m_inspectorBackendDispatcher); > + ASSERT(!m_frontendChannel); > + ASSERT(!m_backendDispatcher); I really like these member renames. 3 levels of "inspector" by this point. Inspector::InspectorController::m_inspector* > Source/JavaScriptCore/inspector/agents/InspectorAgent.h:43 > +class InspectorFrontendDispatchers; lolwhat? I think I typo'd this a long time ago. This doesn't sound real. Drop the bad forward declaration. Thanks for checking the ObjC stuff. I will land this once EWS runs clean. Created attachment 246603 [details]
For EWS
Attachment 246603 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:85: [CppBackendDispatcherHeaderGenerator._generate_alternate_handler_forward_declarations_for_domains] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5]
Total errors found: 1 in 100 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 246605 [details]
For EWS
Attachment 246605 [details] did not pass style-queue:
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:85: [CppBackendDispatcherHeaderGenerator._generate_alternate_handler_forward_declarations_for_domains] Instance of 'CppBackendDispatcherHeaderGenerator' has no 'wrap_with_guard_for_domain' member [pylint/E1101] [5]
Total errors found: 1 in 100 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r180116: <http://trac.webkit.org/changeset/180116> |