[webkit-reviews] review granted: [Bug 208351] CSSFontFace should not need its m_fontSelector data member : [Attachment 420115] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Chris Lord
<clord at igalia.com>'s request for review:
Bug 208351: CSSFontFace should not need its m_fontSelector data member
https://bugs.webkit.org/show_bug.cgi?id=208351

Attachment 420115: Patch

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




--- Comment #7 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 420115
  --> https://bugs.webkit.org/attachment.cgi?id=420115
Patch

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

Thank you!

> Source/WebCore/css/CSSFontFace.cpp:81
> +		      
document->fontSelector().willBeginLoadingFontSoon(*cachedFont);

Pretty sure document can be null here. See line 77 above. I think this occurs
when the page uses the CSS Font Loading API to construct a FontFace object from
script.

It's a bit scary that this didn't cause any tests to fail. I suggest either 1)
adding a test which fails without a null check here, or 2) if it is impossible
create such a test, modifying the signature of this function to take a
Document& instead of a Document*.

> Source/WebCore/css/CSSFontFace.cpp:591
> +		   source->load(document());

I thought we were going to migrate loading code to use ThreadableLoader instead
of CachedResourceLoader?

> Source/WebCore/css/CSSFontSelector.cpp:376
> +    font.removeClient(*this);

This is a bit of a scary place to be doing this. I understand why it's correct,
but it seems a bit brittle; we may want to add additional messages between the
CachedFont and the CSSFontSelector, and it would be surprising if these
messages stopped working just because the font loaded. What's the downside to
instead running this line in the destructor of the FontSelector? That's what
CSSFontFaceSource does.

This is relevant because we're interested in adding streaming support for
fonts, which may involve additional messages between the CachedFont and the
CSSFontSelector.

> Source/WebCore/css/CSSFontSelector.h:113
> +    using CachedFontClient::fontLoaded;

Why is this necessary?