Bug 72835 - Web Inspector: [protocol] generate C++ classes for protocol JSON named types
: Web Inspector: [protocol] generate C++ classes for protocol JSON named types
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 73205 73835
: 72861
  Show dependency treegraph
 
Reported: 2011-11-20 13:10 PST by
Modified: 2011-12-12 07:29 PST (History)


Attachments
Patch (22.71 KB, patch)
2011-11-20 13:23 PST, Peter Rybin
no flags Review Patch | 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 Review Patch | 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 Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Patch (21.42 KB, patch)
2011-12-04 11:51 PST, Peter Rybin
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.38 KB, patch)
2011-12-05 16:27 PST, Peter Rybin
no flags Review Patch | Details | Formatted Diff | Diff
Patch (21.63 KB, patch)
2011-12-08 12:02 PST, Peter Rybin
no flags Review Patch | Details | Formatted Diff | Diff
Builder usage example (2.79 KB, patch)
2011-12-12 07:29 PST, Yury Semikhatsky
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-11-20 13:23:20 PST -------
Created an attachment (id=115996) [details]
Patch
------- Comment #2 From 2011-11-20 14:19:07 PST -------
What is the C/C++ API going to be used for? (Just curious.)
------- Comment #3 From 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 From 2011-11-20 21:47:41 PST -------
(From update of attachment 115996 [details])
could you please also provide a sample file and InspectorBackendDispatcher.cpp/h files
------- Comment #5 From 2011-11-21 00:23:24 PST -------
Created an attachment (id=116041) [details]
Sample generated file (condensed form with C preprocessor defines)
------- Comment #6 From 2011-11-21 00:24:06 PST -------
Created an attachment (id=116042) [details]
Sample generated file (long form)
------- Comment #7 From 2011-11-21 00:42:24 PST -------
Created an attachment (id=116046) [details]
Patch
------- Comment #8 From 2011-11-21 00:45:27 PST -------
Created an attachment (id=116047) [details]
Sample generated file (condensed form with C preprocessor defines)
------- Comment #9 From 2011-11-23 00:17:43 PST -------
(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.
------- Comment #10 From 2011-11-23 00:22:19 PST -------
(In reply to comment #9)
> (From update of attachment 116046 [details] [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 From 2011-11-24 06:33:32 PST -------
(From update of attachment 116046 [details])
r- as we decided to use template based approach.
------- Comment #12 From 2011-11-24 12:46:24 PST -------
Created an attachment (id=116532) [details]
Patch
------- Comment #13 From 2011-11-24 12:48:29 PST -------
Created an attachment (id=116533) [details]
Sample generated file (caseq template technique)
------- Comment #14 From 2011-11-25 00:13:54 PST -------
(From update of attachment 116532 [details])
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 From 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 From 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 From 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 From 2011-11-25 08:23:32 PST -------
Created an attachment (id=116625) [details]
Patch
------- Comment #19 From 2011-11-25 11:15:18 PST -------
(From update of attachment 116625 [details])
lgtm
------- Comment #20 From 2011-11-28 04:31:06 PST -------
(From update of attachment 116625 [details])
Clearing flags on attachment: 116625

Committed r101249: <http://trac.webkit.org/changeset/101249>
------- Comment #21 From 2011-11-28 04:31:18 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #22 From 2011-12-04 11:35:40 PST -------
Reopening.
------- Comment #23 From 2011-12-04 11:51:05 PST -------
Created an attachment (id=117803) [details]
Patch
------- Comment #24 From 2011-12-05 08:56:17 PST -------
(From update of attachment 117803 [details])
Clearing flags on attachment: 117803

Committed r102004: <http://trac.webkit.org/changeset/102004>
------- Comment #25 From 2011-12-05 08:56:24 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #26 From 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 From 2011-12-05 15:06:10 PST -------
(From update of attachment 117803 [details])
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 From 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 From 2011-12-05 16:27:13 PST -------
Created an attachment (id=117952) [details]
Patch
------- Comment #30 From 2011-12-05 16:35:40 PST -------
(From update of attachment 117952 [details])
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 From 2011-12-08 12:02:36 PST -------
Created an attachment (id=118440) [details]
Patch
------- Comment #32 From 2011-12-09 00:33:15 PST -------
(From update of attachment 118440 [details])
Clearing flags on attachment: 118440

Committed r102440: <http://trac.webkit.org/changeset/102440>
------- Comment #33 From 2011-12-09 00:33:29 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #34 From 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 From 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 From 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 From 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 From 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 From 2011-12-12 07:29:40 PST -------
Created an attachment (id=118785) [details]
Builder usage example