Bug 155745 – REGRESSION(r198792): [GTK] Inspector crashes in Inspector::Protocol::getEnumConstantValue since r198792

• Component: Web Inspector
• Status: RESOLVED FIXED

[Alejandro G. Castro]

Just open the inspector after applying patch in bug 155497 and try to execute any command in the console, this is the backtrace I get:

Program received signal SIGSEGV, Segmentation fault.
strlen () at ../sysdeps/x86_64/strlen.S:106
106		movdqu	(%rax), %xmm12
(gdb) bt
#0  strlen () at ../sysdeps/x86_64/strlen.S:106
#1  0x00007fe403bce3c9 in WTF::StringImpl::create(unsigned char const*) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#2  0x00007fe403bdc12c in WTF::String::String(unsigned char const*) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#3  0x00007fe4052074c7 in Inspector::Protocol::getEnumConstantValue(int) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#4  0x00007fe403b90f46 in Inspector::RuntimeBackendDispatcher::parse(long, WTF::RefPtr<Inspector::InspectorObject>&&) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#5  0x00007fe403b83c90 in Inspector::RuntimeBackendDispatcher::dispatch(long, WTF::String const&, WTF::Ref<Inspector::InspectorObject>&&) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#6  0x00007fe40375da18 in Inspector::BackendDispatcher::dispatch(WTF::String const&) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#7  0x00007fe40523ea02 in WebKit::WebInspector::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#8  0x00007fe404f6ae46 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::MessageDecoder, std::default_delete<IPC::MessageDecoder> >) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#9  0x00007fe404f6b873 in IPC::Connection::dispatchOneMessage() () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#10 0x00007fe403bbb25f in WTF::RunLoop::performWork() () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#11 0x00007fe403bea289 in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#12 0x00007fe3fe4d181a in g_main_dispatch (context=0x1bfcf20) at /home/alex/checkout/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3122
#13 g_main_context_dispatch (context=context@entry=0x1bfcf20) at /home/alex/checkout/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3737
#14 0x00007fe3fe4d1b98 in g_main_context_iterate (context=0x1bfcf20, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at /home/alex/checkout/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:3808
#15 0x00007fe3fe4d1eb2 in g_main_loop_run (loop=0x22d0ce0) at /home/alex/checkout/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.44.1/glib/gmain.c:4002
#16 0x00007fe403beaab0 in WTF::RunLoop::run() () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libjavascriptcoregtk-4.0.so.18
#17 0x00007fe405200e72 in int WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain>(int, char**) () from /home/alex/checkout/WebKit/WebKitBuild/Release/lib/libwebkit2gtk-4.0.so.37
#18 0x00007fe3f9703580 in __libc_start_main (main=0x400b40 <main>, argc=2, argv=0x7ffd373b4f48, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffd373b4f38) at libc-start.c:289

[Radar WebKit Bug Importer]

<rdar://problem/25289456>

[Carlos Garcia Campos]

Created attachment 275093 [details]
Patch

[WebKit Commit Bot]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[WebKit Commit Bot]

Attachment 275093 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:76:  [CppProtocolTypesHeaderGenerator.generate_output] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:83:  [CppProtocolTypesHeaderGenerator.generate_output] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:87:  [CppProtocolTypesHeaderGenerator.generate_output] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:89:  [CppProtocolTypesHeaderGenerator.generate_output] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:288:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:311:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'enums_namespace' member  [pylint/E1101] [5]
Total errors found: 6 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Blaze Burg]

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.

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.

[Alejandro G. Castro]

Comment on attachment 275093 [details]
Patch

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

Just tested it, it works for me, thanks. Apparently the linker was not sorting out the situation with 2 definitions of a symbol very well, maybe in case of Apple their linker was smarter or just luckier.

> Source/JavaScriptCore/ChangeLog:15
> +        always end up using the one in WebKit2. Since the

Did you mean JavaScriptCore here?

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

The function with T type used that does the cast to int is defined up in the file.

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

(In reply to comment #6)
> Comment on attachment 275093 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275093&action=review
> 
> Just tested it, it works for me, thanks. Apparently the linker was not
> sorting out the situation with 2 definitions of a symbol very well, maybe in
> case of Apple their linker was smarter or just luckier.
> 
> > Source/JavaScriptCore/ChangeLog:15
> > +        always end up using the one in WebKit2. Since the
> 
> Did you mean JavaScriptCore here?

No, I mean the WebKit2 one which is the small one.

[Carlos Garcia Campos]

(In reply to comment #7)
> > > 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.

I see what you mean now. While it's quite obvious when looking at the generated code, it's not that easy from the generator code, since we don't know if we will have something else after that or another namespace closed.

[Carlos Garcia Campos]

Created attachment 275188 [details]
Updated patch

I think I've addressed all review comments.

[WebKit Commit Bot]

Attachment 275188 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:153:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:158:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:163:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:172:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:295:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:318:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:373:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods.return_type_with_export_macro] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:413:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:418:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:78:  [CppProtocolTypesImplementationGenerator._generate_enum_mapping] Instance of 'CppProtocolTypesImplementationGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
Total errors found: 10 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Created attachment 275189 [details]
Try to fix mac builds

[WebKit Commit Bot]

Attachment 275189 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:153:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:158:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:163:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:172:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:295:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:318:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:373:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods.return_type_with_export_macro] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:413:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:418:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:78:  [CppProtocolTypesImplementationGenerator._generate_enum_mapping] Instance of 'CppProtocolTypesImplementationGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
Total errors found: 10 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

*** Bug 156053 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Created attachment 275268 [details]
Rebased patch to fix the build

Rebase needed after r198872

[WebKit Commit Bot]

Attachment 275268 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:153:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:158:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:163:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:172:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:295:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:318:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:373:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods.return_type_with_export_macro] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:413:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:418:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:78:  [CppProtocolTypesImplementationGenerator._generate_enum_mapping] Instance of 'CppProtocolTypesImplementationGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
Total errors found: 10 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Alberto Lopez Perez]

Reviewers? ping?

This is making the inspector crash like mad in the GTK+ port.

[Carlos Alberto Lopez Perez]

I'm trying to build this on top of r198945 and I'm getting build errors: http://sprunge.us/MdXG


I guess some revision between the upload and r198945 broke the build for this patch.

[Blaze Burg]

Comment on attachment 275268 [details]
Rebased patch to fix the build

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

r=me

Just some style issues remain. Please post an updated patch so we can set cq+ on it.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:173
> +        return ['\n\n'.join(lines)]

The pattern used in these generators is that lines are joined by one newline, and sections are joined by two newlines. So you could just return the array here and the caller will join it the same way.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:412
> +        sections.insert(0, '\n'.join([

Why do we need to use insert here? The generator's sections should be in the same order as the generated sections.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:154
> +        sections.insert(0, 'namespace %s {' % self.helpers_namespace())

Instead of using insert and causing the generator and generated file to have different line order, please use a guard at the top like 'if not self.assigned_enum_values()' and return [] before generating sections.

> Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:156
> +        return ['\n\n'.join(sections)]

See above comment, should not be necessary to join sections here if the caller can extend with this method's result.

[Blaze Burg]

(In reply to comment #19)
> Comment on attachment 275268 [details]
> Rebased patch to fix the build
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275268&action=review
> 
> r=me
> 
> Just some style issues remain. Please post an updated patch so we can set
> cq+ on it.
> 

The reason I ask is that we have been landing a lot of code in the WK2 parts lately, so this is unlikely to build anymore as-is.

[Carlos Garcia Campos]

Created attachment 275494 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 275494 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:416:  at least two spaces before inline comment  [pep8/E261] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:153:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:158:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:163:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:172:  [CppProtocolTypesHeaderGenerator._generate_enum_constant_value_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:295:  [CppProtocolTypesHeaderGenerator._generate_builder_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:318:  [CppProtocolTypesHeaderGenerator._generate_unchecked_setter_for_member] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:370:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:379:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods.return_type_with_export_macro] Instance of 'CppProtocolTypesHeaderGenerator' has no 'model' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_header.py:418:  [CppProtocolTypesHeaderGenerator._generate_declarations_for_enum_conversion_methods] Instance of 'CppProtocolTypesHeaderGenerator' has no 'helpers_namespace' member  [pylint/E1101] [5]
ERROR: Source/JavaScriptCore/inspector/scripts/codegen/generate_cpp_protocol_types_implementation.py:78:  [CppProtocolTypesImplementationGenerator._generate_enum_mapping] Instance of 'CppProtocolTypesImplementationGenerator' has no 'assigned_enum_values' member  [pylint/E1101] [5]
Total errors found: 11 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 275494 [details]
Patch for landing

Clearing flags on attachment: 275494

Committed r199005: <http://trac.webkit.org/changeset/199005>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #23)
> Comment on attachment 275494 [details]
> Patch for landing
> 
> Clearing flags on attachment: 275494
> 
> Committed r199005: <http://trac.webkit.org/changeset/199005>

It broke the WinCairo build: https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/55657

[Csaba Osztrogonác]

(In reply to comment #25)
> (In reply to comment #23)
> > Comment on attachment 275494 [details]
> > Patch for landing
> > 
> > Clearing flags on attachment: 275494
> > 
> > Committed r199005: <http://trac.webkit.org/changeset/199005>
> 
> It broke the WinCairo build:
> https://build.webkit.org/builders/WinCairo%2064-Bit%20Release/builds/55657

It was false positive alarm, clean build fixed the build. It would be great
if the port maintainers could investigate and fix this build system bug.