Bug 180367 – Web Inspector: modernize InjectedScript a bit

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

This code uses all kinds of gross out parameters. Start cleaning this mess up.

Eventually we should use Expected<T, ErrorString> as the return type for many of the simple return values here that currently use out-params. This probably won't work for generated commands, but those could use std::optional<ErrorString> and drop the first parameter.

[Blaze Burg]

Created attachment 328384 [details]
Patch

[EWS Watchlist]

Attachment 328384 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:115:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:115:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:142:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:239:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:96:  out_resultObject is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:96:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:96:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:830:  out_wasThrown is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
ERROR: Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:830:  out_savedResultIndex is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 9 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

(In reply to Build Bot from comment #2)
> Attachment 328384 [details] did not pass style-queue:
> 
> 
> ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:115:
> out_wasThrown is incorrectly named. Don't use underscores in your identifier
> names.  [readability/naming/underscores] [4]
> ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:115:
> out_savedResultIndex is incorrectly named. Don't use underscores in your
> identifier names.  [readability/naming/underscores] [4]
> ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:142:
> out_wasThrown is incorrectly named. Don't use underscores in your identifier
> names.  [readability/naming/underscores] [4]
> ERROR: Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:239:
> out_savedResultIndex is incorrectly named. Don't use underscores in your
> identifier names.  [readability/naming/underscores] [4]
> ERROR: Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:96: 
> out_resultObject is incorrectly named. Don't use underscores in your
> identifier names.  [readability/naming/underscores] [4]
> ERROR: Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:96: 
> out_wasThrown is incorrectly named. Don't use underscores in your identifier
> names.  [readability/naming/underscores] [4]
> ERROR: Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:96: 
> out_savedResultIndex is incorrectly named. Don't use underscores in your
> identifier names.  [readability/naming/underscores] [4]
> ERROR:
> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:830: 
> out_wasThrown is incorrectly named. Don't use underscores in your identifier
> names.  [readability/naming/underscores] [4]
> ERROR:
> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:830: 
> out_savedResultIndex is incorrectly named. Don't use underscores in your
> identifier names.  [readability/naming/underscores] [4]
> Total errors found: 9 in 8 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

Style bot can go jump in a lake. The code reads much, much better with the out_ prefix.

[Alex Christensen]

Comment on attachment 328384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328384&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        If there is only one out-parameter and a void return type, then return the value.

I think we should return a std::pair or a std::tuple instead of having out_-prefixed variable names.

[Timothy Hatcher]

Comment on attachment 328384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328384&action=review

> Source/JavaScriptCore/inspector/InjectedScriptBase.cpp:128
> +    out_resultObject = BindingTraits<Protocol::Runtime::RemoteObject>::runtimeCast(resultObject);

Is "out_" prefix the style we use elsewhere? No a fan.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:127
> +    // FIXME: remove this bridging code when generated protocol commands no longer use OptOutput<T>.

Is there a bug you can reference?

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:256
> +    // FIXME: remove this bridging code when generated protocol commands no longer use OptOutput<T>.

Ditto.

[Blaze Burg]

Comment on attachment 328384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=328384&action=review

>> Source/JavaScriptCore/ChangeLog:10
>> +        If there is only one out-parameter and a void return type, then return the value.
> 
> I think we should return a std::pair or a std::tuple instead of having out_-prefixed variable names.

This is the longer term goal for where it makes sense, along with using Expected<T, ErrorString> rather than ErrorString as a in-out param everywhere. But my first-order goal is to remove OptOutput and use std::optional in generated signatures.

I'm not completely sold on using Tuples instead of out-params in generated protocol code, as it will make it a lot more annoying to use for little gain.

>> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:127
>> +    // FIXME: remove this bridging code when generated protocol commands no longer use OptOutput<T>.
> 
> Is there a bug you can reference?

I'll add one. It's the next thing I want to clean up.

[Blaze Burg]

Committed r225804: <https://trac.webkit.org/changeset/225804>

[Radar WebKit Bug Importer]

<rdar://problem/36003463>