[webkit-reviews] review granted: [Bug 216302] Web Inspector: modernize generated backend protocol code : [Attachment 408318] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has granted Devin Rousso <drousso at apple.com>'s
request for review:
Bug 216302: Web Inspector: modernize generated backend protocol code
https://bugs.webkit.org/show_bug.cgi?id=216302

Attachment 408318: Patch

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




--- Comment #11 from Brian Burg <bburg at apple.com> ---
Comment on attachment 408318
  --> https://bugs.webkit.org/attachment.cgi?id=408318
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>(ins
trumentString);

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