Bug 179673 - Mark WebChromeClient::requestStorageAccess() as final
Summary: Mark WebChromeClient::requestStorageAccess() as final
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-14 10:38 PST by Daniel Bates
Modified: 2017-11-21 17:06 PST (History)
5 users (show)

See Also:


Attachments
Patch (1.59 KB, patch)
2017-11-14 10:41 PST, Daniel Bates
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2017-11-14 10:38:02 PST
Mark WebChromeClient::requestStorageAccess() as final so that we actually call it through a ChromeClient pointer.
Comment 1 Daniel Bates 2017-11-14 10:41:05 PST
Created attachment 326888 [details]
Patch
Comment 2 Build Bot 2017-11-14 10:42:28 PST
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.
Comment 3 Daniel Bates 2017-11-14 12:48:53 PST
Comment on attachment 326888 [details]
Patch

Clearing flags on attachment: 326888

Committed r224835: <https://trac.webkit.org/changeset/224835>
Comment 4 Daniel Bates 2017-11-14 12:48:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Radar WebKit Bug Importer 2017-11-15 09:30:52 PST
<rdar://problem/35561841>
Comment 6 Darin Adler 2017-11-21 09:18:53 PST
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?
Comment 7 Daniel Bates 2017-11-21 17:02:41 PST
(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.
Comment 8 Daniel Bates 2017-11-21 17:06:03 PST
(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