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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jul 29 16:10:47 PDT 2019

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

Attachment 373821: Patch


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

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


> 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

> 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:

    makeString(foo, multiplicationSign, bar);

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

> Source/WebCore/inspector/InspectorOverlay.cpp:849
> +	   FontCascadeDescription fontDescription;
> +	  
> +	   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() });

	FontCascade font(WTFMove(fontDescription), 0, 0);
	return font;

But maybe not until we have more uses.

More information about the webkit-reviews mailing list