Bug 173273 – Modernize UserScript.h

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Modernize UserScript.h

[Alex Christensen]

Created attachment 312692 [details]
Patch

[Alex Christensen]

Created attachment 312696 [details]
Patch

[Alex Christensen]

Created attachment 312708 [details]
Patch

[Alex Christensen]

https://trac.webkit.org/r218146

[Darin Adler]

Comment on attachment 312708 [details]
Patch

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

> Source/WebCore/page/UserScript.h:35
>  class UserScript {

This seems like a struct, not a class. All it has is the constructor and getter functions for all data members. That’s exactly what a const struct is like.

> Source/WebCore/page/UserScript.h:38
> +    UserScript() { }

We could write this instead, which I prefer.

    UserScript() = default;

> Source/WebCore/page/UserScript.h:39
> +    ~UserScript() { }

This should be omitted. This is what the compiler will do if you don’t define a destructor.

> Source/WebCore/page/UserScript.h:59
> +    template<class Encoder> void encode(Encoder&) const;
> +    template<class Decoder> static bool decode(Decoder&, UserScript&);

We normally prefer typename rather than class for the type arguments.

Why do these functions need to be members? They can be written entirely using public interface.

[Alex Christensen]

(In reply to Darin Adler from comment #5)
> Comment on attachment 312708 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=312708&action=review
> 
> > Source/WebCore/page/UserScript.h:35
> >  class UserScript {
> 
> This seems like a struct, not a class. All it has is the constructor and
> getter functions for all data members. That’s exactly what a const struct is
> like.
> 
Yep, could be done.

> > Source/WebCore/page/UserScript.h:38
> > +    UserScript() { }
> 
> We could write this instead, which I prefer.
> 
>     UserScript() = default;
> 
> > Source/WebCore/page/UserScript.h:39
> > +    ~UserScript() { }
> 
> This should be omitted. This is what the compiler will do if you don’t
> define a destructor.
> 
I agree with these pieces of feedback, and I did them in earlier patches, but EWS/El Capitan's compilers failed to link WebKit2 for some reason.

> > Source/WebCore/page/UserScript.h:59
> > +    template<class Encoder> void encode(Encoder&) const;
> > +    template<class Decoder> static bool decode(Decoder&, UserScript&);
> 
> We normally prefer typename rather than class for the type arguments.
> 
> Why do these functions need to be members? They can be written entirely
> using public interface.
> 
Indeed