Bug 141372 - Web Inspector: remove some unnecessary Inspector prefixes from class names in Inspector namespace
Summary: Web Inspector: remove some unnecessary Inspector prefixes from class names in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks: 141608
  Show dependency treegraph
 
Reported: 2015-02-08 12:32 PST by Brian Burg
Modified: 2015-02-14 15:26 PST (History)
9 users (show)

See Also:


Attachments
Proposed Fix (335.10 KB, patch)
2015-02-08 12:55 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (335.75 KB, patch)
2015-02-09 08:07 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Proposed Fix .2 (290.22 KB, patch)
2015-02-09 14:27 PST, Brian Burg
no flags Details | Formatted Diff | Diff
Patch (fix Windows) (291.59 KB, patch)
2015-02-09 19:39 PST, Brian Burg
joepeck: review+
Details | Formatted Diff | Diff
[PATCH] ObjC Generation Changes (needs test rebaseline) (3.72 KB, patch)
2015-02-14 00:00 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
For EWS (301.18 KB, patch)
2015-02-14 12:07 PST, Brian Burg
no flags Details | Formatted Diff | Diff
For EWS (301.17 KB, patch)
2015-02-14 12:51 PST, Brian Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2015-02-08 12:32:12 PST
Keep the prefix for file names, as it provides useful information. Otherwise, convert to namespacing where needed.
Comment 1 Radar WebKit Bug Importer 2015-02-08 12:32:32 PST
<rdar://problem/19760554>
Comment 2 Brian Burg 2015-02-08 12:55:29 PST
Created attachment 246246 [details]
Proposed Fix
Comment 3 WebKit Commit Bot 2015-02-08 12:57:17 PST
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`)
Comment 4 WebKit Commit Bot 2015-02-08 12:57:35 PST
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.
Comment 5 Brian Burg 2015-02-09 08:07:07 PST
Created attachment 246273 [details]
Proposed Fix
Comment 6 WebKit Commit Bot 2015-02-09 08:08:14 PST
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.
Comment 7 Joseph Pecoraro 2015-02-09 12:26:21 PST
> /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 8 Joseph Pecoraro 2015-02-09 12:39:01 PST
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 9 Brian Burg 2015-02-09 14:08:22 PST
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.
Comment 10 Brian Burg 2015-02-09 14:27:00 PST
Created attachment 246290 [details]
Proposed Fix .2
Comment 11 WebKit Commit Bot 2015-02-09 14:28:50 PST
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.
Comment 12 Brian Burg 2015-02-09 19:39:27 PST
Created attachment 246310 [details]
Patch (fix Windows)
Comment 13 WebKit Commit Bot 2015-02-09 19:41:45 PST
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.
Comment 14 Joseph Pecoraro 2015-02-14 00:00:48 PST
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 15 Joseph Pecoraro 2015-02-14 00:08:50 PST
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.
Comment 16 Brian Burg 2015-02-14 09:38:29 PST
Thanks for checking the ObjC stuff. I will land this once EWS runs clean.
Comment 17 Brian Burg 2015-02-14 12:07:55 PST
Created attachment 246603 [details]
For EWS
Comment 18 WebKit Commit Bot 2015-02-14 12:09:33 PST
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.
Comment 19 Brian Burg 2015-02-14 12:51:03 PST
Created attachment 246605 [details]
For EWS
Comment 20 WebKit Commit Bot 2015-02-14 12:53:30 PST
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.
Comment 21 Brian Burg 2015-02-14 15:26:02 PST
Committed r180116: <http://trac.webkit.org/changeset/180116>