Hook up font-display state transitions to the web inspector
<rdar://problem/34749521>
Created attachment 322955 [details] Patch
Created attachment 323490 [details] Patch
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.
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 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?
Created attachment 323756 [details] Patch