Bug 72835 – Web Inspector: [protocol] generate C++ classes for protocol JSON named types

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

[Peter Rybin]

Extend CodeGeneratorInspector.py functionality in a wake of preparing internal typed C++ API for WebKit Remote Debugging protocol.

[Peter Rybin]

Created attachment 115996 [details]
Patch

[Timothy Hatcher]

What is the C/C++ API going to be used for? (Just curious.)

[Peter Rybin]

(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.

[Ilya Tikhonovsky]

Comment on attachment 115996 [details]
Patch

could you please also provide a sample file and InspectorBackendDispatcher.cpp/h files

[Peter Rybin]

Created attachment 116041 [details]
Sample generated file (condensed form with C preprocessor defines)

[Peter Rybin]

Created attachment 116042 [details]
Sample generated file (long form)

[Peter Rybin]

Created attachment 116046 [details]
Patch

[Peter Rybin]

Created attachment 116047 [details]
Sample generated file (condensed form with C preprocessor defines)

[Ilya Tikhonovsky]

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.

[Ilya Tikhonovsky]

(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;
}

[Yury Semikhatsky]

Comment on attachment 116046 [details]
Patch

r- as we decided to use template based approach.

[Peter Rybin]

Created attachment 116532 [details]
Patch

[Peter Rybin]

Created attachment 116533 [details]
Sample generated file (caseq template technique)

[Yury Semikhatsky]

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?

[Andrey Kosyakov]

(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;

[Peter Rybin]

> > 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

[Peter Rybin]

> 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

[Peter Rybin]

Created attachment 116625 [details]
Patch

[Ilya Tikhonovsky]

Comment on attachment 116625 [details]
Patch

lgtm

[Ilya Tikhonovsky]

Comment on attachment 116625 [details]
Patch

Clearing flags on attachment: 116625

Committed r101249: <http://trac.webkit.org/changeset/101249>

[Ilya Tikhonovsky]

All reviewed patches have been landed.  Closing bug.

[Peter Rybin]

Reopening.

[Peter Rybin]

Created attachment 117803 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117803 [details]
Patch

Clearing flags on attachment: 117803

Committed r102004: <http://trac.webkit.org/changeset/102004>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

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?

[Andrey Kosyakov]

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.

[Peter Rybin]

> > 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.

[Peter Rybin]

Created attachment 117952 [details]
Patch

[Peter Rybin]

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

[Peter Rybin]

Created attachment 118440 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 118440 [details]
Patch

Clearing flags on attachment: 118440

Committed r102440: <http://trac.webkit.org/changeset/102440>

[Yury Semikhatsky]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

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

[Patrick R. Gansterer]

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?

[Peter Rybin]

(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.

[Peter Rybin]

I filed the bug and it has been closed by now.

https://bugs.webkit.org/show_bug.cgi?id=74247

[Yury Semikhatsky]

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.

[Yury Semikhatsky]

Created attachment 118785 [details]
Builder usage example