Bug 199369 - Web Inspector: Overlay: add page width/height display
Summary: Web Inspector: Overlay: add page width/height display
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-01 11:24 PDT by Devin Rousso
Modified: 2019-07-31 13:55 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.42 KB, patch)
2019-07-06 15:54 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (5.08 KB, patch)
2019-07-09 22:25 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (2.87 MB, image/png)
2019-07-09 22:25 PDT, Devin Rousso
no flags Details
Patch (14.75 KB, patch)
2019-07-31 12:56 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (63.29 KB, image/png)
2019-07-31 12:56 PDT, Devin Rousso
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-07-01 11:24:15 PDT
Often one of the most useful parts of the page rulers is being able to see the width/height of the page.  There are ways of getting this information now (e.g. logging `window.innerWidth`), but they all involve some other user action.  It would be great to be able to see this information whenever rulers are shown so no extra actions are required of the user.
Comment 1 Devin Rousso 2019-07-06 15:54:36 PDT
Created attachment 373585 [details]
Patch
Comment 2 Joseph Pecoraro 2019-07-09 15:40:20 PDT
Got a screenshot?
Comment 3 Joseph Pecoraro 2019-07-09 15:49:23 PDT
Comment on attachment 373585 [details]
Patch

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

> Source/WebCore/inspector/InspectorOverlay.cpp:844
> +        const UChar multiplicationSignUChar[] = { 0x00D7, 0 };

Is there any reason this needs to be a `UChar[]` and not just a `UChar`?

It seems like `makeString` would support a `UChar` since StringConcatenate has `template<> class StringTypeAdapter<UChar, void> {...}`. Unfortunately there aren't tests for it on `TestWebKitAPI/Tests/WTF/StringConcatenate.cpp` though. It does seem
Comment 4 Devin Rousso 2019-07-09 22:25:15 PDT
Created attachment 373821 [details]
Patch

This does have one unfortunate side-effect right now, in that if the highlighted node is large enough, the node info will show _under_ the width/height in the top-left corner :(
Comment 5 Devin Rousso 2019-07-09 22:25:34 PDT
Created attachment 373822 [details]
[Image] After Patch is applied
Comment 6 Joseph Pecoraro 2019-07-29 16:10:47 PDT
Comment on attachment 373821 [details]
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.
Comment 7 Devin Rousso 2019-07-31 12:56:41 PDT
Created attachment 375236 [details]
Patch
Comment 8 Devin Rousso 2019-07-31 12:56:59 PDT
Created attachment 375237 [details]
[Image] After Patch is applied
Comment 9 WebKit Commit Bot 2019-07-31 13:54:24 PDT
Comment on attachment 375236 [details]
Patch

Clearing flags on attachment: 375236

Committed r248053: <https://trac.webkit.org/changeset/248053>
Comment 10 WebKit Commit Bot 2019-07-31 13:54:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2019-07-31 13:55:36 PDT
<rdar://problem/53778717>