[webkit-reviews] review granted: [Bug 199369] Web Inspector: Overlay: add page width/height display : [Attachment 373821] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 199369: Web Inspector: Overlay: add page width/height display
https://bugs.webkit.org/show_bug.cgi?id=199369

Attachment 373821: Patch

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




--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 373821
  --> https://bugs.webkit.org/attachment.cgi?id=373821
Patch

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

r=me

> Source/WebCore/inspector/InspectorOverlay.cpp:77
> +static constexpr float viewportSizePadding = 5;

This constant seems like it could be put closer to the code as it won't be used
elsewhere.

> Source/WebCore/inspector/InspectorOverlay.cpp:-79
> -    const UChar ellipsisUChar[] = { 0x2026, 0 };

Err, I was suggesting these be at the top level scope like so:

    const UChar ellipsis = 0x2026;
    const UChar multiplicationSign = 0x00D7;

Then I suspect it could just be used as expected:

    string.append(ellipsis);
    makeString(foo, multiplicationSign, bar);

I don't know why this is done weirdly in both `InspectorOverlay.cpp` and
`InspectorDOMAgent.cpp`.

> Source/WebCore/inspector/InspectorOverlay.cpp:849
> +	   FontCascadeDescription fontDescription;
> +	  
fontDescription.setOneFamily(m_page.settings().sansSerifFontFamily());
> +	   fontDescription.setComputedSize(14);
> +
> +	   FontCascade font(WTFMove(fontDescription), 0, 0);
> +	   font.update(nullptr);

This seems to be a common pattern in the file. It might be nice to have a
helper along the lines of:

    static FontCascade overlayFont(float computedSize)
    {
	FontCascadeDescription fontDescription;
	fontDescription.setFamilies({ "Menlo",
m_page.settings().fixedFontFamily() });
	fontDescription.setComputedSize(11);

	FontCascade font(WTFMove(fontDescription), 0, 0);
	font.update(nullptr);
	return font;
    }

But maybe not until we have more uses.