[webkit-reviews] review granted: [Bug 229644] document.fonts.size needs to update style so it doesn't return stale values : [Attachment 436923] 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 229644: document.fonts.size needs to update style so it doesn't return
stale values
https://bugs.webkit.org/show_bug.cgi?id=229644

Attachment 436923: Patch

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




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

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

> Source/WebCore/css/CSSFontFaceSet.cpp:108
> +    auto protect = makeRef(const_cast<CSSFontFaceSet&>(*this));

Self-protecting is not the correct pattern for our reference counting. Instead
callers should be protecting the pointers of objects before they call
functions. Please talk to Ryosuke about this if you don’t agree.

Also, we don’t need makeRef for this any more; if we wanted a "protector" we
could write:

    Ref protect = const_cast<CSSFontFaceSet&>(*this);

But probably won’t be doing that.

> Source/WebCore/css/CSSFontFaceSet.cpp:110
> +	   m_owningFontSelector->fontStyleUpdateNeeded();

Names here don’t make sense to me. This function’s name says "after updating
style" which sounds asynchronous, but the name of the function we are calling
says "update needed", which sounds like a request to schedule something in the
future, asynchronously or lazily.

> Source/WebCore/css/CSSFontSelector.h:92
> +    void fontStyleUpdateNeeded();

Should rename this so it’s clear it’s a synchronous operation, not a passive
notification that triggers something lazy in the future (as mentioned above).

> Source/WebCore/css/FontFaceSet.cpp:115
> +    return m_backing->faceCountAfterUpdatingStyle();

I find this function confusing. When would you call the "bad face count"
function that doesn’t update style? Names really don’t make that clear.

> Source/WebCore/dom/Document.cpp:7447
> +    updateStyleIfNeeded(); // FIXME: This is unnecessary. Instead, the
actual accessors in the FontFaceSet need to update style.

Seems like a good direction.