[webkit-reviews] review granted: [Bug 177688] Hook up font-display state transitions to the web inspector : [Attachment 323490] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 12 00:24:35 PDT 2017


Daniel Bates <dbates at webkit.org> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 177688: Hook up font-display state transitions to the web inspector
https://bugs.webkit.org/show_bug.cgi?id=177688

Attachment 323490: Patch

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




--- Comment #6 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 323490
  --> https://bugs.webkit.org/attachment.cgi?id=323490
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->documen
t(), *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?


More information about the webkit-reviews mailing list