Bug 179673 – Mark WebChromeClient::requestStorageAccess() as final

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

Mark WebChromeClient::requestStorageAccess() as final so that we actually call it through a ChromeClient pointer.

[Daniel Bates]

Created attachment 326888 [details]
Patch

[Build Bot]

Attachment 326888 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.h:345:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 326888 [details]
Patch

Clearing flags on attachment: 326888

Committed r224835: <https://trac.webkit.org/changeset/224835>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35561841>

[Darin Adler]

Comment on attachment 326888 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        Mark WebChromeClient::requestStorageAccess() as final so that it overrides the virtual function
> +        in ChromeClient with the same name and hence we actually call it through a ChromeClient pointer.

This comment doesn’t make sense to me. If a function signature matches, then it will override the virtual function. It doesn’t need to be marked "final" or "override" to do that.

It’s OK to mark this final so that we’d get an error if the function signature didn’t match, and so that if it’s called directly it can use a non-polymorphic function call.

On the other hand, I would have expected to get a warning if this is overriding and not marked final in a class that uses override and final. Maybe I don’t understand how the clang warning works. Maybe it works better if you use just override and not final?

[Daniel Bates]

(In reply to Darin Adler from comment #6)
> Comment on attachment 326888 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326888&action=review
> 
> > Source/WebKit/ChangeLog:9
> > +        Mark WebChromeClient::requestStorageAccess() as final so that it overrides the virtual function
> > +        in ChromeClient with the same name and hence we actually call it through a ChromeClient pointer.
> 
> This comment doesn’t make sense to me. 

You’re right! This is nonsensical. I was trying to convey that this function could hide the virtual function of the same name if it is not marked final (or override) and its signature changes from that of the base class virtual function. And that this bug could be concealed if all callers of this function called through a derived class pointer. You expressed this more succinctly in the first sentence of your third paragraph.

> If a function signature matches, then
> it will override the virtual function. It doesn’t need to be marked "final"
> or "override" to do that.
> 
> It’s OK to mark this final so that we’d get an error if the function
> signature didn’t match, and so that if it’s called directly it can use a
> non-polymorphic function call.
> 
> On the other hand, I would have expected to get a warning if this is
> overriding and not marked final in a class that uses override and final.

I expected this too. I did not see any bot failures on build WebKit.org at the time I posted this patch. However my version of clang did emit a warning about this function. Therefore I posted this patch. I was building with Apple internal software so maybe the calling code is guarded by some kind of macro guard that is enabled for me or maybe there is a bug in the clang version we are using on build.webkit.org/EWS. I did not check. I am not near a computer to check at the moment.

> Maybe I don’t understand how the clang warning works. Maybe it works better
> if you use just override and not final?

It should work the same. With regards to using override vs final, I like using final to convey that we do not expect further overrides (at the time of writing) and to allow the compiler to replace the call with a non-polymorphic function call as you pointed out above.

[Daniel Bates]

(In reply to Daniel Bates from comment #7)
> (In reply to Darin Adler from comment #6)
> > Comment on attachment 326888 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=326888&action=review
> > 
> > > Source/WebKit/ChangeLog:9
> > > +        Mark WebChromeClient::requestStorageAccess() as final so that it overrides the virtual function
> > > +        in ChromeClient with the same name and hence we actually call it through a ChromeClient pointer.
> > 
> > This comment doesn’t make sense to me. 
> 
> [...]
> You expressed this more succinctly in the first sentence of your third paragraph.
> 

*more succinctly and correctly phrased