<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Inspector crashes when using the console"
href="https://bugs.webkit.org/show_bug.cgi?id=155745#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Inspector crashes when using the console"
href="https://bugs.webkit.org/show_bug.cgi?id=155745">bug 155745</a>
from <span class="vcard"><a class="email" href="mailto:bburg@apple.com" title="Brian Burg <bburg@apple.com>"> <span class="fn">Brian Burg</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=275093&action=diff" name="attach_275093" title="Patch">attachment 275093</a> <a href="attachment.cgi?id=275093&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=275093&action=review">https://bugs.webkit.org/attachment.cgi?id=275093&action=review</a>
Great work! Just a few naming suggestions, and let's wait for EWS.
It's weird that this crash happens on GTK but not Mac since we generate the same protocol groups.
In the longer term there won't be a need for client code (inspector Agents etc) to use this helper directly because the protocol bindings will do the type checking and conversions itself.
<span class="quote">> Source/JavaScriptCore/ChangeLog:19
> + getEnumConstantValue() generated and used foe every framework, so</span >
Nit: 'for'
<span class="quote">> Source/JavaScriptCore/ChangeLog:25
> + (CppGenerator.enums_namespace): Return the namespace name that</span >
Suggestion: If you are going to line wrap with this few columns, it's a lot easier to parse the changelog if you add newlines after each text block.
<span class="quote">> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:219
> + if (typeString == Inspector::Protocol::InspectorEnums::getEnumConstantValue(Inspector::Protocol::Debugger::BreakpointAction::Type::Log)) {</span >
Please change the namespace to XXXHelpers or XXXConversions. There are other helpers we may want to put in a disambiguated namespace.
<span class="quote">> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:53
> + def enums_namespace(self):</span >
As above, please rename this function to helper_namespace or similar.
<span class="quote">> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:76
> + sections.append('namespace %s {' % self.enums_namespace())</span >
I would recommend extracting this entire snippet into a private method, the top-level generation method is getting long.
<span class="quote">> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:89
> + sections.append('} // namespace %s' % self.enums_namespace())</span >
Please inline the namespace wrapping into self._generate_declarations_for_enum_conversion_methods() and do not emit it if there are no lines emitted into the namespace.
<span class="quote">> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:298
> + lines.append(' m_result->%(keyedSet)s(ASCIILiteral("%(name)s"), Inspector::Protocol::%(enumsNamespace)s::getEnumConstantValue(value));' % setter_args)</span >
I am surprised that removing the cast doesn't cause a compiler error.
<span class="quote">> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:70
> + sections.append('} // namespace %s' % self.enums_namespace())</span >
See comment above.
<span class="quote">> Source/JavaScriptCore/inspector/scripts/tests/expected/commands-with-async-attribute.json-result:814
> +</span >
This extra newline isn't needed.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>