> 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.
- R. Niwa
_______________________________________________
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev
