[webkit-reviews] review granted: [Bug 175384] Implement font-display loading behaviors : [Attachment 322247] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 175384: Implement font-display loading behaviors
https://bugs.webkit.org/show_bug.cgi?id=175384

Attachment 322247: Patch

https://bugs.webkit.org/attachment.cgi?id=322247&action=review




--- Comment #25 from Darin Adler <darin at apple.com> ---
Comment on attachment 322247
  --> https://bugs.webkit.org/attachment.cgi?id=322247
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322247&action=review

Normally for testing we don’t use magic numeric values. It’s more common to use
strings in JavaScript to specify such things. I think that would be better
rather than these special numbers. For one thing, the numbers will all all have
to be renumbered if the CSS specification adds a new one.

> Source/WebCore/css/CSSFontFace.cpp:501
> +	   // Don't cancel the font download, so that it might exist upon the
next page the user navigates to.

I think the wording “so that it might exist upon” is strange here. It would be
good to write this out with more specific language. Are we saying that we
should finish the font download so it gets into the cache? Or is there some
more precise way of talking about what’s good about finishing the download?
Exactly what is the benefit when the next page uses the same font? Also, will
the download continue indefinitely? Will it stop when we leave this page?

I understand that the page-specified time out needn’t cancel the download. But
will the download ever get canceled if it takes a really long time?

More precise language in this comment could help me understand what I think of
the policy. And might help you clarify to others what your rationale is. Lets
word it clearly enough that someone can disagree. I hope, though, it doesn’t
turn into a paragraph.

> Source/WebCore/css/CSSFontFace.cpp:515
> +    if (status() == Status::Failure)
> +	   return true;

Confusing to add this here and not change the name of the function. Seems like
this can now return true when it’s not true that all sources failed.

> Source/WebCore/css/CSSFontFace.cpp:618
> +    struct TimeoutValues {
> +	   Seconds blockPeriod;
> +	   Seconds swapPeriod;
> +    } timeoutValues[] = {
> +	   { 3_s, Seconds(std::numeric_limits<double>::infinity()) },
> +	   { 3_s, Seconds(std::numeric_limits<double>::infinity()) },
> +	   { 0_s, Seconds(std::numeric_limits<double>::infinity()) },
> +	   { 0.1_s, 3_s },
> +	   { 0.1_s, 0_s },
> +	   // Debugging values below
> +	   { Seconds(std::numeric_limits<double>::infinity()), 0_s },
> +	   { 0_s, Seconds(std::numeric_limits<double>::infinity()) },
> +	   { 0_s, 0_s },
> +    };
> +    auto index =
fontTimeoutIndex().value_or(static_cast<unsigned>(m_loadingBehavior));
> +    const TimeoutValues& values = timeoutValues[index];

The timeoutValues array should probably be declared const or constexpr. Should
write Seconds::infinity() instead of that longer thing.

I don’t see anything here that checks that the length of the array matches the
number of values in the enumeration, and there’s also no check at runtime: I
don’t like the idea that a bad value for the enum could result in a buffer
overrun. Is there a clean way to tighten those two things up? I’d use auto& for
the the of "values".

I don’t think the name values is really great for the timeouts. Makes the code
below a bit hard to read.

> Source/WebCore/css/CSSFontFace.cpp:625
> +	   if (values.blockPeriod > 0_s && values.blockPeriod <
Seconds(std::numeric_limits<double>::infinity()))

Should write Seconds::infinity() instead of that longer thing. Could also
consider using isfinite or !isinf instead.

> Source/WebCore/css/CSSFontFace.cpp:626
> +	       m_timeoutTimer.startOneShot(values.blockPeriod);

Do we need an else that stops the timer? What guarantees we don’t already have
one going?

> Source/WebCore/css/CSSFontFace.cpp:629
> +	   if (values.swapPeriod > 0_s && values.swapPeriod <
Seconds(std::numeric_limits<double>::infinity()))

Ditto.

> Source/WebCore/css/CSSFontFace.cpp:630
> +	       m_timeoutTimer.startOneShot(values.swapPeriod);

Ditto.

> Source/WebCore/css/CSSFontFace.cpp:644
> +    // The state transition across a 0-second timer should be synchronous.

Comments needs to say "why". This comment should both say why this is desirable
(suggested behavior in the specification? helpful for testing? important to
user experience?) and maybe also why it is OK to do the work synchronously. In
other contexts it has proven quite important to not do things synchronously so
that side effects don’t cause infinite recursion. Why is that not an issue
here?

There is also a fragile dependency between code above, which checks this same
condition and does not set a timer, and the code here. Would be cleaner if
there were not two separate pieces of code that both need to implement the
policy of doing things synchronously. I believe that we could move all the
timer management down here so that we could either start the timer or make the
synchronous call in the same place. I don’t see any reason the timer has to be
started/stopped before calling out to the clients.

Could also factor things so we did a better job sharing the logic about moving
forward from one state to the next; the only difference between loading ->
timed-out and timed-out -> failure here is which time constant is used and what
status values are involved.

> Source/WebCore/css/CSSFontFace.cpp:748
> +    if (allSourcesFailed())
> +	   return nullptr;

Why is this needed? The loop below will do nothing if the sources have already
failed. Maybe because allSourcesFailed has been changed to check something
different?

> Source/WebCore/css/CSSFontFace.h:152
> +    std::optional<unsigned> fontTimeoutIndex() const;

I don’t think anything makes it clear that this is for testing only nor is the
concept of a "font timeout index" really a clear one; seems very specific to
using an enumeration as an index into a table inside one function, not a good
abstraction or concept for what we are specifying.

To cite an example, the name fontLoadingTimingOverrideForTesting would make it
clear.

Since we don’t want testing to race, it’s also pretty clear that there are only
a few values that are useful for our testing. Zero, infinity, not sure what
else.

> Source/WebCore/css/CSSSegmentedFontFace.cpp:72
> +	       // We need to retain the intermediate results. Otherwise, we get
use-after-free.

I think this comment needs a rewrite.

- I suggest writing the comment in terms of needing an owner for the result. We
need to store a reference to the object since callers will not take ownership
and it expects us to own it and expects the pointer to be valid as long as the
font face object is alive. At least I think that’s what callers expect here.
Unclear in fact.

- I think it’s a mess to Ref all the intermediate results we have ever
returned. For one thing, could easily be a waste of memory! Can we come up with
a different design? Maybe we can change the font function to return
RefPtr<Font> or Ref<Font> instead of extending the lifetime ourselves by
keeping a vector of everything we have ever returned.

- The word retain is part of Objective-C terminology for this, not our
Ref/RefPtr terminology.

- "otherwise, we get use-after-free" is not the clearest way to put it; we
could say that about any object lifetime mistake and I think it’s a bit
excessively dramatic