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).
Created attachment 124452 [details] Patch
Created attachment 124453 [details] Original InspectorFrontend.h
Created attachment 124454 [details] Original InspectorFrontend.cpp
Created attachment 124455 [details] New InspectorFrontend.h
Created attachment 124456 [details] New InspectorFrontend.cpp
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?
> > 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?
Created attachment 124614 [details] Patch
Created attachment 124615 [details] New InspectorFrontend.h
Created attachment 124616 [details] New InspectorFrontend.cpp
Comment on attachment 124614 [details] Patch LGTM
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.
Committed r106360: <http://trac.webkit.org/changeset/106360>