Bug 110159 – Web Inspector: take large strings out of CodeGeneratorInspector.py

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

[Peter Rybin]

CodeGeneratorInspector.py largely contains multiline string literals.
Those should be taken away into a separate file to keep code and string resources better separated.

[Peter Rybin]

Created attachment 188951 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 188951 [details]
Patch

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

> Source/WebCore/inspector/CodeGeneratorInspector.py:40
> +import CodeGeneratorInspectorStrings

Maybe CodeGeneratorInspectorTemplates?

[Ilya Tikhonovsky]

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

What do you think about further splitting of this file. I'd expect to see one template file per one result file.

> Source/WebCore/inspector/CodeGeneratorInspectorStrings.py:701
> +#endif  // !ASSERT_DISABLED

Wrong comment here and in many other places like that.

> Source/WebCore/inspector/CodeGeneratorInspectorStrings.py:922
> +class_binding_builder_part_1 = (
> +"""        AllFieldsSet = %s
> +    };
> +
> +    template<int STATE>
> +    class Builder {
> +    private:
> +        RefPtr<InspectorObject> m_result;
> +
> +        template<int STEP> Builder<STATE | STEP>& castState()
> +        {
> +            return *reinterpret_cast<Builder<STATE | STEP>*>(this);
> +        }
> +
> +        Builder(PassRefPtr</*%s*/InspectorObject> ptr)
> +        {
> +            COMPILE_ASSERT(STATE == NoFieldsSet, builder_created_in_non_init_state);
> +            m_result = ptr;
> +        }
> +        friend class %s;
> +    public:
> +""")

I think that it would be more declarative and less error prone if you convert it to a function with named argument instead of a string.

[Peter Rybin]

> > Source/WebCore/inspector/CodeGeneratorInspector.py:40
> > +import CodeGeneratorInspectorStrings
> Maybe CodeGeneratorInspectorTemplates?

Please write how positive you are about this change, because I'm 50/50.

The reason why I chose "strings" is because I meant this file as plain storage for multiline strings. I don't care whether they are templates or what.

Multiline string is the only thing you cannot take with you into type-safe language if you ever want to.

[Peter Rybin]

> What do you think about further splitting of this file. I'd expect to see one template file per one result file.

That would bring more strange files into repository, but I don't see a clear benefit from this. You are not going to get a real general-purpose template engine anyway. Current code structure is very traceable. For example, all generated scripts declare the path to the generator file. 

> > Source/WebCore/inspector/CodeGeneratorInspectorStrings.py:701
> > +#endif  // !ASSERT_DISABLED
> Wrong comment here and in many other places like that.

Thanks.

> > Source/WebCore/inspector/CodeGeneratorInspectorStrings.py:922
> > +class_binding_builder_part_1 = (
> > +"""        AllFieldsSet = %s
> > +    };
> > +
> > +    template<int STATE>
> > +    class Builder {
> > +    private:
> > +        RefPtr<InspectorObject> m_result;
> > +
> > +        template<int STEP> Builder<STATE | STEP>& castState()
> > +        {
> > +            return *reinterpret_cast<Builder<STATE | STEP>*>(this);
> > +        }
> > +
> > +        Builder(PassRefPtr</*%s*/InspectorObject> ptr)
> > +        {
> > +            COMPILE_ASSERT(STATE == NoFieldsSet, builder_created_in_non_init_state);
> > +            m_result = ptr;
> > +        }
> > +        friend class %s;
> > +    public:
> > +""")
> 
> I think that it would be more declarative and less error prone if you convert it to a function with named argument instead of a string.

Unfortunately that's not what I planned to do. I see at as a collection of multiline string resources, no code or any other mark-up. If I ever were to switch script to type-safe language, I would need multiline strings separate. In this form as I suggest, I would be able to write a really simple RegExp-based parser to easily read this strings. This is basically a pseudo-python similar to like JSON is a pseuod-JavaScript.

[Peter Rybin]

Created attachment 189164 [details]
Patch

[Yury Semikhatsky]

(In reply to comment #4)
> > > Source/WebCore/inspector/CodeGeneratorInspector.py:40
> > > +import CodeGeneratorInspectorStrings
> > Maybe CodeGeneratorInspectorTemplates?
> 
> Please write how positive you are about this change, because I'm 50/50.
> 
> The reason why I chose "strings" is because I meant this file as plain storage for multiline strings. I don't care whether they are templates or what.
> 
> Multiline string is the only thing you cannot take with you into type-safe language if you ever want to.

Ok, that makes sense.

[WebKit Review Bot]

Comment on attachment 189164 [details]
Patch

Clearing flags on attachment: 189164

Committed r143586: <http://trac.webkit.org/changeset/143586>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.