Bug 177688 – Hook up font-display state transitions to the web inspector

• Status: ASSIGNED
• Component: Text

[Myles C. Maxfield]

Hook up font-display state transitions to the web inspector

[Radar WebKit Bug Importer]

<rdar://problem/34749521>

[Myles C. Maxfield]

Created attachment 322955 [details]
Patch

[Myles C. Maxfield]

Created attachment 323490 [details]
Patch

[Build Bot]

Comment on attachment 323490 [details]
Patch

Attachment 323490 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4831735

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 323514 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[Daniel Bates]

Comment on attachment 323490 [details]
Patch

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

> Source/WebCore/css/CSSFontFace.cpp:47
>  #include "StyleProperties.h"

If we cannot get rid of the include CSSFontFace.h from InspectorInstrumentation.h (see remark in that file) then we should remove the include StyleRule.h (below) since CSSFontFace.h includes it.

> Source/WebCore/css/CSSFontFace.cpp:104
> +    InspectorInstrumentation::fontFaceBlockWillBeDestroyed(*m_fontSelector->document(), *this, gatherCachedFonts());

What guarantees that m_fontSelector has a non-null document()?

> Source/WebCore/css/CSSFontFace.cpp:623
> +Vector<std::reference_wrapper<CachedFont>> CSSFontFace::gatherCachedFonts() const

I am not near a computer with a checkout. Is “gather” a common prefix we use for similar functions? Is this prefix even needed?

> Source/WebCore/css/CSSFontFace.cpp:625
> +    Vector<std::reference_wrapper<CachedFont>> result;

Can we make an educated guess about the size of this vector and reserveInitialCapacity()? This will help minimize vector resizing during iteration.

> Source/WebCore/css/CSSFontFace.cpp:657
> +    auto oldStatus = m_status;

I take it you feel having this local improves the readability of the code in constrast to structuring the code such that we first notify the inspector and then update m_status.

> Source/WebCore/css/CSSFontFace.cpp:698
> +    InspectorInstrumentation::fontFaceBlockWasCreated(*m_fontSelector->document(), *this, gatherCachedFonts());

What guarantees that m_fontSelector has a non-null document()?

> Source/WebCore/css/CSSFontFace.h:184
> +

I suggest that we add a typedef (using the C++11 using syntax) for the data type, say CachedFontVector, instead of repeating “Vector<std::reference_wrapper<CachedFont>>” everywhere.

> Source/WebCore/css/CSSFontFaceSource.h:74
> +    CachedFont* cachedFont() const { return m_font.get(); }

Although this function does not actually modify m_font (and hence is const with respect to mutation of its own intense variables) the caller can mutate m_font since we return a non-const pointer to it. We tend to have member functions be in one of two groups: either be const and return const data or be non-cont and return non-const data. Can we pick one of these forms?

> Source/WebCore/inspector/InspectorInstrumentation.h:34
> +#include "CSSFontFace.h"

This is unfortunate that we need to include this header just to have access to the enum class CSSFontFace::Status. I wish we could avoid this. Maybe we have to wait until a future C++ to be able to forward declare it. Alternatively we could move the enum class outside the CSSFontFace class. Then we can forward declare it in this file.

> Source/WebCore/inspector/InspectorInstrumentation.h:282
> +    static void fontFaceBlockWasCreated(Document&, const CSSFontFace&, const Vector<std::reference_wrapper<CachedFont>>&);

Block? Is that standard terminology?

[Myles C. Maxfield]

Created attachment 323756 [details]
Patch