Bug 150767 – Clean up some CSS & Font code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Clean up some CSS & Font code

[Myles C. Maxfield]

Created attachment 264507 [details]
Patch

[Darin Adler]

Comment on attachment 264507 [details]
Patch

GTK and EFL build failure is:

Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:118:18: error: prototype for 'WTF::PassRefPtr<WebCore::Font> WebCore::Font::platformCreateScaledFont(const WebCore::FontDescription&, float) const' does not match any in class 'WebCore::Font'

Windows build failure is:

Source\WebCore\platform\graphics\win\SimpleFontDataWin.cpp(126): error C2556: 'WTF::PassRefPtr<T> WebCore::Font::platformCreateScaledFont(const WebCore::FontDescription &,float) const': overloaded function differs only by return type from 'WTF::RefPtr<WebCore::Font> WebCore::Font::platformCreateScaledFont(const WebCore::FontDescription &,float) const' [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

[Darin Adler]

Comment on attachment 264507 [details]
Patch

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

Changes look OK, but I think you can do even better.

> Source/WebCore/css/CSSFontSelector.cpp:206
> +            Settings* settings = document ? document->frame() ? &document->frame()->settings() : nullptr : nullptr;

No need to go through the frame:

    Settings* settings = document ? document->settings() : nullptr;

On a separate note, what caller actually calls this with null for the document?

> Source/WebCore/css/RuleSet.cpp:365
> -    const Vector<RefPtr<StyleRuleImport>>& importRules = sheet->importRules();
> +    const Vector<RefPtr<StyleRuleImport>>& importRules = sheet.importRules();
>      for (unsigned i = 0; i < importRules.size(); ++i) {
>          StyleRuleImport* importRule = importRules[i].get();

At first, I thought you should use auto&, but really thus should be a modern for loop:

    for (auto& rule : sheet.importRules()) {

> Source/WebCore/platform/graphics/Font.cpp:268
> +RefPtr<Font> Font::verticalRightOrientationFont() const

Seems to me this should return Font& or possible const Font&, not RefPtr<Font>. It can’t be null, and this function does not create a new object and transfer ownership to the caller.

> Source/WebCore/platform/graphics/Font.cpp:281
> +RefPtr<Font> Font::uprightOrientationFont() const

Seems to me this should return Font& or possibly const Font&, not RefPtr<Font>. It can’t be null, and this function does not create a new object and transfer ownership to the caller. If I am wrong about null, then maybe Font* or const Font*.

> Source/WebCore/platform/graphics/Font.cpp:291
> +RefPtr<Font> Font::smallCapsFont(const FontDescription& fontDescription) const

Ditto.

> Source/WebCore/platform/graphics/Font.cpp:301
> +RefPtr<Font> Font::emphasisMarkFont(const FontDescription& fontDescription) const

Ditto.

> Source/WebCore/platform/graphics/Font.cpp:311
> +RefPtr<Font> Font::brokenIdeographFont() const

Ditto.

> Source/WebCore/platform/graphics/Font.cpp:323
> +RefPtr<Font> Font::nonSyntheticItalicFont() const

Ditto.

> Source/WebCore/platform/graphics/Font.cpp:366
> +RefPtr<Font> Font::createScaledFont(const FontDescription& fontDescription, float scaleFactor) const

Ditto.

[Myles C. Maxfield]

Comment on attachment 264507 [details]
Patch

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

>> Source/WebCore/css/CSSFontSelector.cpp:206
>> +            Settings* settings = document ? document->frame() ? &document->frame()->settings() : nullptr : nullptr;
> 
> No need to go through the frame:
> 
>     Settings* settings = document ? document->settings() : nullptr;
> 
> On a separate note, what caller actually calls this with null for the document?

The lifetime of the font selector is the same as the style resolver, which can outlive the document.

>> Source/WebCore/platform/graphics/Font.cpp:291
>> +RefPtr<Font> Font::smallCapsFont(const FontDescription& fontDescription) const
> 
> Ditto.

Unfortunately, createScaledFont() can return nullptr.

[Myles C. Maxfield]

Created attachment 264535 [details]
Patch for committing

[WebKit Commit Bot]

Comment on attachment 264535 [details]
Patch for committing

Clearing flags on attachment: 264535

Committed r191871: <http://trac.webkit.org/changeset/191871>

[Radar WebKit Bug Importer]

<rdar://problem/23767722>