Extend CodeGeneratorInspector.py functionality in a wake of preparing internal typed C++ API for WebKit Remote Debugging protocol.
Created attachment 115996 [details] Patch
What is the C/C++ API going to be used for? (Just curious.)
(In reply to comment #2) > What is the C/C++ API going to be used for? (Just curious.) While our remote debugging protocol is well-defined and published nowdays, we don't have even runtime asserts that actual outgoing messages conform. This API will provide compile-time guarantee that C++ code builds outgoing messages strictly as as declared.
Comment on attachment 115996 [details] Patch could you please also provide a sample file and InspectorBackendDispatcher.cpp/h files
Created attachment 116041 [details] Sample generated file (condensed form with C preprocessor defines)
Created attachment 116042 [details] Sample generated file (long form)
Created attachment 116046 [details] Patch
Created attachment 116047 [details] Sample generated file (condensed form with C preprocessor defines)
Comment on attachment 116046 [details] Patch After the next round of review I decided that I'm against this implementation of native data classes for Inspector protocol. The plain version looks too long and complex. The variant with macros just masks this complexity. The generator code with support for both versions looks weird.
(In reply to comment #9) > (From update of attachment 116046 [details]) > After the next round of review I decided that I'm against this implementation of native data classes for Inspector protocol. The plain version looks too long and complex. The variant with macros just masks this complexity. The generator code with support for both versions looks weird. the sample template based version for discussion. #include <string> #include <iostream> #include <map> typedef std::map<std::string, std::string> map_type; // sample data container class ProtocolObjectFoo { public: enum { NO_FIELDS_SET = 0, FOO_SET = 1, BAR_SET = 2, BAZZ_SET = 4, ALL_FIELDS_SET = (FOO_SET | BAR_SET | BAZZ_SET) }; template<int n> class Builder { private: friend class ProtocolObjectFoo; Builder<n>() { static_assert(n == 0, "Builder<n> with n not equal to 0 was called"); m_result = new map_type(); } template<int m> operator Builder<m>& () { return *reinterpret_cast<Builder<m>*>(this); } map_type *m_result; public: Builder<n | FOO_SET>& setFoo(std::string value) { static_assert((n & FOO_SET) == 0, "ProtocolObjectFoo::setFoo was called second time"); (*m_result)["foo"] = value; return *this; } Builder<n | BAR_SET>& setBar(std::string value) { static_assert((n & BAR_SET) == 0, "ProtocolObjectFoo::setBar was called second time"); (*m_result)["bar"] = value; return *this; } Builder<n | BAZZ_SET>& setBazz(std::string value) { static_assert((n & BAZZ_SET) == 0, "ProtocolObjectFoo::setBazz was called second time"); (*m_result)["buzz"] = value; return *this; } operator map_type*() { static_assert(n == ALL_FIELDS_SET, "ProtocolObjectFoo not all the properties were set"); return m_result; } }; static Builder<NO_FIELDS_SET> create() { return Builder<NO_FIELDS_SET>(); } }; int main() { map_type* result1 = ProtocolObjectFoo::create() .setBazz("bbb") .setFoo("fff") .setBazz("bbb") // compile time for second .setBar("rrr"); map_type* result2 = ProtocolObjectFoo::create() .setBazz("bbb") // .setFoo("fff") // compile time error when not all the properties were set .setBar("rrr"); for (map_type::const_iterator it = result1->begin(); it != result1->end(); ++it) std::cout << it->first << ": " << it->second << std::endl; }
Comment on attachment 116046 [details] Patch r- as we decided to use template based approach.
Created attachment 116532 [details] Patch
Created attachment 116533 [details] Sample generated file (caseq template technique)
Comment on attachment 116532 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116532&action=review > Source/WebCore/inspector/CodeGeneratorInspector.py:534 > + template<int STEP> inline Builder<STATE | STEP>& castState() Do we really need all these methods to be inline?
(In reply to comment #13) > Sample generated file (caseq template technique) I generally like the result, but still got a few notes: - reinterpret_cast from InspectorObject to a specific object type looks quite unfortunate. Why not use a concrete type for m_result? - why do we need explicit inlines on every method? - let's pass string parameters by a const reference; - Builder constructor should by private, with outer class a friend; - it's probably worth implementing cast operator for PassRefPtr via cast operator for RefPtr;
> > Source/WebCore/inspector/CodeGeneratorInspector.py:534 > > + template<int STEP> inline Builder<STATE | STEP>& castState() > > Do we really need all these methods to be inline? Fixed
> I generally like the result, but still got a few notes: > - reinterpret_cast from InspectorObject to a specific object type looks quite unfortunate. Why not use a concrete type for m_result? done > - why do we need explicit inlines on every method? done > - let's pass string parameters by a const reference; done > - Builder constructor should by private, with outer class a friend; done > - it's probably worth implementing cast operator for PassRefPtr via cast operator for RefPtr; done
Created attachment 116625 [details] Patch
Comment on attachment 116625 [details] Patch lgtm
Comment on attachment 116625 [details] Patch Clearing flags on attachment: 116625 Committed r101249: <http://trac.webkit.org/changeset/101249>
All reviewed patches have been landed. Closing bug.
Reopening.
Created attachment 117803 [details] Patch
Comment on attachment 117803 [details] Patch Clearing flags on attachment: 117803 Committed r102004: <http://trac.webkit.org/changeset/102004>
It broke the --minimal build: http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/38215/steps/compile-webkit/logs/stdio Could you fix it, please?
Comment on attachment 117803 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117803&action=review > Source/WebCore/inspector/CodeGeneratorInspector.py:110 > + def lower_camel_case_to_upper(str): > + if len(str) > 0 and str[0].islower(): > + str = str[0].upper() + str[1:] > + return str String.capitalize()? > Source/WebCore/inspector/CodeGeneratorInspector.py:138 > + pos = 0 > + while pos < len(output): > + output[pos] = output[pos].upper() > + pos += 1 > + return "_".join(output) Why not just "_".join(output).upper()? > Source/WebCore/inspector/CodeGeneratorInspector.py:152 > + def split_camel_case_(str): > + output = [] > + pos_being = 0 > + pos = 1 > + has_oneletter = False > + while pos < len(str): > + if str[pos].isupper(): > + output.append(str[pos_being:pos].upper()) > + if pos - pos_being == 1: > + has_oneletter = True > + pos_being = pos > + pos += 1 This looks as it were ported from COBOL. Can this be rewritten into python -- e.g., use regexps? > Source/WebCore/inspector/CodeGeneratorInspector.py:459 > + output.append("namespace ") > + output.append(json_type["id"]) > + output.append(" {\n") Consider using interpolation for better readability. > Source/WebCore/inspector/CodeGeneratorInspector.py:466 > + item_c_name = enum_item.replace('-', '_') > + output.append("const char* const ") > + output.append(Capitalizer.upper_camel_case_to_lower(item_c_name)) > + output.append(" = \"") > + output.append(enum_item) > + output.append("\";\n") ditto > Source/WebCore/inspector/CodeGeneratorInspector.py:732 > + def fill_recursive(path_part, depth): > + if depth <= 0 or path_part == '/': > + return > + fill_recursive(os.path.dirname(path_part), depth - 1) > + components.append(os.path.basename(path_part)) Does this have to be recursive? Also, consider os.path.split(). Or, given that you join them back with "/".join() below, you could as well have used a regexp.
> > Source/WebCore/inspector/CodeGeneratorInspector.py:110 > > + def lower_camel_case_to_upper(str): > > + if len(str) > 0 and str[0].islower(): > > + str = str[0].upper() + str[1:] > > + return str > String.capitalize()? No. According to documentation it makes all letters but first lowercase. > > Source/WebCore/inspector/CodeGeneratorInspector.py:138 > > + pos = 0 > > + while pos < len(output): > > + output[pos] = output[pos].upper() > > + pos += 1 > > + return "_".join(output) > Why not just "_".join(output).upper()? Done > > Source/WebCore/inspector/CodeGeneratorInspector.py:152 > > + def split_camel_case_(str): > > + output = [] > > + pos_being = 0 > > + pos = 1 > > + has_oneletter = False > > + while pos < len(str): > > + if str[pos].isupper(): > > + output.append(str[pos_being:pos].upper()) > > + if pos - pos_being == 1: > > + has_oneletter = True > > + pos_being = pos > > + pos += 1 > This looks as it were ported from COBOL. Can this be rewritten into python -- e.g., use regexps? I don't really see how it can be re-written nicer. At least in 15 minutes. > > Source/WebCore/inspector/CodeGeneratorInspector.py:459 > > + output.append("namespace ") > > + output.append(json_type["id"]) > > + output.append(" {\n") > Consider using interpolation for better readability. I do use a lot of interpolation throughout the script. I'm not convinced it always gives better readability. > > Source/WebCore/inspector/CodeGeneratorInspector.py:732 > > + def fill_recursive(path_part, depth): > > + if depth <= 0 or path_part == '/': > > + return > > + fill_recursive(os.path.dirname(path_part), depth - 1) > > + components.append(os.path.basename(path_part)) > Does this have to be recursive? Also, consider os.path.split(). Or, given that you join them back with "/".join() below, you could as well have used a regexp. I don't know. I have to split path into components. Given I only can get the last component per time, recursion looked pretty nice solution. I don't like to use regexp, because I'm not sure about Windows stuff.
Created attachment 117952 [details] Patch
Comment on attachment 117952 [details] Patch This is only a response for caseq's codereivew. The patch is still not ready -- it break qt-minimal. http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/38215/steps/compile-webkit/logs/stdio
Created attachment 118440 [details] Patch
Comment on attachment 118440 [details] Patch Clearing flags on attachment: 118440 Committed r102440: <http://trac.webkit.org/changeset/102440>
Peter, please don't remove (OOPS!) from generated ChangeLog entries, otherwise our scripts won't be able to substitute reviewer's name and we will get changelog with Reviwed by NOBODY. line like here: http://trac.webkit.org/changeset/102440/trunk/Source/WebCore/ChangeLog
This patch breaks Windows CE. After this patch you generate a class with the RGBA. RGBA is a macro on windows and will cause sooner or later problems on the other windows platforms too (depending on the include order). What about giving the classes a prefix?
(In reply to comment #35) > This patch breaks Windows CE. After this patch you generate a class with the RGBA. RGBA is a macro on windows and will cause sooner or later problems on the other windows platforms too (depending on the include order). > What about giving the classes a prefix? Probably prefix would be too heavyweight solution. I think it's worth keeping a manually-filled map of exceptions for typenames. I'm going to prepare a patch.
I filed the bug and it has been closed by now. https://bugs.webkit.org/show_bug.cgi?id=74247
Today when trying to use the new typed API for building memory report in http://trac.webkit.org/changeset/102569/trunk/Source/WebCore/inspector/InspectorMemoryAgent.cpp I encountered several problems which I think we need to address: 1. Sometimes result construction may be spread over a method or even several methods. In this case we have to either first calculate all properties and then set all of them in one statement: RefPtr<DOMGroup> result = DOMGroup::create() .setSize(size_var) .setTitle(title_var) .setNodeCount(node_count_var); would be more convenient to have some result object which we could incrementally fill with the properties but with current approach we would have to declare variables types like Memory::DOMGroup::Builder<TITLE_SET | NODE_COUNT_SET> which is too cumbersome. 2. In case result is populated in several methods we need to pass one builder as a parameter and return another builder as a result. Type declaration is still quite long.
Created attachment 118785 [details] Builder usage example