[Webkit-unassigned] [Bug 155745] [GTK] Inspector crashes when using the console

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=155745

--- Comment #5 from Brian Burg <bburg at apple.com> ---
Comment on attachment 275093
  --> https://bugs.webkit.org/attachment.cgi?id=275093
Patch

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

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.

> Source/JavaScriptCore/ChangeLog:19
> +        getEnumConstantValue() generated and used foe every framework, so

Nit: 'for'

> Source/JavaScriptCore/ChangeLog:25
> +        (CppGenerator.enums_namespace): Return the namespace name that

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.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:219
> +    if (typeString == Inspector::Protocol::InspectorEnums::getEnumConstantValue(Inspector::Protocol::Debugger::BreakpointAction::Type::Log)) {

Please change the namespace to XXXHelpers or XXXConversions. There are other helpers we may want to put in a disambiguated namespace.

> Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:53
> +    def enums_namespace(self):

As above, please rename this function to helper_namespace or similar.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:76
> +        sections.append('namespace %s {' % self.enums_namespace())

I would recommend extracting this entire snippet into a private method, the top-level generation method is getting long.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:89
> +        sections.append('} // namespace %s' % self.enums_namespace())

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.

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

I am surprised that removing the cast doesn't cause a compiler error.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:70
> +        sections.append('} // namespace %s' % self.enums_namespace())

See comment above.

> Source/JavaScriptCore/inspector/scripts/tests/expected/commands-with-async-attribute.json-result:814
> +

This extra newline isn't needed.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160329/12ac2571/attachment-0001.html>