Web Inspector: modernize generated backend protocol code

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

- use `Optional&&` and `RefPtr&&` instead of `*`
 - use `Ref&&` instead of `&`
 - parse enum values and pass the enum directly instead of the raw string
 - don't use out parameters

[Devin Rousso]

Created attachment 408308 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/68547649>

[EWS Watchlist]

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

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 408310 [details]
Patch

[Devin Rousso]

Created attachment 408313 [details]
Patch

[Devin Rousso]

Created attachment 408314 [details]
Patch

[Devin Rousso]

Created attachment 408315 [details]
Patch

[Devin Rousso]

Created attachment 408316 [details]
Patch

[Devin Rousso]

Created attachment 408318 [details]
Patch

[BJ Burg]

Comment on attachment 408318 [details]
Patch

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

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.cpp:279
> +    T result;

Very nice. But, why is object passed by pointer instead of reference? The error message for !object seems wrong, that case is about the 'params' object or whatever being empty. It would be better to handle that case at callers of this, if its not too much extra pain.

> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.cpp:304
> +    return getPropertyValue<Optional<bool>>(object, name, required, &JSON::Value::asBoolean, "Boolean");

Uh.. yeah.. this whole area is a mess.

> Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:38
> +using ErrorString = String;

I've been wondering if we want to make this flexible enough to return more than just a string in the future. Seems ok for now.

> Source/JavaScriptCore/inspector/InspectorProtocolTypes.h:59
> +        return static_reference_cast<JSON::ArrayOf<T>>(static_reference_cast<JSON::ArrayBase>(array.releaseNonNull()));

Didn't know this was a thing!

> Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp:151
> +        m_scriptProfilerAgent->stopTracking();

It would be prudent to at least log any errors returned by the agent calls that happen here.

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:62
> +    Protocol::ErrorString errorString;

Nit: move this next to the first assignment.

> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.h:66
> +    virtual InjectedScript injectedScriptForEval(Protocol::ErrorString&, Optional<Protocol::Runtime::ExecutionContextId>&&) = 0;

Seems like InjectedScript needs some modernization, too. Can you file & relate bugs for followup modernizations?

> Source/JavaScriptCore/inspector/agents/InspectorConsoleAgent.cpp:200
> +    auto [timestamp, snapshotData] = WTFMove(result.value());

Neat.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:138
> +        ignoreCount = options->getInteger(Protocol::Debugger::BreakpointOptions::ignoreCountKey).valueOr(0);

Nice!

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:514
> +static bool parseLocation(Protocol::ErrorString& errorString, const JSON::Object& location, JSC::SourceID& sourceID, unsigned& lineNumber, unsigned& columnNumber)

Followup: more modernization possible.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:544
> +    auto protocolBreakpoint = ProtocolBreakpoint::fromPayload(errorString, !!url ? url : urlRegex, !!urlRegex, lineNumber, columnNumber.valueOr(0), WTFMove(options));

Followup: more modernization possible.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:573
> +    return { { protocolBreakpoint->id(), WTFMove(locations) } };

Nice.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:145
> +    virtual void internalDisable(bool isBeingDestroyed);

OK

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:59
> +    disable();

MARK: I reviewed from the top to here. (9/9/2020 @ 9:15am)

> Source/WebCore/inspector/agents/worker/WorkerAuditAgent.cpp:-51
> -InjectedScript WorkerAuditAgent::injectedScriptForEval(ErrorString& errorString, const int* executionContextId)

MARK: I reviewed from here to the bottom. (9/9/2020 @ 9:15am)

> Source/WebDriver/Session.cpp:1048
> +        if (!inViewCenterPointX || !inViewCenterPointY) {

So much nicer!

> Source/WebKit/UIProcess/Automation/Automation.json:32
>              "description": "The coordinate system in which rects, points, and sizes are to be interpreted.",

Nit: document that 'LayoutViewport' is legacy

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:-386
> -    Optional<float> x;

Why did this change from float to double? (Not a problem just curious)

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:430
> +static Inspector::Protocol::Automation::PageLoadStrategy pageLoadStrategyFromParameter(Optional<Inspector::Protocol::Automation::PageLoadStrategy>&& optionalPageLoadStrategy)

Nit: I think this function can be removed and the default value inlined.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1013
> +    case Inspector::Protocol::Automation::CoordinateSystem::Viewport:

Nit: add link to followup bug to remove 'LayoutViewport' if you plan to rename the wire value on the client side (safaridriver/webkitgtkdriver).

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:-1675
> -        if (!parsedInteraction)

Ayy, nice to hoist the parsing to earlier.

> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1914
> +            if (!!pressedCharKeyString)

Is this actually necessary? If pressedCharKeyString is Optional<String> then operator bool() defined on Optional should just work, right?

> Tools/TestWebKitAPI/Tests/WTF/JSONValue.cpp:63
> +        // Integers can be get as doubles.

Nit: grammaro

> Tools/TestWebKitAPI/Tests/WTF/JSONValue.cpp:82
> +        // Doubles can be get as integers.

Nit: grammaro

[BJ Burg]

Comment on attachment 408318 [details]
Patch

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

r=me, fantastic stamina. Please make remaining bots happy and be around to fix problems when this lands.

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:-53
> -{

o_O

> Source/JavaScriptCore/inspector/agents/InspectorRuntimeAgent.cpp:345
> +

Nit: extra newline

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:68
> +            # fallthrough

Nit. # Fallthrough.

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:74
> +        if isinstance(_type, ObjectType) or _type.qualified_name() in ['object']:

What's the case where it's not ObjectType but the qualified_name() is 'object'?

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:119
> +            # fallthrough

Ditto.

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:124
> +            # fallthrough

Ditto (more below)

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:254
> +            # fallthroug

Nit: typo

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:256
> +        if isinstance(_type, EnumType) and _type.is_anonymous:

Can you file a followup bug about modernizing / removing anonymous enums?

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:349
> +    def should_move_argument(_type, is_optional):

Nice, this is cleaner.

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator_templates.py:98
> +    void dispatch(long protocol_callId, const String& protocol_method, Ref<JSON::Object>&& protocol_message) final;

Nit: I prefer protocol_requestId.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_header.py:212
> +        return self.wrap_with_guard_for_condition(command.condition, "    void %s(long protocl_callId, RefPtr<JSON::Object>&& protocol_parameters);" % command.command_name)

Nit: typo

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_implementation.py:185
> +            elif CppGenerator.should_release_argument(_type, parameter.is_optional):

V. nice

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_implementation.py:263
> +                    parameter_expression = 'WTFMove(%s)' % variable_name

This code should use the helpers that you created (should_move, should_dereference, etc.)

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_implementation.py:369
> +                lines.append('    auto [%s] = WTFMove(result.value());' % ", ".join(result_extraction_names))

Suggestion: destructured_tuple_names?

> Source/WebCore/inspector/InspectorCanvas.cpp:913
>      initialStatePayload->setContent(getCanvasContentAsDataURL(ignored));

Another followup to modernize getCanvasContentAsDataURL()...

> Source/WebCore/inspector/InspectorStyleSheet.cpp:537
> +        if (auto range = buildSourceRangeObject(sourceData->ruleBodyRange, m_parentStyleSheet->lineEndings()))

Do we expect this to ever fail?

> Source/WebCore/inspector/InspectorStyleSheet.cpp:696
> +                        auto newStatus = activeIt->value->getString(Protocol::CSS::CSSProperty::statusKey);

This section was tricky to follow, but I think it's correct.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1179
> +    auto result = inspectorStyle->buildObjectForStyle();

NIt: extra newline

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:495
>          if (!includePseudo || *includePseudo) {

I think this should be includePseudo.value_or(true)

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:511
>          if (!includeInherited || *includeInherited) {

Ditto above.

> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:1040
> +void InspectorCSSAgent::didRemoveDOMNode(Node& node, Protocol::DOM::NodeId nodeId)

Nice.

> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:192
>      if (!inspectorCanvas)

Another modernization followup for assertInspectorCanvas() and friends.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:323
>      setSearchingForNode(ignored, false, nullptr, false);

Another modernization followup for setSearchingForNode().

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:703
> +    Element* element = assertEditableElement(errorString, nodeId);

More modernization followup for assertEditableElement() .

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:707
> +    if (!m_domEditor->setAttribute(*element, name, value, errorString))

More modernization followup for setAttribute().

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2017
> +                    if (auto controlledElementId = pushNodePathToFrontend(controlledElement))

Slightly different behavior, but its OK.

> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2754
> +        if (auto nodeId = pushNodePathToFrontend(errorString, node))

Modernization followup: pushNodePathToFrontend()

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:151
> +        if (!setAnimationFrameBreakpoint(errorString, WTFMove(breakpoint)))

Modernization followup: setAnimationFrameBreakpoint & friends.

> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:328
> +        m_pauseOnAllURLsBreakpoint = WTFMove(breakpoint);

Nit: newline

> Source/WebCore/inspector/agents/InspectorDOMStorageAgent.cpp:101
> +    RefPtr<StorageArea> storageArea = findStorageArea(errorString, WTFMove(storageId), frame);

Modernization followup: findStorageArea() and friends.

> Source/WebCore/inspector/agents/InspectorIndexedDBAgent.cpp:-283
> -    static NeverDestroyed<const String> numberType(MAKE_STATIC_STRING_IMPL("number"));

Eww

> Source/WebCore/inspector/agents/InspectorIndexedDBAgent.cpp:398
> +        if (!primaryKey)

Thanks for adding error handling!

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:210
>  

Nit: extra newline

> Source/WebCore/inspector/agents/InspectorPageAgent.cpp:404
> -    if (reloadFromOrigin)
> +    if (ignoreCache && *ignoreCache)

Use value_or(false)? In general I don't like the foo && *foo pattern. (and its cousins like !foo || !*foo)

> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:168
> +        auto instrument = Protocol::Helpers::parseEnumValueFromString<Protocol::Timeline::Instrument>(instrumentString);

Should we auto-parse/convert arrays of enums? (Not necessarily right now)

> Source/WebCore/inspector/agents/page/PageNetworkAgent.cpp:94
> +void PageNetworkAgent::setResourceCachingDisabledInteral(bool disabled)

ERROR: speling

[Devin Rousso]

Comment on attachment 408318 [details]
Patch

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

>> Source/JavaScriptCore/inspector/InspectorBackendDispatcher.cpp:279
>> +    T result;
> 
> Very nice. But, why is object passed by pointer instead of reference? The error message for !object seems wrong, that case is about the 'params' object or whatever being empty. It would be better to handle that case at callers of this, if its not too much extra pain.

In this case, `object` actually is the `params` object, which can be `nullptr` if the JSON message is malformed 😅

Doing this in the caller would mean that every `*BackendDispatcher::*` would need to check for whether `params` exists or not and throw an error if that command has a required property.  IMO, this way is much cleaner and avoids duplicated code.

I'll rename `object` to `params` for clarity.

>> Source/JavaScriptCore/inspector/JSGlobalObjectConsoleClient.cpp:151
>> +        m_scriptProfilerAgent->stopTracking();
> 
> It would be prudent to at least log any errors returned by the agent calls that happen here.

Log to where?  Most of the `stop*`/`disable*` commands tend to be very lenient and not result in errors as they are sorta "overused" in order to make sure things really are turned off.  I think logging errors would likely be misleading most of the time.

>> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.cpp:62
>> +    Protocol::ErrorString errorString;
> 
> Nit: move this next to the first assignment.

I intentionally put these as the first thing in the command handler so that it was kinda "obvious" that we need to remove it.  This would relate to followup(s) for removing `Protocol::ErrorString` as out-parameters in helper methods too.

>> Source/JavaScriptCore/inspector/agents/InspectorAuditAgent.h:66
>> +    virtual InjectedScript injectedScriptForEval(Protocol::ErrorString&, Optional<Protocol::Runtime::ExecutionContextId>&&) = 0;
> 
> Seems like InjectedScript needs some modernization, too. Can you file & relate bugs for followup modernizations?

<https://webkit.org/b/216341> Web Inspector: modernize InjectedScript code

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:514
>> +static bool parseLocation(Protocol::ErrorString& errorString, const JSON::Object& location, JSC::SourceID& sourceID, unsigned& lineNumber, unsigned& columnNumber)
> 
> Followup: more modernization possible.

<https://webkit.org/b/216342> Web Inspector: modernize *DebuggerAgent code

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:544
>> +    auto protocolBreakpoint = ProtocolBreakpoint::fromPayload(errorString, !!url ? url : urlRegex, !!urlRegex, lineNumber, columnNumber.valueOr(0), WTFMove(options));
> 
> Followup: more modernization possible.

<https://webkit.org/b/216342> Web Inspector: modernize *DebuggerAgent code

>> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:145
>> +    virtual void internalDisable(bool isBeingDestroyed);
> 
> OK

Yeah this is perhaps one of the only downsides to this change, in that we can no longer have other methods that share the same name as commands if they have the same arguments.  IMO that's not really a downside tho, as I personally believe there should be 1-1 mappings for commands :)

>> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:74
>> +        if isinstance(_type, ObjectType) or _type.qualified_name() in ['object']:
> 
> What's the case where it's not ObjectType but the qualified_name() is 'object'?

I believe that inlined/anonymous objects are a `PrimitiveType` and have a `qualified_name` of `'object'` (e.g. the `data` parameter of the `Debugger.paused` event).

>> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:256
>> +        if isinstance(_type, EnumType) and _type.is_anonymous:
> 
> Can you file a followup bug about modernizing / removing anonymous enums?

<https://webkit.org/b/216343> Web Inspector: remove anonymous enums from protocol

>> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_backend_dispatcher_implementation.py:263
>> +                    parameter_expression = 'WTFMove(%s)' % variable_name
> 
> This code should use the helpers that you created (should_move, should_dereference, etc.)

I remember trying to also use it for this code too, but not getting it to work right.  The rest of the code that uses those helpers deals with the "final" type of those objects, whereas this code is about actually generating those final types (e.g. in every other case, we always want to `releaseNonNull` an optional object/array, but in this case we just want to `WTFMove` it).  This is because the `BackendDispatcher::get*` always returns an "optional" value (e.g. always `RefPtr` never `Ref`) even for properties that we expect to exist.

>> Source/WebCore/inspector/InspectorCanvas.cpp:913
>>      initialStatePayload->setContent(getCanvasContentAsDataURL(ignored));
> 
> Another followup to modernize getCanvasContentAsDataURL()...

<https://webkit.org/b/216344> Web Inspector: modernize *CanvasAgent code

>> Source/WebCore/inspector/InspectorStyleSheet.cpp:537
>> +        if (auto range = buildSourceRangeObject(sourceData->ruleBodyRange, m_parentStyleSheet->lineEndings()))
> 
> Do we expect this to ever fail?

There's an early `return nullptr;` in that function, so I guess yes?  This was fine before because `set*` allowed for `RefPtr` and (I believe) could handle invalid values.  Now that we expect/require valid values, we should check it just to be sure.

>> Source/WebCore/inspector/InspectorStyleSheet.cpp:696
>> +                        auto newStatus = activeIt->value->getString(Protocol::CSS::CSSProperty::statusKey);
> 
> This section was tricky to follow, but I think it's correct.

this entire file is tricky to follow T.T

>> Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:495
>>          if (!includePseudo || *includePseudo) {
> 
> I think this should be includePseudo.value_or(true)

i personally don't like using `valueOr` cause it's more work than the separate operations

>> Source/WebCore/inspector/agents/InspectorCanvasAgent.cpp:192
>>      if (!inspectorCanvas)
> 
> Another modernization followup for assertInspectorCanvas() and friends.

<https://webkit.org/b/216344> Web Inspector: modernize *CanvasAgent code

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:323
>>      setSearchingForNode(ignored, false, nullptr, false);
> 
> Another modernization followup for setSearchingForNode().

<https://webkit.org/b/216345> Web Inspector: modernize *DOMAgent code and related helper classes

FYI this would probably fall under my existing desire to just rebuild the DOM agent
 - <https://webkit.org/b/189687> Web Inspector: preserve DOM.NodeId if a node is removed and re-added
 - <https://webkit.org/b/213499> Web Inspector: allow DOM nodes to be instrumented at any point, regardless of whether the main document has also been instrumented

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:703
>> +    Element* element = assertEditableElement(errorString, nodeId);
> 
> More modernization followup for assertEditableElement() .

ditto

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:707
>> +    if (!m_domEditor->setAttribute(*element, name, value, errorString))
> 
> More modernization followup for setAttribute().

ditto

>> Source/WebCore/inspector/agents/InspectorDOMAgent.cpp:2754
>> +        if (auto nodeId = pushNodePathToFrontend(errorString, node))
> 
> Modernization followup: pushNodePathToFrontend()

ditto

>> Source/WebCore/inspector/agents/InspectorDOMDebuggerAgent.cpp:151
>> +        if (!setAnimationFrameBreakpoint(errorString, WTFMove(breakpoint)))
> 
> Modernization followup: setAnimationFrameBreakpoint & friends.

<https://webkit.org/b/216346> Web Inspector: modernize *DOMDebuggerAgent code

>> Source/WebCore/inspector/agents/InspectorDOMStorageAgent.cpp:101
>> +    RefPtr<StorageArea> storageArea = findStorageArea(errorString, WTFMove(storageId), frame);
> 
> Modernization followup: findStorageArea() and friends.

<https://webkit.org/b/216347> Web Inspector: modernize *DOMStorageAgent code

>> Source/WebCore/inspector/agents/InspectorTimelineAgent.cpp:168
>> +        auto instrument = Protocol::Helpers::parseEnumValueFromString<Protocol::Timeline::Instrument>(instrumentString);
> 
> Should we auto-parse/convert arrays of enums? (Not necessarily right now)

yes, we should really be able to auto-parse/auto-convert all the things

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:-386
>> -    Optional<float> x;
> 
> Why did this change from float to double? (Not a problem just curious)

there are no longer template overloads for `getNumber`, so the value must either be an `int` or a `double`

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1013
>> +    case Inspector::Protocol::Automation::CoordinateSystem::Viewport:
> 
> Nit: add link to followup bug to remove 'LayoutViewport' if you plan to rename the wire value on the client side (safaridriver/webkitgtkdriver).

<https://webkit.org/b/216348> remove code handling incorrectly named `Automation.CoordinateSystem.LayoutViewport` in favor of the correctly named `Automation.CoordinateSystem.Viewport`

>> Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1914
>> +            if (!!pressedCharKeyString)
> 
> Is this actually necessary? If pressedCharKeyString is Optional<String> then operator bool() defined on Optional should just work, right?

So `getString` doesn't return `Optional<String>` as `String` is already a tri-state (null vs empty vs value), meaning that we can use just `String` and know whether it exists or not.

Unfortunately, while `String` doesn't have `operator bool()`, it does have `operator!`, so I'm using that instead.  I could also use `!string.isNull()` if that'd be preferred.

[Devin Rousso]

Created attachment 408416 [details]
Patch

[Devin Rousso]

Comment on attachment 408416 [details]
Patch

I'm pretty sure the win bot failure is not accurate as it's complaining about code that should've changed in this patch.

e.g.
```
C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\DerivedSources\ForwardingHeaders\JavaScriptCore\InspectorProtocolObjects.h(487,1): error C2664: 'void WTF::JSONImpl::ObjectBase::setObject(const WTF::String &,WTF::Ref<WTF::JSONImpl::ObjectBase,WTF::DumbPtrTraits<T>> &&)': cannot convert argument 2 from 'WTF::RefPtr<Inspector::Protocol::Animation::Effect,WTF::DumbPtrTraits<T>>' to 'WTF::Ref<WTF::JSONImpl::ObjectBase,WTF::DumbPtrTraits<T>> &&' [C:\cygwin\home\buildbot\worker\Windows-EWS\build\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
```
is complaining about the argument being a `RefPtr<Inspector::Protocol::Animation::Effect>`, which in this case is the argument to `Inspector::Protocol::Animation::Animation::setEffect`, but this patch changes that argument to be a `Ref` (the code is autogenerated)

The win bot seems to have issues when changes are made to autogenerated inspector code :(

[Devin Rousso]

also, it's worth mentioning that attachment 408318 [details] didn't have any issues, and attachment 408416 [details] didn't change anything near the code that seems to be causing problems

[EWS]

Committed r266885: <https://trac.webkit.org/changeset/266885>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408416 [details].

[Fujii Hironori]

Committed r266889: <https://trac.webkit.org/changeset/266889>