Bug 103164 - Removing unnecessary friend classes in RenderObject: LayoutRepainter, RenderSVGContainer
Summary: Removing unnecessary friend classes in RenderObject: LayoutRepainter, RenderS...
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P3 Normal
Assignee: Nobody
Depends on:
Reported: 2012-11-23 18:37 PST by Adenilson Cavalcanti Silva
Modified: 2012-11-26 15:28 PST (History)
4 users (show)

See Also:

The patch v01 (2.90 KB, patch)
2012-11-23 18:47 PST, Adenilson Cavalcanti Silva
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Implementing reviewers suggestion (location of moved function) (3.18 KB, patch)
2012-11-26 14:27 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adenilson Cavalcanti Silva 2012-11-23 18:37:57 PST
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).
Comment 1 Adenilson Cavalcanti Silva 2012-11-23 18:47:06 PST
Created attachment 175843 [details]
The patch v01
Comment 2 Eric Seidel (no email) 2012-11-23 20:08:34 PST
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.
Comment 3 Adenilson Cavalcanti Silva 2012-11-26 05:03:44 PST

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.
Comment 4 Simon Fraser (smfr) 2012-11-26 13:27:28 PST
Comment on attachment 175843 [details]
The patch v01

Please put the declaration for outlineBoundsForRepaint() just under rectWithOutlineForRepaint().
Comment 5 Eric Seidel (no email) 2012-11-26 13:37:11 PST
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.
Comment 6 Adenilson Cavalcanti Silva 2012-11-26 14:27:45 PST
Created attachment 176068 [details]
Implementing reviewers suggestion (location of moved function)

Now outlineBoundsForRepaint() follows just after rectWithOutlineForRepaint().
Comment 7 WebKit Review Bot 2012-11-26 15:28:44 PST
Comment on attachment 176068 [details]
Implementing reviewers suggestion (location of moved function)

Clearing flags on attachment: 176068

Committed r135779: <http://trac.webkit.org/changeset/135779>
Comment 8 WebKit Review Bot 2012-11-26 15:28:48 PST
All reviewed patches have been landed.  Closing bug.