Bug 112782 – [WK2] Remove repeating code in declaration of WK2 API classes

• Component: WebKit2
• Status: RESOLVED FIXED

[Mikhail Pozdnyakov]

The following two lines of code are repeated in every class inheriting WebKit::APIObject.

1) static const Type APIType = SomeAPIType;
2) virtual Type type() const { return APIType; }

This code copy/paste could be avoided.

[Mikhail Pozdnyakov]

Created attachment 194002 [details]
WIP patch

This proposal patch introduces template class 

template <APIObject::Type ArgumentType>
class TypedAPIObject : public APIObject

which contains the common functionality.

[EFL EWS Bot]

Comment on attachment 194002 [details]
WIP patch

Attachment 194002 [details] did not pass efl-ews (efl):
Output: http://webkit-commit-queue.appspot.com/results/17140413

[Mikhail Pozdnyakov]

Comment on attachment 194002 [details]
WIP patch

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

> Source/WebKit2/UIProcess/efl/WebPopupItemEfl.h:35
> +class WebPopupItemEfl : TypedAPIObject<APIObject::TypePopupMenuItem> {

argh, missed 'public' here :(

[Benjamin Poulain]

Comment on attachment 194002 [details]
WIP patch

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

This looks like a great idea but if you plan to only fix the ELF types, I am gonna stab you :)

> Source/WebKit2/Shared/APIObject.h:165
> +    virtual Type type() const { return APIType; }

Shouldn't it be private?

[Mikhail Pozdnyakov]

(In reply to comment #4)
> (From update of attachment 194002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194002&action=review
> 
> This looks like a great idea but if you plan to only fix the ELF types, I am gonna stab you :)
> 
No no, it was just an illustration patch, I am planning to deploy it to every object :)

> > Source/WebKit2/Shared/APIObject.h:165
> > +    virtual Type type() const { return APIType; }
> 
> Shouldn't it be private?

It could, but this would mean that the object cannot invoke it's own type() method, so would keep it protected.

[Mikhail Pozdnyakov]

Created attachment 194269 [details]
patch

[Benjamin Poulain]

Comment on attachment 194269 [details]
patch

Great change!
Can you deploy it to all APIObject? (WebFrameListenerProxy's subclass as you mentioned on IRC)

[Mikhail Pozdnyakov]

(In reply to comment #7)
> (From update of attachment 194269 [details])
> Great change!
> Can you deploy it to all APIObject? (WebFrameListenerProxy's subclass as you mentioned on IRC)

Unfortunately it's not that easy to deploy to WebFrameListenerProxy's subclasses because WebFrameProxy contains RefPtr<WebFrameListenerProxy> m_activeListener; hence WebFrameListenerProxy has to inherit ThreadSafeRefCounted directly.
The only solution that I see is virtual inheritance, would you like it?

[Benjamin Poulain]

Comment on attachment 194269 [details]
patch

> Unfortunately it's not that easy to deploy to WebFrameListenerProxy's subclasses because WebFrameProxy contains RefPtr<WebFrameListenerProxy> m_activeListener; hence WebFrameListenerProxy has to inherit ThreadSafeRefCounted directly.
> The only solution that I see is virtual inheritance, would you like it?

Nope, screw that :)

[WebKit Review Bot]

Comment on attachment 194269 [details]
patch

Clearing flags on attachment: 194269

Committed r147403: <http://trac.webkit.org/changeset/147403>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.