> On Jan 12, 2023, at 9:05 PM, Ryosuke Niwa via webkit-dev
> <webkit-dev@lists.webkit.org> wrote:
>
>
>> On Jan 12, 2023, at 8:37 PM, Darin Adler <da...@apple.com> wrote:
>>
>>> On Jan 12, 2023, at 9:35 PM, Ryosuke Niwa <rn...@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
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
