<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span> changed
<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>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">CC</td>
<td>
</td>
<td>cgarcia@igalia.com
</td>
</tr></table>
<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#c7">Comment # 7</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:cgarcia@igalia.com" title="Carlos Garcia Campos <cgarcia@igalia.com>"> <span class="fn">Carlos Garcia Campos</span></a>
</span></b>
<pre>(In reply to <a href="show_bug.cgi?id=155745#c5">comment #5</a>)
<span class="quote">> 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.</span >
Thanks for the review.
<span class="quote">> It's weird that this crash happens on GTK but not Mac since we generate the
> same protocol groups.</span >
I was also surprised, my guess is that for some reason in Mac you always end up using the JSC table that is bigger.
<span class="quote">> 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'</span >
Oops.
<span class="quote">> > 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.</span >
It's emacs default :-) I'll try to make it easier to read either adding new lines or configuring emacs to use longer lines.
<span class="quote">> > 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.</span >
Helpers is more generic, just in case we end up adding something that is not a conversor.
<span class="quote">> > Source/JavaScriptCore/inspector/scripts/codegen/cpp_generator.py:53
> > + def enums_namespace(self):
>
> As above, please rename this function to helper_namespace or similar.</span >
Sure.
<span class="quote">> > 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.</span >
Ok.
<span class="quote">> > 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.</span >
Ok.
<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)
>
> I am surprised that removing the cast doesn't cause a compiler error.</span >
We have a template function that makes the cast already.
<span class="quote">> > 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.</span >
That was --reset-results I haven't changed tests results manually.</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>