Bug 180367 - Web Inspector: modernize InjectedScript a bit
Summary: Web Inspector: modernize InjectedScript a bit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Brian Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-04 11:27 PST by Brian Burg
Modified: 2017-12-12 13:50 PST (History)
11 users (show)

See Also:


Attachments
Patch (39.18 KB, patch)
2017-12-04 13:41 PST, Brian Burg
timothy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Burg 2017-12-04 11:27:29 PST
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.
Comment 1 Brian Burg 2017-12-04 13:41:09 PST
Created attachment 328384 [details]
Patch
Comment 2 EWS Watchlist 2017-12-04 13:42:59 PST
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.
Comment 3 Brian Burg 2017-12-04 13:43:56 PST
(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.
Comment 4 Alex Christensen 2017-12-05 15:08:41 PST
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.
Comment 5 Timothy Hatcher 2017-12-05 22:39:20 PST
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.
Comment 6 Brian Burg 2017-12-08 14:06:17 PST
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.
Comment 7 Brian Burg 2017-12-12 13:49:17 PST
Committed r225804: <https://trac.webkit.org/changeset/225804>
Comment 8 Radar WebKit Bug Importer 2017-12-12 13:50:45 PST
<rdar://problem/36003463>