103164 2012-11-23 18:37:57 -0800 Removing unnecessary friend classes in RenderObject: LayoutRepainter, RenderSVGContainer 2012-11-26 15:28:48 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All RESOLVED FIXED P3 Normal --- 1 savagobr webkit-unassigned eric ojan simon.fraser webkit.review.bot oldest_to_newest 774741 0 savagobr 2012-11-23 18:37:57 -0800 As a continuation of bug #103159, there are two other classes that have unneeded friend access level to RenderObject. RenderSVGContainer was the same case as RenderBox (an one liner patch, simply remove the friend keyword), but LayoutRepainter depended in a call to a protected virtual const function (outlineBoundsForRepaint()). This patch makes the const function public. I've looked also in the remaining classes that are friends of RenderObject, and have some comments with questions as follow below: a) RenderBlock (inherits indirectly from RenderObject) & RenderObjectChildList: they are going to be tricky since depend on calls to some protected members marked as 'dangerous to use!' (i.e. setters for siblings and the parent of the RenderObject). I assume that the approach used for one will work for the other... question: should they remain friends or shall we provide access to the setters via a proxy/bridge class? The second approach would at least keep in a central place the access to the protected/private members of RenderObject. Suggestions? b) RenderLayer whose access is due to an instance of RenderScrollbarPart, I feel this one will be easier to solve (since RenderScrollbarPart IIRC is a RenderBlock). 774742 1 175843 savagobr 2012-11-23 18:47:06 -0800 Created attachment 175843 The patch v01 774754 2 eric 2012-11-23 20:08:34 -0800 I suspect that having RenderLayer be the only possible caller of that function may have been part of the original intention. Still I think having it public is probably a better design. I've CC'd Simon in case he'd like to comment. 775368 3 savagobr 2012-11-26 05:03:44 -0800 Eric Just to confirm, should I send another patch with previous changes *and* make RenderObject's functions (setPreviousSibling, setNextSibling, setParent) no longer protected? That would free the remaining classes from having friend access to RenderObject. 775820 4 175843 simon.fraser 2012-11-26 13:27:28 -0800 Comment on attachment 175843 The patch v01 Please put the declaration for outlineBoundsForRepaint() just under rectWithOutlineForRepaint(). 775834 5 eric 2012-11-26 13:37:11 -0800 This patch is fine as is. I'm not sure it makes sense to make setPreviousSibling, etc. generically public, as those are dangerous functions. It's unclear why any code would need to access those anyway? Thanks for the patch. 775896 6 176068 savagobr 2012-11-26 14:27:45 -0800 Created attachment 176068 Implementing reviewers suggestion (location of moved function) Now outlineBoundsForRepaint() follows just after rectWithOutlineForRepaint(). 775972 7 176068 webkit.review.bot 2012-11-26 15:28:44 -0800 Comment on attachment 176068 Implementing reviewers suggestion (location of moved function) Clearing flags on attachment: 176068 Committed r135779: <http://trac.webkit.org/changeset/135779> 775973 8 webkit.review.bot 2012-11-26 15:28:48 -0800 All reviewed patches have been landed. Closing bug. 175843 2012-11-23 18:47:06 -0800 2012-11-26 14:27:45 -0800 The patch v01 0001-Removing-unnecessary-friend-2classes.patch text/plain 2967 savagobr RnJvbSAzMjBhZjVjNTgwNDUxNjc5MjY1ODM1ZjFlMDk2OGI1MjczYzk0ZmQ2IE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBZGVuaWxzb24gQ2F2YWxjYW50aSA8Y2F2YWxjYW50aWlAZ21h aWwuY29tPgpEYXRlOiBGcmksIDIzIE5vdiAyMDEyIDIyOjQyOjMxIC0wNDAwClN1YmplY3Q6IFtQ QVRDSF0gMiBsZXNzLCAzIGxlZnQuCgpTaWduZWQtb2ZmLWJ5OiBBZGVuaWxzb24gQ2F2YWxjYW50 aSA8Y2F2YWxjYW50aWlAZ21haWwuY29tPgotLS0KIFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyAg ICAgICAgICAgICAgICB8ICAgMTcgKysrKysrKysrKysrKysrKysKIFNvdXJjZS9XZWJDb3JlL3Jl bmRlcmluZy9SZW5kZXJPYmplY3QuaCB8ICAgIDcgKysrLS0tLQogMiBmaWxlcyBjaGFuZ2VkLCAy MCBpbnNlcnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJD b3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBlZThlZWE1Li4w ZGIxZjI0IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNl L1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDUgKzEsMjIgQEAKIDIwMTItMTEtMjMgIEFkZW5pbHNv biBDYXZhbGNhbnRpICA8Y2F2YWxjYW50aWlAZ21haWwuY29tPgogCisgICAgICAgIFJlbW92aW5n IHVubmVjZXNzYXJ5IGZyaWVuZCBjbGFzc2VzIGluIFJlbmRlck9iamVjdDogTGF5b3V0UmVwYWlu dGVyLCBSZW5kZXJTVkdDb250YWluZXIKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTEwMzE2NAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFJlbW92aW5nIHNvbWUgb2YgY2xhc3NlcyBtYXJrZWQgYXMgZnJpZW5k IG9mIFJlbmRlck9iamVjdC4gVGhpcyBwYXRjaCBzb2x2ZXMgdGhpcyBpc3N1ZQorICAgICAgICBm b3IgMiBjbGFzc2VzOiBSZW5kZXJTVkdDb250YWluZXIgKHRoYXQgaXMgZGVyaXZlZCBmcm9tIFJl bmRlck9iamVjdCkgYW5kIExheW91dFJlcGFpbnRlcgorICAgICAgICAodGhhdCBhY2Nlc3NlcyBv bmUgY29uc3QgbWVtYmVyIGZ1bmN0aW9uIGluIFJlbmRlck9iamVjdCB0aGF0IGlzIG5vdyBtYWRl IHB1YmxpYykuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzLCBubyBjaGFuZ2VzIGluIGZ1bmN0aW9u YWxpdHkuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVyT2JqZWN0Lmg6CisgICAgICAgIChS ZW5kZXJPYmplY3QpOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyT2JqZWN0OjpvdXRsaW5lQm91 bmRzRm9yUmVwYWludCk6CisKKzIwMTItMTEtMjMgIEFkZW5pbHNvbiBDYXZhbGNhbnRpICA8Y2F2 YWxjYW50aWlAZ21haWwuY29tPgorCiAgICAgICAgIFJlbW92aW5nIHVubmVjZXNzYXJ5IFJlbmRl ckJveCBmcmllbmQgYWNjZXNzIGluIFJlbmRlck9iamVjdAogICAgICAgICBodHRwczovL2J1Z3Mu d2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAzMTU5CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJPYmplY3QuaCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmlu Zy9SZW5kZXJPYmplY3QuaAppbmRleCA5ZmZiY2ZiLi4zZTMyZTlmIDEwMDY0NAotLS0gYS9Tb3Vy Y2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmgKKysrIGIvU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL1JlbmRlck9iamVjdC5oCkBAIC0xNTUsMTEgKzE1NSw5IEBAIGNvbnN0IGludCBz aG93VHJlZUNoYXJhY3Rlck9mZnNldCA9IDM5OwogCiAvLyBCYXNlIGNsYXNzIGZvciBhbGwgcmVu ZGVyaW5nIHRyZWUgb2JqZWN0cy4KIGNsYXNzIFJlbmRlck9iamVjdCA6IHB1YmxpYyBDYWNoZWRJ bWFnZUNsaWVudCB7Ci0gICAgZnJpZW5kIGNsYXNzIExheW91dFJlcGFpbnRlcjsKICAgICBmcmll bmQgY2xhc3MgUmVuZGVyQmxvY2s7CiAgICAgZnJpZW5kIGNsYXNzIFJlbmRlckxheWVyOwogICAg IGZyaWVuZCBjbGFzcyBSZW5kZXJPYmplY3RDaGlsZExpc3Q7Ci0gICAgZnJpZW5kIGNsYXNzIFJl bmRlclNWR0NvbnRhaW5lcjsKIHB1YmxpYzoKICAgICAvLyBBbm9ueW1vdXMgb2JqZWN0cyBzaG91 bGQgcGFzcyB0aGUgZG9jdW1lbnQgYXMgdGhlaXIgbm9kZSwgYW5kIHRoZXkgd2lsbCB0aGVuIGF1 dG9tYXRpY2FsbHkgYmUKICAgICAvLyBtYXJrZWQgYXMgYW5vbnltb3VzIGluIHRoZSBjb25zdHJ1 Y3Rvci4KQEAgLTk1Niw2ICs5NTQsOSBAQCBwdWJsaWM6CiAKICAgICBSZXNwZWN0SW1hZ2VPcmll bnRhdGlvbkVudW0gc2hvdWxkUmVzcGVjdEltYWdlT3JpZW50YXRpb24oKSBjb25zdDsKIAorICAg IHZpcnR1YWwgTGF5b3V0UmVjdCBvdXRsaW5lQm91bmRzRm9yUmVwYWludChjb25zdCBSZW5kZXJM YXllck1vZGVsT2JqZWN0KiAvKnJlcGFpbnRDb250YWluZXIqLywgY29uc3QgUmVuZGVyR2VvbWV0 cnlNYXAqID0gMCkgY29uc3QgeyByZXR1cm4gTGF5b3V0UmVjdCgpOyB9CisKKwogcHJvdGVjdGVk OgogICAgIGlubGluZSBib29sIGxheWVyQ3JlYXRpb25BbGxvd2VkRm9yU3VidHJlZSgpIGNvbnN0 OwogCkBAIC05ODAsOCArOTgxLDYgQEAgcHJvdGVjdGVkOgogICAgIHZpcnR1YWwgdm9pZCB3aWxs QmVEZXN0cm95ZWQoKTsKICAgICB2b2lkIGFyZW5hRGVsZXRlKFJlbmRlckFyZW5hKiwgdm9pZCog b2JqZWN0QmFzZSk7CiAKLSAgICB2aXJ0dWFsIExheW91dFJlY3Qgb3V0bGluZUJvdW5kc0ZvclJl cGFpbnQoY29uc3QgUmVuZGVyTGF5ZXJNb2RlbE9iamVjdCogLypyZXBhaW50Q29udGFpbmVyKi8s IGNvbnN0IFJlbmRlckdlb21ldHJ5TWFwKiA9IDApIGNvbnN0IHsgcmV0dXJuIExheW91dFJlY3Qo KTsgfQotCiAgICAgdmlydHVhbCBib29sIGNhbkJlUmVwbGFjZWRXaXRoSW5saW5lUnVuSW4oKSBj b25zdDsKIAogICAgIHZpcnR1YWwgdm9pZCBpbnNlcnRlZEludG9UcmVlKCk7Ci0tIAoxLjcuMTAu NAoK 176068 2012-11-26 14:27:45 -0800 2012-11-26 15:28:44 -0800 Implementing reviewers suggestion (location of moved function) 0002-Removing-unnecessary-friend-2classes.patch text/plain 3253 savagobr RnJvbSBmYmM1MWM5N2ZmM2JiNzc4NjdlMzQ3ZTAyZmRlY2NhNjZiMTdkNDkzIE1vbiBTZXAgMTcg MDA6MDA6MDAgMjAwMQpGcm9tOiBBZGVuaWxzb24gQ2F2YWxjYW50aSA8Y2F2YWxjYW50aWlAZ21h aWwuY29tPgpEYXRlOiBGcmksIDIzIE5vdiAyMDEyIDIyOjQyOjMxIC0wNDAwClN1YmplY3Q6IFtQ QVRDSF0gTGVzcyAyIGZyaWVuZHMsIGltcGxlbWVudCByZXZpZXdlcnMgY29tbWVudHMuCgpTaWdu ZWQtb2ZmLWJ5OiBBZGVuaWxzb24gQ2F2YWxjYW50aSA8Y2F2YWxjYW50aWlAZ21haWwuY29tPgot LS0KIFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyAgICAgICAgICAgICAgICB8ICAgMTcgKysrKysr KysrKysrKysrKysKIFNvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJPYmplY3QuaCB8ICAg IDUgKy0tLS0KIDIgZmlsZXMgY2hhbmdlZCwgMTggaW5zZXJ0aW9ucygrKSwgNCBkZWxldGlvbnMo LSkKCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKaW5kZXggNmI3YmYzMC4uOWViZTUwOCAxMDA2NDQKLS0tIGEvU291cmNlL1dl YkNvcmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsx LDIwIEBACisyMDEyLTExLTIzICBBZGVuaWxzb24gQ2F2YWxjYW50aSAgPGNhdmFsY2FudGlpQGdt YWlsLmNvbT4KKworICAgICAgICBSZW1vdmluZyB1bm5lY2Vzc2FyeSBmcmllbmQgY2xhc3NlcyBp biBSZW5kZXJPYmplY3Q6IExheW91dFJlcGFpbnRlciwgUmVuZGVyU1ZHQ29udGFpbmVyCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDMxNjQKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBSZW1vdmluZyBzb21l IG9mIGNsYXNzZXMgbWFya2VkIGFzIGZyaWVuZCBvZiBSZW5kZXJPYmplY3QuIFRoaXMgcGF0Y2gg c29sdmVzIHRoaXMgaXNzdWUKKyAgICAgICAgZm9yIDIgY2xhc3NlczogUmVuZGVyU1ZHQ29udGFp bmVyICh0aGF0IGlzIGRlcml2ZWQgZnJvbSBSZW5kZXJPYmplY3QpIGFuZCBMYXlvdXRSZXBhaW50 ZXIKKyAgICAgICAgKHRoYXQgYWNjZXNzZXMgb25lIGNvbnN0IG1lbWJlciBmdW5jdGlvbiBpbiBS ZW5kZXJPYmplY3QgdGhhdCBpcyBub3cgbWFkZSBwdWJsaWMpLgorCisgICAgICAgIE5vIG5ldyB0 ZXN0cywgbm8gY2hhbmdlcyBpbiBmdW5jdGlvbmFsaXR5LgorCisgICAgICAgICogcmVuZGVyaW5n L1JlbmRlck9iamVjdC5oOgorICAgICAgICAoUmVuZGVyT2JqZWN0KToKKyAgICAgICAgKFdlYkNv cmU6OlJlbmRlck9iamVjdDo6b3V0bGluZUJvdW5kc0ZvclJlcGFpbnQpOgorCiAyMDEyLTExLTI2 ICBQcmF0aWsgU29sYW5raSAgPHBzb2xhbmtpQGFwcGxlLmNvbT4KIAogICAgICAgICBBZGQgUmVz b3VyY2VCdWZmZXI6OmFwcGVuZChDRkRhdGFSZWYpIHRvIGdldCBjb2RlIHRvIGNvbXBpbGUgd2l0 aCBVU0UoTkVUV09SS19DRkRBVEFfQVJSQVlfQ0FMTEJBQ0spCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyT2JqZWN0LmggYi9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJp bmcvUmVuZGVyT2JqZWN0LmgKaW5kZXggOWZmYmNmYi4uZTA4MjFjZiAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYkNvcmUvcmVuZGVyaW5nL1JlbmRlck9iamVjdC5oCisrKyBiL1NvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9SZW5kZXJPYmplY3QuaApAQCAtMTU1LDExICsxNTUsOSBAQCBjb25zdCBpbnQg c2hvd1RyZWVDaGFyYWN0ZXJPZmZzZXQgPSAzOTsKIAogLy8gQmFzZSBjbGFzcyBmb3IgYWxsIHJl bmRlcmluZyB0cmVlIG9iamVjdHMuCiBjbGFzcyBSZW5kZXJPYmplY3QgOiBwdWJsaWMgQ2FjaGVk SW1hZ2VDbGllbnQgewotICAgIGZyaWVuZCBjbGFzcyBMYXlvdXRSZXBhaW50ZXI7CiAgICAgZnJp ZW5kIGNsYXNzIFJlbmRlckJsb2NrOwogICAgIGZyaWVuZCBjbGFzcyBSZW5kZXJMYXllcjsKICAg ICBmcmllbmQgY2xhc3MgUmVuZGVyT2JqZWN0Q2hpbGRMaXN0OwotICAgIGZyaWVuZCBjbGFzcyBS ZW5kZXJTVkdDb250YWluZXI7CiBwdWJsaWM6CiAgICAgLy8gQW5vbnltb3VzIG9iamVjdHMgc2hv dWxkIHBhc3MgdGhlIGRvY3VtZW50IGFzIHRoZWlyIG5vZGUsIGFuZCB0aGV5IHdpbGwgdGhlbiBh dXRvbWF0aWNhbGx5IGJlCiAgICAgLy8gbWFya2VkIGFzIGFub255bW91cyBpbiB0aGUgY29uc3Ry dWN0b3IuCkBAIC04MTEsNiArODA5LDcgQEAgcHVibGljOgogICAgIEludFJlY3QgcGl4ZWxTbmFw cGVkQWJzb2x1dGVDbGlwcGVkT3ZlcmZsb3dSZWN0KCkgY29uc3Q7CiAgICAgdmlydHVhbCBMYXlv dXRSZWN0IGNsaXBwZWRPdmVyZmxvd1JlY3RGb3JSZXBhaW50KGNvbnN0IFJlbmRlckxheWVyTW9k ZWxPYmplY3QqIHJlcGFpbnRDb250YWluZXIpIGNvbnN0OwogICAgIHZpcnR1YWwgTGF5b3V0UmVj dCByZWN0V2l0aE91dGxpbmVGb3JSZXBhaW50KGNvbnN0IFJlbmRlckxheWVyTW9kZWxPYmplY3Qq IHJlcGFpbnRDb250YWluZXIsIExheW91dFVuaXQgb3V0bGluZVdpZHRoKSBjb25zdDsKKyAgICB2 aXJ0dWFsIExheW91dFJlY3Qgb3V0bGluZUJvdW5kc0ZvclJlcGFpbnQoY29uc3QgUmVuZGVyTGF5 ZXJNb2RlbE9iamVjdCogLypyZXBhaW50Q29udGFpbmVyKi8sIGNvbnN0IFJlbmRlckdlb21ldHJ5 TWFwKiA9IDApIGNvbnN0IHsgcmV0dXJuIExheW91dFJlY3QoKTsgfQogCiAgICAgLy8gR2l2ZW4g YSByZWN0IGluIHRoZSBvYmplY3QncyBjb29yZGluYXRlIHNwYWNlLCBjb21wdXRlIGEgcmVjdCBz dWl0YWJsZSBmb3IgcmVwYWludGluZwogICAgIC8vIHRoYXQgcmVjdCBpbiB2aWV3IGNvb3JkaW5h dGVzLgpAQCAtOTgwLDggKzk3OSw2IEBAIHByb3RlY3RlZDoKICAgICB2aXJ0dWFsIHZvaWQgd2ls bEJlRGVzdHJveWVkKCk7CiAgICAgdm9pZCBhcmVuYURlbGV0ZShSZW5kZXJBcmVuYSosIHZvaWQq IG9iamVjdEJhc2UpOwogCi0gICAgdmlydHVhbCBMYXlvdXRSZWN0IG91dGxpbmVCb3VuZHNGb3JS ZXBhaW50KGNvbnN0IFJlbmRlckxheWVyTW9kZWxPYmplY3QqIC8qcmVwYWludENvbnRhaW5lciov LCBjb25zdCBSZW5kZXJHZW9tZXRyeU1hcCogPSAwKSBjb25zdCB7IHJldHVybiBMYXlvdXRSZWN0 KCk7IH0KLQogICAgIHZpcnR1YWwgYm9vbCBjYW5CZVJlcGxhY2VkV2l0aElubGluZVJ1bkluKCkg Y29uc3Q7CiAKICAgICB2aXJ0dWFsIHZvaWQgaW5zZXJ0ZWRJbnRvVHJlZSgpOwotLSAKMS43LjEw LjQKCg==