Bug 141372 – Web Inspector: remove some unnecessary Inspector prefixes from class names in Inspector namespace

• Component: Web Inspector
• Status: RESOLVED FIXED

[Brian Burg]

Keep the prefix for file names, as it provides useful information. Otherwise, convert to namespacing where needed.

[Radar WebKit Bug Importer]

<rdar://problem/19760554>

[Brian Burg]

Created attachment 246246 [details]
Proposed Fix

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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.

[Brian Burg]

Created attachment 246273 [details]
Proposed Fix

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

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.

[Brian Burg]

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.

[Brian Burg]

Created attachment 246290 [details]
Proposed Fix .2

[WebKit Commit Bot]

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.

[Brian Burg]

Created attachment 246310 [details]
Patch (fix Windows)

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

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!

[Joseph Pecoraro]

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.

[Brian Burg]

Thanks for checking the ObjC stuff. I will land this once EWS runs clean.

[Brian Burg]

Created attachment 246603 [details]
For EWS

[WebKit Commit Bot]

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.

[Brian Burg]

Created attachment 246605 [details]
For EWS

[WebKit Commit Bot]

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.

[Brian Burg]

Committed r180116: <http://trac.webkit.org/changeset/180116>