[webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa]

> On Jan 12, 2023, at 9:05 PM, Ryosuke Niwa via webkit-dev <webkit-dev at lists.webkit.org> wrote:
> 
> 
>> On Jan 12, 2023, at 8:37 PM, Darin Adler <darin at apple.com> wrote:
>> 
>>> On Jan 12, 2023, at 9:35 PM, Ryosuke Niwa <rniwa at apple.com> wrote:
>>> 
>>> One alternative is to make bar() return RefPtr although that would be a bit heavy handed in the case of trivial function calls like this: document().frame()->ownerElement()
>> 
>> I don’t quite follow. You just said that all arguments including this need to have RefPtr or something like it.
> 
> Right. The caller should keep an object alive.
> 
>> What makes it OK to call ownerElement without a RefPtr? What is a “trivial function” and how can we tell which functions are the trivial ones?
> 
> We made an exception to “trivial functions” to allow chaining like the one above possible. A trivial function is any inlined accessor of a member variable the form: Document& document() { return m_document.get(); } or Frame* frame() { return m_frame.get(); }
> 
> Anything more complicated than that, or for any out-of-lined functions, we should be storing “this” on a smart pointer type.
> 
> 
> Here’s an example:
> static void printNavigationErrorMessage(Frame& frame, const URL& activeURL, const char* reason)
> {
>     String message = "Unsafe JavaScript attempt to initiate navigation for frame with URL '" + frame.document()->url().string() + "' from frame with URL '" + activeURL.string() + "'. " + reason + "\n";
> 
>     // FIXME: should we print to the console of the document performing the navigation instead?
>     frame.document()->domWindow()->printErrorMessage(message);
> }
> 
> This function, under our rule, should be rewritten like this:
> static void printNavigationErrorMessage(Frame& frame, const URL& activeURL, const char* reason)
> {
>     String message = "Unsafe JavaScript attempt to initiate navigation for frame with URL '" + frame.document()->url().string() + "' from frame with URL '" + activeURL.string() + "'. " + reason + "\n";
> 
>     // FIXME: should we print to the console of the document performing the navigation instead?
>     RefPtr { frame.document()->domWindow() }->printErrorMessage(message);
> }
> 
> Here, it’s okay to call document() without first storing “frame” in a smart pointer because it is a function argument. By transitive property, the caller of this function should be keeping the frame object alive. It’s okay to call domWindow() because it’s a trivial accessor function of the form:
> 
> DOMWindow* domWindow() const { return m_domWindow.get(); }
> 
> But it’s not okay to call printErrorMessage without first storing DOMWindow in a RefPtr because it’s a non-trivial function, and domWindow() was not an argument to this function so there is no caller to guarantee the lifetime of it.
> 
> 
> Here’s another example:
> int Element::clientLeft()
> {
>     document().updateLayoutIgnorePendingStylesheets();
> 
>     if (auto* renderer = renderBox()) {
>         auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
>         return convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft, *renderer).toDouble());
>     }
>     return 0;
> }
> 
> This function should be rewritten to something like this assuming RenderObject is updated to support CheckedPtr:
> int Element::clientLeft()
> {
>     Ref { document() }->updateLayoutIgnorePendingStylesheets();
> 
>     if (CheckedPtr renderer = renderBox()) {
>         auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
>         return convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft, *renderer).toDouble());
>     }
>     return 0;
> }
> 
> Here, we must store document() in Ref before calling updateLayoutIgnorePendingStylesheets because updateLayoutIgnorePendingStylesheets is a non-trivial function, and document() is not one of the arguments. Similarly, we need to store renderBox() in a smart pointer since clientLeft() is a non-trivial function. But we don’t have to store “this” in Ref/RefPtr before calling convertToNonSubpixelValue because “this” is an implicit function argument.

Oops, correction. convertToNonSubpixelValue is a static function which takes double & LegacyCSSOMElementMetricsRoundingStrategy so this is irrelevant there.

- R. Niwa

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20230112/8195c0ae/attachment.htm>