Bug 72835 - Web Inspector: [protocol] generate C++ classes for protocol JSON named types
Summary: Web Inspector: [protocol] generate C++ classes for protocol JSON named types
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 73205 73835
Blocks: 72861
  Show dependency treegraph
 
Reported: 2011-11-20 13:10 PST by Peter Rybin
Modified: 2011-12-12 07:29 PST (History)
15 users (show)

See Also:


Attachments
Patch (22.71 KB, patch)
2011-11-20 13:23 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Sample generated file (condensed form with C preprocessor defines) (77.13 KB, text/plain)
2011-11-21 00:23 PST, Peter Rybin
no flags Details
Sample generated file (long form) (127.73 KB, text/plain)
2011-11-21 00:24 PST, Peter Rybin
no flags Details
Patch (23.42 KB, patch)
2011-11-21 00:42 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Sample generated file (condensed form with C preprocessor defines) (82.10 KB, text/plain)
2011-11-21 00:45 PST, Peter Rybin
no flags Details
Patch (20.67 KB, patch)
2011-11-24 12:46 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Sample generated file (caseq template technique) (123.58 KB, text/plain)
2011-11-24 12:48 PST, Peter Rybin
no flags Details
Patch (21.51 KB, patch)
2011-11-25 08:23 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (21.42 KB, patch)
2011-12-04 11:51 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (21.38 KB, patch)
2011-12-05 16:27 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Patch (21.63 KB, patch)
2011-12-08 12:02 PST, Peter Rybin
no flags Details | Formatted Diff | Diff
Builder usage example (2.79 KB, patch)
2011-12-12 07:29 PST, Yury Semikhatsky
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Rybin 2011-11-20 13:10:05 PST
Extend CodeGeneratorInspector.py functionality in a wake of preparing internal typed C++ API for WebKit Remote Debugging protocol.
Comment 1 Peter Rybin 2011-11-20 13:23:20 PST
Created attachment 115996 [details]
Patch
Comment 2 Timothy Hatcher 2011-11-20 14:19:07 PST
What is the C/C++ API going to be used for? (Just curious.)
Comment 3 Peter Rybin 2011-11-20 15:29:41 PST
(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 4 Ilya Tikhonovsky 2011-11-20 21:47:41 PST
Comment on attachment 115996 [details]
Patch

could you please also provide a sample file and InspectorBackendDispatcher.cpp/h files
Comment 5 Peter Rybin 2011-11-21 00:23:24 PST
Created attachment 116041 [details]
Sample generated file (condensed form with C preprocessor defines)
Comment 6 Peter Rybin 2011-11-21 00:24:06 PST
Created attachment 116042 [details]
Sample generated file (long form)
Comment 7 Peter Rybin 2011-11-21 00:42:24 PST
Created attachment 116046 [details]
Patch
Comment 8 Peter Rybin 2011-11-21 00:45:27 PST
Created attachment 116047 [details]
Sample generated file (condensed form with C preprocessor defines)
Comment 9 Ilya Tikhonovsky 2011-11-23 00:17:43 PST
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.
Comment 10 Ilya Tikhonovsky 2011-11-23 00:22:19 PST
(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 11 Yury Semikhatsky 2011-11-24 06:33:32 PST
Comment on attachment 116046 [details]
Patch

r- as we decided to use template based approach.
Comment 12 Peter Rybin 2011-11-24 12:46:24 PST
Created attachment 116532 [details]
Patch
Comment 13 Peter Rybin 2011-11-24 12:48:29 PST
Created attachment 116533 [details]
Sample generated file (caseq template technique)
Comment 14 Yury Semikhatsky 2011-11-25 00:13:54 PST
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?
Comment 15 Andrey Kosyakov 2011-11-25 00:42:23 PST
(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;
Comment 16 Peter Rybin 2011-11-25 07:57:46 PST
> > 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
Comment 17 Peter Rybin 2011-11-25 08:22:24 PST
> 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
Comment 18 Peter Rybin 2011-11-25 08:23:32 PST
Created attachment 116625 [details]
Patch
Comment 19 Ilya Tikhonovsky 2011-11-25 11:15:18 PST
Comment on attachment 116625 [details]
Patch

lgtm
Comment 20 Ilya Tikhonovsky 2011-11-28 04:31:06 PST
Comment on attachment 116625 [details]
Patch

Clearing flags on attachment: 116625

Committed r101249: <http://trac.webkit.org/changeset/101249>
Comment 21 Ilya Tikhonovsky 2011-11-28 04:31:18 PST
All reviewed patches have been landed.  Closing bug.
Comment 22 Peter Rybin 2011-12-04 11:35:40 PST
Reopening.
Comment 23 Peter Rybin 2011-12-04 11:51:05 PST
Created attachment 117803 [details]
Patch
Comment 24 WebKit Review Bot 2011-12-05 08:56:17 PST
Comment on attachment 117803 [details]
Patch

Clearing flags on attachment: 117803

Committed r102004: <http://trac.webkit.org/changeset/102004>
Comment 25 WebKit Review Bot 2011-12-05 08:56:24 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Csaba Osztrogonác 2011-12-05 09:07:39 PST
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 27 Andrey Kosyakov 2011-12-05 15:06:10 PST
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.
Comment 28 Peter Rybin 2011-12-05 16:21:53 PST
> > 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.
Comment 29 Peter Rybin 2011-12-05 16:27:13 PST
Created attachment 117952 [details]
Patch
Comment 30 Peter Rybin 2011-12-05 16:35:40 PST
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
Comment 31 Peter Rybin 2011-12-08 12:02:36 PST
Created attachment 118440 [details]
Patch
Comment 32 Yury Semikhatsky 2011-12-09 00:33:15 PST
Comment on attachment 118440 [details]
Patch

Clearing flags on attachment: 118440

Committed r102440: <http://trac.webkit.org/changeset/102440>
Comment 33 Yury Semikhatsky 2011-12-09 00:33:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 34 Yury Semikhatsky 2011-12-09 00:38:24 PST
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
Comment 35 Patrick R. Gansterer 2011-12-10 07:50:30 PST
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?
Comment 36 Peter Rybin 2011-12-10 13:50:53 PST
(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.
Comment 37 Peter Rybin 2011-12-11 17:21:49 PST
I filed the bug and it has been closed by now.

https://bugs.webkit.org/show_bug.cgi?id=74247
Comment 38 Yury Semikhatsky 2011-12-12 06:09:46 PST
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.
Comment 39 Yury Semikhatsky 2011-12-12 07:29:40 PST
Created attachment 118785 [details]
Builder usage example