| Summary: | Web Inspector: reimplement protocol backend/frontend source generator | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Peter Rybin <peter.rybin> | ||||||||||||||||||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Ilya Tikhonovsky <loislo> | ||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | apavlov, bweinstein, gustavo, joepeck, keishi, loislo, peter.rybin, pfeldman, pmuellr, rik, thakis, timothy, webkit.review.bot, xan.lopez, yurys | ||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||
| Bug Depends on: | 70486 | ||||||||||||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||
|
Description
Peter Rybin
2011-10-03 14:56:44 PDT
Created attachment 109534 [details]
Patch
Comment on attachment 109534 [details]
Patch
It can't be applied to the ToT could you please rebaseline it.
Created attachment 109704 [details]
Patch
Patch is uploaded this time to try on buildbots Comment on attachment 109704 [details] Patch Attachment 109704 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9938611 Comment on attachment 109704 [details] Patch Attachment 109704 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9940546 Comment on attachment 109704 [details] Patch Attachment 109704 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9960001 Comment on attachment 109704 [details]
Patch
EWS reports multiple errors (qt, gtk, efl). Make sure your make files are ok.
Created attachment 109826 [details]
Patch
Comment on attachment 109826 [details] Patch Attachment 109826 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9953527 Comment on attachment 109826 [details] Patch Attachment 109826 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9958351 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 Created attachment 109871 [details]
Patch
Comment on attachment 109871 [details] Patch Attachment 109871 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9981028 Comment on attachment 109871 [details] Patch Attachment 109871 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9964095 Created attachment 109991 [details]
Patch
Comment on attachment 109991 [details] Patch Attachment 109991 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9977304 Created attachment 110021 [details]
Patch
Comment on attachment 110021 [details] Patch Attachment 110021 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9964416 Created attachment 110041 [details]
Patch
Comment on attachment 110041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=110041&action=review > Source/WebCore/ChangeLog:10655 > +2011-10-03 Peter Rybin <peter.rybin@gmail.com> > + > + 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 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 > > +2011-10-03 Peter Rybin <peter.rybin@gmail.com> > > + > > + 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 Created attachment 110162 [details]
Patch
> > > 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. Created attachment 110523 [details]
Patch
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 > +2011-10-04 Peter Rybin <peter.rybin@gmail.com> You should nuke this > Source/WebCore/ChangeLog:13819 > +2011-10-03 Peter Rybin <peter.rybin@gmail.com> 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 (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 > > +2011-10-04 Peter Rybin <peter.rybin@gmail.com> > You should nuke this Done > > Source/WebCore/ChangeLog:13819 > > +2011-10-03 Peter Rybin <peter.rybin@gmail.com> > > 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. Created attachment 111276 [details]
Patch
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. Created attachment 111279 [details]
Patch
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. 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. Created attachment 111660 [details]
Patch
(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 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 Comment on attachment 111660 [details] Patch Clearing flags on attachment: 111660 Committed r97954: <http://trac.webkit.org/changeset/97954> All reviewed patches have been landed. Closing bug. Created attachment 111828 [details]
Patch
Committed r98069: <http://trac.webkit.org/changeset/98069> Comment on attachment 111828 [details]
Patch
Clearing r? from the patch that has landed.
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?
(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 |