Bug 177688

Summary: Hook up font-display state transitions to the web inspector
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: TextAssignee: Myles C. Maxfield <mmaxfield>
Status: ASSIGNED ---    
Severity: Normal CC: buildbot, dbates, joepeck, mmaxfield, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 167332    
Description Flags
dbates: review+, buildbot: commit-queue-
Archive of layout-test-results from ews121 for ios-simulator-wk2
Patch none

Description Myles C. Maxfield 2017-09-29 15:53:31 PDT
Hook up font-display state transitions to the web inspector
Comment 1 Radar WebKit Bug Importer 2017-09-29 15:53:56 PDT
Comment 2 Myles C. Maxfield 2017-10-05 17:59:47 PDT
Created attachment 322955 [details]
Comment 3 Myles C. Maxfield 2017-10-11 16:56:44 PDT
Created attachment 323490 [details]
Comment 4 Build Bot 2017-10-11 22:55:11 PDT
Comment on attachment 323490 [details]

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.
Comment 5 Build Bot 2017-10-11 22:55:13 PDT
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
Comment 6 Daniel Bates 2017-10-12 00:24:35 PDT
Comment on attachment 323490 [details]

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?
Comment 7 Myles C. Maxfield 2017-10-13 15:33:42 PDT
Created attachment 323756 [details]