Bug 51863 – [chromium] WEBKIT_API and styling fixes for the chromium api.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[David Levin]

See summary.

Making some fixes according to https://lists.webkit.org/pipermail/webkit-dev/2010-December/015188.html

[David Levin]

Created attachment 77864 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 77864 [details]
Patch

These look right to me.

[Darin Fisher (:fishd, Google)]

Comment on attachment 77864 [details]
Patch

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

> WebKit/chromium/public/WebDOMEventListener.h:51
> +    virtual ~WebDOMEventListener();

this will break the build.  because the implementation is provided by
a .cpp file and callable by chromium, it needs to be marked WEBKIT_API.

it is rather unusual for us to have virtual destructors in the API like
this.  the destructor needs to be virtual because this is an interface
meant to be subclassed.  to keep the rules simple, we could just have
this be implemented inline to a private method, and then that private
method would be marked WEBKIT_API.

[David Levin]

(In reply to comment #3)
> (From update of attachment 77864 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77864&action=review
> 
> > WebKit/chromium/public/WebDOMEventListener.h:51
> > +    virtual ~WebDOMEventListener();
> 
> this will break the build.  because the implementation is provided by
> a .cpp file and callable by chromium, it needs to be marked WEBKIT_API.
> 
> it is rather unusual for us to have virtual destructors in the API like
> this.  the destructor needs to be virtual because this is an interface
> meant to be subclassed.  to keep the rules simple, we could just have
> this be implemented inline to a private method, and then that private
> method would be marked WEBKIT_API.

According to https://lists.webkit.org/pipermail/webkit-dev/2010-December/015188.html, virtual functions should not have WEBKIT_API. Why doesn't that apply in this case?

[David Levin]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 77864 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=77864&action=review
> > 
> > > WebKit/chromium/public/WebDOMEventListener.h:51
> > > +    virtual ~WebDOMEventListener();
> > 
> > this will break the build.  because the implementation is provided by
> > a .cpp file and callable by chromium, it needs to be marked WEBKIT_API.
> > 
> > it is rather unusual for us to have virtual destructors in the API like
> > this.  the destructor needs to be virtual because this is an interface
> > meant to be subclassed.  to keep the rules simple, we could just have
> > this be implemented inline to a private method, and then that private
> > method would be marked WEBKIT_API.
> 
> According to https://lists.webkit.org/pipermail/webkit-dev/2010-December/015188.html, virtual functions should not have WEBKIT_API. Why doesn't that apply in this case?

Perhaps this is it:

virtual functions should have WEBKIT_API if a derived implementation may be implemented outside of WebKit, and it may want to call the base implementation and that is the case for WebDOMEventListener?

[Darin Fisher (:fishd, Google)]

(In reply to comment #4)
> According to https://lists.webkit.org/pipermail/webkit-dev/2010-December/015188.html, virtual functions should not have WEBKIT_API. Why doesn't that apply in this case?

Sorry, my expectation when writing that rule was that all virtual functions would have inline implementations.  It is a rare case that they do not.

To make the rules easier to follow, how about we make the virtual ~WebDOMEventListener have an inline implementation that calls a private WEBKIT_API method, named 'reset' and similarly have the constructor call an 'initialize' method.  The constructor should null out m_private before calling initialize.

[David Levin]

Created attachment 77927 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 77927 [details]
Patch

Clearing flags on attachment: 77927

Committed r75106: <http://trac.webkit.org/changeset/75106>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.