179673 2017-11-14 10:38:02 -0800 Mark WebChromeClient::requestStorageAccess() as final 2017-11-21 17:06:03 -0800 1 1 1 Unclassified WebKit WebKit Misc. WebKit Local Build All All RESOLVED FIXED InRadar P2 Normal --- 1 dbates dbates buildbot darin sam webkit-bug-importer wilander oldest_to_newest 1371432 0 dbates 2017-11-14 10:38:02 -0800 Mark WebChromeClient::requestStorageAccess() as final so that we actually call it through a ChromeClient pointer. 1371436 1 326888 dbates 2017-11-14 10:41:05 -0800 Created attachment 326888 Patch 1371437 2 buildbot 2017-11-14 10:42:28 -0800 Attachment 326888 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. 1371525 3 326888 dbates 2017-11-14 12:48:53 -0800 Comment on attachment 326888 Patch Clearing flags on attachment: 326888 Committed r224835: <https://trac.webkit.org/changeset/224835> 1371526 4 dbates 2017-11-14 12:48:55 -0800 All reviewed patches have been landed. Closing bug. 1371856 5 webkit-bug-importer 2017-11-15 09:30:52 -0800 <rdar://problem/35561841> 1374053 6 326888 darin 2017-11-21 09:18:53 -0800 Comment on attachment 326888 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? 1374116 7 dbates 2017-11-21 17:02:41 -0800 (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. 1374118 8 dbates 2017-11-21 17:06:03 -0800 (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 326888 2017-11-14 10:41:05 -0800 2017-11-14 12:48:53 -0800 Patch bug-179673-20171114104104.patch text/plain 1628 dbates U3VidmVyc2lvbiBSZXZpc2lvbjogMjI0ODE2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IDllMjM2Mzg4MDAyODE1NGVl ZmE4ZTNhN2U2NGViMmZhY2Y2MWI0YTUuLjQyZTc1MmM1NzIyMzI1NGU2OTZmNDJjODU2NTQ4MzBi MzkxOTk1YzAgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTctMTEtMTQgIERhbmllbCBC YXRlcyAgPGRhYmF0ZXNAYXBwbGUuY29tPgorCisgICAgICAgIE1hcmsgV2ViQ2hyb21lQ2xpZW50 OjpyZXF1ZXN0U3RvcmFnZUFjY2VzcygpIGFzIGZpbmFsCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNzk2NzMKKworICAgICAgICBSZXZpZXdlZCBieSBO T0JPRFkgKE9PUFMhKS4KKworICAgICAgICBNYXJrIFdlYkNocm9tZUNsaWVudDo6cmVxdWVzdFN0 b3JhZ2VBY2Nlc3MoKSBhcyBmaW5hbCBzbyB0aGF0IGl0IG92ZXJyaWRlcyB0aGUgdmlydHVhbCBm dW5jdGlvbgorICAgICAgICBpbiBDaHJvbWVDbGllbnQgd2l0aCB0aGUgc2FtZSBuYW1lIGFuZCBo ZW5jZSB3ZSBhY3R1YWxseSBjYWxsIGl0IHRocm91Z2ggYSBDaHJvbWVDbGllbnQgcG9pbnRlci4K KworICAgICAgICAqIFdlYlByb2Nlc3MvV2ViQ29yZVN1cHBvcnQvV2ViQ2hyb21lQ2xpZW50Lmg6 CisKIDIwMTctMTEtMTQgIEJyZW50IEZ1bGdoYW0gIDxiZnVsZ2hhbUBhcHBsZS5jb20+CiAKICAg ICAgICAgUkVHUkVTU0lPTihyMjI0Nzk5KTogV2ViS2l0IGNyYXNoZXMgb24gU2llcnJhIGR1ZSB0 byBzYW5kYm94IHZpb2xhdGlvbgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdC9XZWJQcm9jZXNz L1dlYkNvcmVTdXBwb3J0L1dlYkNocm9tZUNsaWVudC5oIGIvU291cmNlL1dlYktpdC9XZWJQcm9j ZXNzL1dlYkNvcmVTdXBwb3J0L1dlYkNocm9tZUNsaWVudC5oCmluZGV4IDY5MWFlZDc5ZTUxMWY2 YTg2MzE2NzI0ZjA3YmVhZmI4NWY0M2U4NjEuLmI0OTg4MzA4ZTI5YjA5NDA5OTliMjExMWM3ZWQx ZTZhNGJjMGVhZWIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvV2ViUHJvY2Vzcy9XZWJDb3Jl U3VwcG9ydC9XZWJDaHJvbWVDbGllbnQuaAorKysgYi9Tb3VyY2UvV2ViS2l0L1dlYlByb2Nlc3Mv V2ViQ29yZVN1cHBvcnQvV2ViQ2hyb21lQ2xpZW50LmgKQEAgLTM0Miw3ICszNDIsNyBAQCBwcml2 YXRlOgogCiAgICAgdm9pZCBkaWRJbnZhbGlkYXRlRG9jdW1lbnRNYXJrZXJSZWN0cygpIGZpbmFs OwogCi0gICAgdm9pZCByZXF1ZXN0U3RvcmFnZUFjY2VzcyhTdHJpbmcmJiBzdWJGcmFtZUhvc3Qs IFN0cmluZyYmIHRvcEZyYW1lSG9zdCwgV1RGOjpGdW5jdGlvbjx2b2lkIChib29sKT4mJik7Cisg ICAgdm9pZCByZXF1ZXN0U3RvcmFnZUFjY2VzcyhTdHJpbmcmJiBzdWJGcmFtZUhvc3QsIFN0cmlu ZyYmIHRvcEZyYW1lSG9zdCwgV1RGOjpGdW5jdGlvbjx2b2lkIChib29sKT4mJikgZmluYWw7CiAK ICAgICBTdHJpbmcgbV9jYWNoZWRUb29sVGlwOwogICAgIG11dGFibGUgUmVmUHRyPFdlYkZyYW1l PiBtX2NhY2hlZEZyYW1lU2V0TGFyZ2VzdEZyYW1lOwo=