Bug 77289 – Web Inspector: CodeGeneratorInspector.py: reimplement generated array types

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

[Peter Rybin]

Array type should be rendered more of a helper kind, rather than the current first class citizen kind (when it gets its name from parameter name and incapsulates more important item type).

[Peter Rybin]

Created attachment 124452 [details]
Patch

[Peter Rybin]

Created attachment 124453 [details]
Original InspectorFrontend.h

[Peter Rybin]

Created attachment 124454 [details]
Original InspectorFrontend.cpp

[Peter Rybin]

Created attachment 124455 [details]
New InspectorFrontend.h

[Peter Rybin]

Created attachment 124456 [details]
New InspectorFrontend.cpp

[Andrey Kosyakov]

Comment on attachment 124452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124452&action=review

> Source/WebCore/inspector/CodeGeneratorInspector.py:1580
> +template<typename T>
> +class ArrayItemHelper {
> +public:
> +    typedef typename T::ItemTraits Traits;
> +};

Why not use T::ItemTraits directly?

> Source/WebCore/inspector/CodeGeneratorInspector.py:1583
> +class ArrayOf : public InspectorArray {

We don't normally use Of in container type names, e.g. it's map or vector, not mapOf or VectorOf.

> Source/WebCore/inspector/CodeGeneratorInspector.py:1609
> +class StructItemTraits {
> +public:

just struct?

> Source/WebCore/inspector/CodeGeneratorInspector.py:1610
> +    static void pushRefPtr(InspectorArray* array, PassRefPtr<InspectorObject> value)

why do we need to include type into method name, i.e. why not just push()?

> Source/WebCore/inspector/CodeGeneratorInspector.py:1618
> +    template<typename T>
> +    static void assertCorrectValue(InspectorValue* value) {
> +        T::assertCorrectValue(value);
> +    }

Why do we need this wrapper?

> Source/WebCore/inspector/CodeGeneratorInspector.py:1625
> +class ArrayItemHelper<String> {
> +public:
> +    class Traits {
> +    public:

just struct?

> Source/WebCore/inspector/CodeGeneratorInspector.py:1626
> +        static void pushRaw(InspectorArray* array, const String& value)

just push()?

> Source/WebCore/inspector/CodeGeneratorInspector.py:1637
> +class ArrayItemHelper<InspectorObject> {
> +public:
> +    class Traits {
> +    public:

ditto

> Source/WebCore/inspector/CodeGeneratorInspector.py:2115
> +        ArrayItemHelper<T>::Traits::template assertCorrectValue<T>(array->get(i).get());

Does this compile?

[Peter Rybin]

> > Source/WebCore/inspector/CodeGeneratorInspector.py:1580
> > +template<typename T>
> > +class ArrayItemHelper {
> > +public:
> > +    typedef typename T::ItemTraits Traits;
> > +};
> Why not use T::ItemTraits directly?

This template type is a point where C++ specialization takes part. This class is only a default implementation.
I added a comment.

> > Source/WebCore/inspector/CodeGeneratorInspector.py:1583
> > +class ArrayOf : public InspectorArray {

> We don't normally use Of in container type names, e.g. it's map or vector, not mapOf or VectorOf.

Get rid of "of"? It's cleaner? Thanks Sean! :)

> > Source/WebCore/inspector/CodeGeneratorInspector.py:1609
> > +class StructItemTraits {
> > +public:
> just struct?
Done

> > Source/WebCore/inspector/CodeGeneratorInspector.py:1610
> > +    static void pushRefPtr(InspectorArray* array, PassRefPtr<InspectorObject> value)
> why do we need to include type into method name, i.e. why not just push()?

There is a template/specialization machinery with "T"/"RefPtr<T>" method overloading here, plus the entire code is generated. I would prefer to at least dismiss internal method overloading in favor of keeping it cleaner. So I have 2 explicitly named methods. This is not user-facing.

> > Source/WebCore/inspector/CodeGeneratorInspector.py:1618
> > +    template<typename T>
> > +    static void assertCorrectValue(InspectorValue* value) {
> > +        T::assertCorrectValue(value);
> > +    }

> Why do we need this wrapper?
> > Source/WebCore/inspector/CodeGeneratorInspector.py:1625
> > +class ArrayItemHelper<String> {
> > +public:
> > +    class Traits {
> > +    public:
> just struct?

Done

> > Source/WebCore/inspector/CodeGeneratorInspector.py:1626
> > +        static void pushRaw(InspectorArray* array, const String& value)
> just push()?

See above.

> > Source/WebCore/inspector/CodeGeneratorInspector.py:1637
> > +class ArrayItemHelper<InspectorObject> {
> > +public:
> > +    class Traits {
> > +    public:
> ditto

Done

> > Source/WebCore/inspector/CodeGeneratorInspector.py:2115
> > +        ArrayItemHelper<T>::Traits::template assertCorrectValue<T>(array->get(i).get());
> Does this compile?
Yes. Why?

[Peter Rybin]

Created attachment 124614 [details]
Patch

[Peter Rybin]

Created attachment 124615 [details]
New InspectorFrontend.h

[Peter Rybin]

Created attachment 124616 [details]
New InspectorFrontend.cpp

[Andrey Kosyakov]

Comment on attachment 124614 [details]
Patch

LGTM

[Vsevolod Vlasov]

Comment on attachment 124614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124614&action=review

> Source/WebCore/inspector/CodeGeneratorInspector.py:1712
> +

Extra line.

> Source/WebCore/inspector/CodeGeneratorInspector.py:2204
> +    for (unsigned i = 0; i < array->length(); i++) {

Please remove braces.

[Vsevolod Vlasov]

Committed r106360: <http://trac.webkit.org/changeset/106360>