Bug 222550 – Remove document accessor on CSSFontSelector

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Lord]

Previous work (see bug 208351) removes some instances of unnecessary Document dependence where ScriptExecutionContext would do and make it easier to alter things to be Worker-compatible in the future. Removing CSSFontSelector::document() in favour of ::scriptExecutionContext() would be another step that would help in this process.

[Chris Lord]

Created attachment 421825 [details]
Patch

[Darin Adler]

Comment on attachment 421825 [details]
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?

[Chris Lord]

(In reply to Darin Adler from comment #2)
> Comment on attachment 421825 [details]
> 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?

Is this in regards to the constructor? I can certainly take a look.

> > 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.

Sure, I think for some reason I was under the impression that auto wouldn't work, probably from a different version of the patch with some ambiguity here...

> > 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?

I'll take a look and see if there's anything I can trim.

> > 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?

That's right, I can add a comment to this effect. I already have the follow-up patches that allow construction with a generic ScriptExecutionContext, so hopefully there won't be too big a time with this slightly confusing behaviour.

[Chris Lord]

Created attachment 421916 [details]
Patch

[EWS]

Committed r273726: <https://commits.webkit.org/r273726>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421916 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74919118>