Bug 177688 - Hook up font-display state transitions to the web inspector
Summary: Hook up font-display state transitions to the web inspector
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 167332
  Show dependency treegraph
 
Reported: 2017-09-29 15:53 PDT by Myles C. Maxfield
Modified: 2017-10-13 15:34 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.40 KB, patch)
2017-10-05 17:59 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff
Patch (8.79 KB, patch)
2017-10-11 16:56 PDT, Myles C. Maxfield
dbates: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (1.35 MB, application/zip)
2017-10-11 22:55 PDT, Build Bot
no flags Details
Patch (8.94 KB, patch)
2017-10-13 15:33 PDT, Myles C. Maxfield
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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
<rdar://problem/34749521>
Comment 2 Myles C. Maxfield 2017-10-05 17:59:47 PDT
Created attachment 322955 [details]
Patch
Comment 3 Myles C. Maxfield 2017-10-11 16:56:44 PDT
Created attachment 323490 [details]
Patch
Comment 4 Build Bot 2017-10-11 22:55:11 PDT
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.
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]
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?
Comment 7 Myles C. Maxfield 2017-10-13 15:33:42 PDT
Created attachment 323756 [details]
Patch