[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

Carlos Garcia Campos <cgarcia at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |cgarcia at igalia.com

--- Comment #7 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to comment #5)
> Comment on attachment 275093 [details]
> 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.

Thanks for the review.

> It's weird that this crash happens on GTK but not Mac since we generate the
> same protocol groups.

I was also surprised, my guess is that for some reason in Mac you always end up using the JSC table that is bigger.

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

Oops.

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

It's emacs default :-) I'll try to make it easier to read either adding new lines or configuring emacs to use longer lines.

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

Helpers is more generic, just in case we end up adding something that is not a conversor.

> > Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:53
> > +    def enums_namespace(self):
> 
> As above, please rename this function to helper_namespace or similar.

Sure.

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

Ok.

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

Ok.

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

We have a template function that makes the cast already.

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

That was --reset-results I haven't changed tests results manually.

-- 
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/20160330/b98e52f6/attachment-0001.html>