[webkit-reviews] review granted: [Bug 222550] Remove document accessor on CSSFontSelector : [Attachment 421825] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 222550: Remove document accessor on CSSFontSelector
https://bugs.webkit.org/show_bug.cgi?id=222550

Attachment 421825: Patch

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




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

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

So many pointers. Are these all truly optional with meaningful null cases? Can
any be changed to references?

> Source/WebCore/css/CSSFontFace.cpp:77
> +	       const Settings::Values* settings = context ?
&context->settingsValues() : nullptr;

I think auto instead of const Settings::Values* would be better.

> Source/WebCore/css/CSSFontFace.cpp:97
> +    const Settings::Values* settings = context ? &context->settingsValues()
: nullptr;

Ditto.

> Source/WebCore/css/CSSFontFace.h:31
> +#include "Settings.h"

A real shame that Settings::Values means adding more header dependencies like
this.

Can we remove any other forward declarations or includes since Settings.h pulls
in more?

> Source/WebCore/css/CSSFontSelector.cpp:236
> +    return m_document.get();

Need a comment to explain why we hold a document yet return only a script
execution context; generally that’s a bad pattern that we try to avoid.
Presumably the reason is that we intend to eventually not hold a document?