Bug 69295 – Web Inspector: reimplement protocol backend/frontend source generator

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Peter Rybin]

Inspector includes generated C++ and JavaScript source. Currently generator is written in Python/Perl and uses intermediate IDL format.
Get rid of IDL intermediate and rewrite generator fully in Python.
This is need to prepare protocol validator development.

[Peter Rybin]

Created attachment 109534 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 109534 [details]
Patch

It can't be applied to the ToT could you please rebaseline it.

[Peter Rybin]

Created attachment 109704 [details]
Patch

[Peter Rybin]

Patch is uploaded this time to try on buildbots

[Early Warning System Bot]

Comment on attachment 109704 [details]
Patch

Attachment 109704 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9938611

[Gyuyoung Kim]

Comment on attachment 109704 [details]
Patch

Attachment 109704 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9940546

[Gustavo Noronha (kov)]

Comment on attachment 109704 [details]
Patch

Attachment 109704 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9960001

[Pavel Feldman]

Comment on attachment 109704 [details]
Patch

EWS reports multiple errors (qt, gtk, efl). Make sure your make files are ok.

[Peter Rybin]

Created attachment 109826 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 109826 [details]
Patch

Attachment 109826 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9953527

[Early Warning System Bot]

Comment on attachment 109826 [details]
Patch

Attachment 109826 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9958351

[Ilya Tikhonovsky]

InspectorProtocolVersion.h was generated into WebCore folder instead of WebKit folder.
Also it was added to the list of files for compilation.

gcc -c -pipe -O2 -o obj/release/InspectorProtocolVersion.o ../../WebCore/generated/InspectorProtocolVersion.h

[Peter Rybin]

Created attachment 109871 [details]
Patch

[Early Warning System Bot]

Comment on attachment 109871 [details]
Patch

Attachment 109871 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9981028

[Gyuyoung Kim]

Comment on attachment 109871 [details]
Patch

Attachment 109871 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9964095

[Peter Rybin]

Created attachment 109991 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 109991 [details]
Patch

Attachment 109991 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9977304

[Peter Rybin]

Created attachment 110021 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 110021 [details]
Patch

Attachment 110021 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9964416

[Peter Rybin]

Created attachment 110041 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 110041 [details]
Patch

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

> Source/WebCore/ChangeLog:10655
> [phone]  Peter Rybin  <[e-mail]>
> +
> +        Web Inspector: reimplement protocol backend/frontend source generator
> +        https://bugs.webkit.org/show_bug.cgi?id=69295
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        No new tests. (OOPS!)
> +
> +        * WebCore.gyp/WebCore.gyp:
> +        * inspector/CodeGeneratorInspector.pm:
> +        (finish):
> +        * inspector/CodeGeneratorInspector.py: Added.
> +

second change log entry.

> Source/WebCore/CodeGenerators.pri:679
> +inspectorJSON.output = $${WC_GENERATED_SOURCES_DIR}/InspectorFrontend.cpp $${WC_GENERATED_SOURCES_DIR}/InspectorBackendDispatcher.cpp

please add headers too

> Source/WebCore/WebCore.gyp/WebCore.gyp:369
> +          'message': 'Generating Inspector protocol sources from Inspector.idl',

from Inspector.json

> Source/WebCore/inspector/CodeGeneratorInspector.py:446
> +if (!protocolErrors->length())
> +    $agentField->$methodName(&error$agentCallParams);

wrong indent in the generated file.

> Source/WebCore/inspector/CodeGeneratorInspector.py:449
> +${responseCook}sendResponse(callId, result, String::format("Some arguments of method '%s' can't be processed", "$domainName.$methodName"), protocolErrors, error);

wrong indent in the generated file.
It'd be nice to extract String::format("Some arguments of method '%s' can't be processed", "some method name") call to an inline function.

> Source/WebCore/inspector/CodeGeneratorInspector.py:457
> +$code    if (m_inspectorFrontendChannel)

improvement: I'd like to convert this if statement to guard.

> Source/WebCore/inspector/CodeGeneratorInspector.py:462
> +    frontend_h = string.Template("""// Copyright (c) 2010 The Chromium Authors. All rights reserved.

Copyright (c) 2011

> Source/WebCore/inspector/CodeGeneratorInspector.py:482
> +

unnecessary empty line

> Source/WebCore/inspector/CodeGeneratorInspector.py:485
> +$domainClassList
> +private:

unnecessary empty line was inserted between last class and private statement

> Source/WebCore/inspector/CodeGeneratorInspector.py:493
> +    backend_h = string.Template("""// Copyright (c) 2010 The Chromium Authors. All rights reserved.

license 2011

> Source/WebCore/inspector/CodeGeneratorInspector.py:539
> +

unnecessary empty line

> Source/WebCore/inspector/CodeGeneratorInspector.py:540
> +$enumConstants

$methodNamesEnumContent ?

> Source/WebCore/inspector/CodeGeneratorInspector.py:541
> +};

wrong indent

> Source/WebCore/inspector/CodeGeneratorInspector.py:565
> +    backend_cpp = string.Template("""// Copyright (c) 2010 The Chromium Authors. All rights reserved.

Copyright (c) 2011

> Source/WebCore/inspector/CodeGeneratorInspector.py:589
> +

unnecessary empty string

> Source/WebCore/inspector/CodeGeneratorInspector.py:601
> +$messageHandlers

wrong indent in the generated file.

> Source/WebCore/inspector/CodeGeneratorInspector.py:666
> +    if (m_inspectorFrontendChannel)

improvement: I'd like to convert this if statement to guard.

> Source/WebCore/inspector/CodeGeneratorInspector.py:701
> +    if (m_inspectorFrontendChannel)

improvement: I'd like to convert this if statement to guard.

> Source/WebCore/inspector/CodeGeneratorInspector.py:840
> +PassRefPtr<InspectorObject> InspectorBackendDispatcher::getObject(InspectorObject* object, const String& name, bool* valueFound, InspectorArray* protocolErrors)
> +{
> +    ASSERT(protocolErrors);
> +
> +    if (valueFound)
> +        *valueFound = false;
> +
> +    RefPtr<InspectorObject> value = InspectorObject::create();
> +
> +    if (!object) {
> +        if (!valueFound) {
> +            // Required parameter in missing params container.
> +            protocolErrors->pushString(String::format("'params' object must contain required parameter '%s' with type 'Object'.", name.utf8().data()));
> +        }
> +        return value;
> +    }
> +
> +    InspectorObject::const_iterator end = object->end();
> +    InspectorObject::const_iterator valueIterator = object->find(name);
> +
> +    if (valueIterator == end) {
> +        if (!valueFound)
> +            protocolErrors->pushString(String::format("Parameter '%s' with type 'Object' was not found.", name.utf8().data()));
> +        return value;
> +    }
> +
> +    if (!valueIterator->second->asObject(&value))
> +        protocolErrors->pushString(String::format("Parameter '%s' has wrong type. It must be 'Object'.", name.utf8().data()));
> +    else
> +        if (valueFound)
> +            *valueFound = true;
> +    return value;
> +}
> +

The old version is generating the getters

> Source/WebCore/inspector/CodeGeneratorInspector.py:874
> +bool InspectorBackendDispatcher::getCommandName(const String& message, String* result)

empty line required

> Source/WebCore/inspector/CodeGeneratorInspector.py:914
> +
> +

unnecessary empty lines

> Source/WebCore/inspector/CodeGeneratorInspector.py:928
> +    backend_js = string.Template("""// Copyright (c) 2010 The Chromium Authors. All rights reserved.

license 2011

> Source/WebCore/inspector/CodeGeneratorInspector.py:940
> +$delegates$eventArgs$domainDispatchers    }

wrong indent in the generated code

> Source/WebCore/inspector/CodeGeneratorInspector.py:1398
> +    enumConstants=join(Generator.method_name_enum_list, "\n"),

bad name

[Peter Rybin]

Hi Ilya,

Thank you for review!

In this change I'm trying to provide a generator that is character-compatible with the old version. Old extra or missing lines are kept as legacy. I think we should leave all improvements to the next CL not to do 2 things at once.

> > Source/WebCore/ChangeLog:10655
> > [phone]  Peter Rybin  <[e-mail]>
> > +
> > +        Web Inspector: reimplement protocol backend/frontend source generator
> > +        https://bugs.webkit.org/show_bug.cgi?id=69295
> > +
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        No new tests. (OOPS!)
> > +
> > +        * WebCore.gyp/WebCore.gyp:
> > +        * inspector/CodeGeneratorInspector.pm:
> > +        (finish):
> > +        * inspector/CodeGeneratorInspector.py: Added.
> > +
> 
> second change log entry.

Done

> > Source/WebCore/CodeGenerators.pri:679
> > +inspectorJSON.output = $${WC_GENERATED_SOURCES_DIR}/InspectorFrontend.cpp $${WC_GENERATED_SOURCES_DIR}/InspectorBackendDispatcher.cpp
> please add headers too

Are you sure it's the right thing to do? Look at the removed target below, it doesn't list header files.

> > Source/WebCore/WebCore.gyp/WebCore.gyp:369
> > +          'message': 'Generating Inspector protocol sources from Inspector.idl',
> from Inspector.json
Done

> > Source/WebCore/inspector/CodeGeneratorInspector.py:449
> > +${responseCook}sendResponse(callId, result, String::format("Some arguments of method '%s' can't be processed", "$domainName.$methodName"), protocolErrors, error);
> It'd be nice to extract String::format("Some arguments of method '%s' can't be processed", "some method name") call to an inline function.

There's no need for formatting it on runtime. We should format it right in generator instead.

> > Source/WebCore/inspector/CodeGeneratorInspector.py:540
> > +$enumConstants
> $methodNamesEnumContent ?

Done

> > Source/WebCore/inspector/CodeGeneratorInspector.py:601
> > +$messageHandlers
> wrong indent in the generated file.

This is a multi-line placeholder. Indents are inside argument.

> > Source/WebCore/inspector/CodeGeneratorInspector.py:840
> > +PassRefPtr<InspectorObject> InspectorBackendDispatcher::getObject(InspectorObject* object, const String& name, bool* valueFound, InspectorArray* protocolErrors)
> > +{
> The old version is generating the getters

I missed this. Let me handle it in the next change. I would consider leaving it as it is or programming using C++ template technology (to keep the minimum amount of generated code).

> > Source/WebCore/inspector/CodeGeneratorInspector.py:1398
> > +    enumConstants=join(Generator.method_name_enum_list, "\n"),
> bad name

Done

[Peter Rybin]

Created attachment 110162 [details]
Patch

[Ilya Tikhonovsky]

> > > Source/WebCore/inspector/CodeGeneratorInspector.py:449
> > > +${responseCook}sendResponse(callId, result, String::format("Some arguments of method '%s' can't be processed", "$domainName.$methodName"), protocolErrors, error);
> > It'd be nice to extract String::format("Some arguments of method '%s' can't be processed", "some method name") call to an inline function.
> 
> There's no need for formatting it on runtime. We should format it right in generator instead.
> 

In this case the size of binary will be bigger.


> > > Source/WebCore/inspector/CodeGeneratorInspector.py:601
> > > +$messageHandlers
> > wrong indent in the generated file.
> 
> This is a multi-line placeholder. Indents are inside argument.

I saw the wrong indent in the generated file.

[Peter Rybin]

Created attachment 110523 [details]
Patch

[Pavel Feldman]

Comment on attachment 110523 [details]
Patch

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

Minor formal comments, otherwise code looks good.

> Source/WebCore/ChangeLog:3
> +        Web Inspector: reimplement protocol backend/frontend source generator

You should be more explicit wrt rationale of the change. I.e. port source generator from perl to python, explain why.

> Source/WebCore/ChangeLog:3178
> [phone]  Peter Rybin  <[e-mail]>

You should nuke this

> Source/WebCore/ChangeLog:13819
> [phone]  Peter Rybin  <[e-mail]>

ditto

> Source/WebCore/WebCore.gyp/WebCore.gyp:363
> +            '../inspector/CodeGeneratorInspector.py',

prefer <@(_inputs)

> Source/WebCore/inspector/CodeGeneratorInspector.py:493
> +    backend_h = string.Template("""// Copyright (c) 2010 The Chromium Authors. All rights reserved.

2011

[Peter Rybin]

(In reply to comment #26)
> > Source/WebCore/ChangeLog:3
> > +        Web Inspector: reimplement protocol backend/frontend source generator
> You should be more explicit wrt rationale of the change. I.e. port source generator from perl to python, explain why.

Done

> > Source/WebCore/ChangeLog:3178
> > [phone]  Peter Rybin  <[e-mail]>
> You should nuke this

Done

> > Source/WebCore/ChangeLog:13819
> > [phone]  Peter Rybin  <[e-mail]>
> 
> ditto

Doneto

> > Source/WebCore/WebCore.gyp/WebCore.gyp:363
> > +            '../inspector/CodeGeneratorInspector.py',
> 
> prefer <@(_inputs)
Done

> > Source/WebCore/inspector/CodeGeneratorInspector.py:493
> > +    backend_h = string.Template("""// Copyright (c) 2010 The Chromium Authors. All rights reserved.
> 2011

Let's not modify output during this change. This change is the true port.

[Peter Rybin]

Created attachment 111276 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


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

[Peter Rybin]

Created attachment 111279 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
From git://git.webkit.org/WebKit
   38bca30..7d79b00  master     -> origin/master
	M	Source/WebCore/ChangeLog
	M	Source/WebCore/accessibility/AccessibilitySlider.h
	M	Source/WebCore/accessibility/AccessibilityScrollView.h
	M	Source/WebCore/accessibility/AccessibilityTableHeaderContainer.cpp
	M	Source/WebCore/accessibility/AccessibilityTableColumn.cpp
	M	Source/WebCore/accessibility/AccessibilityRenderObject.h
	M	Source/WebCore/accessibility/AccessibilityObject.h
	M	Source/WebCore/accessibility/AccessibilitySlider.cpp
	M	Source/WebCore/accessibility/AccessibilityScrollView.cpp
	M	Source/WebCore/accessibility/AccessibilityRenderObject.cpp
	M	Source/WebCore/accessibility/AccessibilityObject.cpp
	M	Source/WebCore/accessibility/AccessibilityTableHeaderContainer.h
	M	Source/WebCore/accessibility/AccessibilityTableColumn.h
r97629 = db16dcbed1aa59c3f8b9618e43d75410fa38e632 (refs/remotes/trunk)
	M	LayoutTests/platform/qt-wk2/Skipped
	M	LayoutTests/ChangeLog
r97630 = 7d79b00ad79af37d995a3601775a897ea3539050 (refs/remotes/trunk)
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/trunk.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


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

[Ilya Tikhonovsky]

Comment on attachment 111279 [details]
Patch

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

> Source/WebCore/inspector/CodeGeneratorInspector.py:265
> +        def getCParamType(cls, param_type, optional):
> +            if (param_type == ParamType.EVENT):
> +                if optional:
> +                    return cls._ref_c_type
> +                else:
> +                    return cls._plain_c_type
> +            else:
> +                return cls._plain_c_type

I'm a bit confused by the mix of styles in this script.
I'd like to see the names of methods, member variables, etc formatted according to WebKit style guide.

[Peter Rybin]

Created attachment 111660 [details]
Patch

[Peter Rybin]

(In reply to comment #32)
> (From update of attachment 111279 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111279&action=review
> 
> > Source/WebCore/inspector/CodeGeneratorInspector.py:265
> > +        def getCParamType(cls, param_type, optional):
> > +            if (param_type == ParamType.EVENT):
> > +                if optional:
> > +                    return cls._ref_c_type
> > +                else:
> > +                    return cls._plain_c_type
> > +            else:
> > +                return cls._plain_c_type
> 
> I'm a bit confused by the mix of styles in this script.
> I'd like to see the names of methods, member variables, etc formatted according to WebKit style guide.

Done

[Ilya Tikhonovsky]

Comment on attachment 111660 [details]
Patch

I asked Peter to keep the generator code to be consistent with WebKit style guide. http://www.webkit.org/coding/coding-style.html
But Peter found that WebKit community has a separate guide for Python and informally supports PEP8.

"Informally, we try to follow  PEP8. Eventually we may make this official." from http://trac.webkit.org/wiki/PythonGuidelines


lgtm

[Ilya Tikhonovsky]

Comment on attachment 111660 [details]
Patch

Clearing flags on attachment: 111660

Committed r97954: <http://trac.webkit.org/changeset/97954>

[Ilya Tikhonovsky]

All reviewed patches have been landed.  Closing bug.

[Ilya Tikhonovsky]

Created attachment 111828 [details]
Patch

[Ilya Tikhonovsky]

Committed r98069: <http://trac.webkit.org/changeset/98069>

[Pavel Feldman]

Comment on attachment 111828 [details]
Patch

Clearing r? from the patch that has landed.

[Nico Weber]

The build output when building chromium now looks like this:

hummer:src thakis$ time ninja -C out/Release/ chrome 
ninja: Entering directory `out/Release/'
[595/10791] ACTION Generating Inspector protocol sources from Inspector.json
    writing ../../../../../out/Release/gen/webkit/InspectorBackendDispatcher.h
    writing ../../../../../out/Release/gen/webcore/InspectorBackendDispatcher.cpp
    writing ../../../../../out/Release/gen/webkit/InspectorFrontend.h
    writing ../../../../../out/Release/gen/webcore/InspectorFrontend.cpp
    writing ../../../../../out/Release/gen/webkit/InspectorTypeBuilder.h
    writing ../../../../../out/Release/gen/webcore/InspectorBackendCommands.js
[5508/10791] CXX obj/third_party/WebKit/Source/WebCore/html/webcore_html.HTMLStyleElement.o


Can you remove the print spew from this new generator please?

[Peter Rybin]

(In reply to comment #41)
> The build output when building chromium now looks like this:
> 
> hummer:src thakis$ time ninja -C out/Release/ chrome 
> ninja: Entering directory `out/Release/'
> [595/10791] ACTION Generating Inspector protocol sources from Inspector.json
>     writing ../../../../../out/Release/gen/webkit/InspectorBackendDispatcher.h
>     writing ../../../../../out/Release/gen/webcore/InspectorBackendDispatcher.cpp
>     writing ../../../../../out/Release/gen/webkit/InspectorFrontend.h
>     writing ../../../../../out/Release/gen/webcore/InspectorFrontend.cpp
>     writing ../../../../../out/Release/gen/webkit/InspectorTypeBuilder.h
>     writing ../../../../../out/Release/gen/webcore/InspectorBackendCommands.js
> [5508/10791] CXX obj/third_party/WebKit/Source/WebCore/html/webcore_html.HTMLStyleElement.o
> 
> 
> Can you remove the print spew from this new generator please?

Sorry about this.
https://bugs.webkit.org/show_bug.cgi?id=91758