| Summary: | Clean up some CSS & Font code | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||||
| Component: | New Bugs | Assignee: | Myles C. Maxfield <mmaxfield> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | commit-queue, dino, jonlee, kling, simon.fraser, thorton, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Myles C. Maxfield
2015-10-31 23:17:03 PDT
Created attachment 264507 [details]
Patch
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]
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. 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. Created attachment 264535 [details]
Patch for committing
Comment on attachment 264535 [details] Patch for committing Clearing flags on attachment: 264535 Committed r191871: <http://trac.webkit.org/changeset/191871> |