Bug 175734 – Web Inspector: Capture Element Screenshot looks fuzzy

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

It appears to capture to 1x on a Retina display.

[Blaze Burg]

<rdar://problem/33803377>

[Blaze Burg]

Created attachment 328662 [details]
WIP

[EWS Watchlist]

Attachment 328662 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:9:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 328662 [details]
WIP

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

> Source/WebCore/platform/graphics/ImageBuffer.h:108
> +    String toDataURL(const String& mimeType, std::optional<double> quality = std::nullopt, ScaleBehavior = Scaled) const;

It's hard to wrap my head around what "Scaled" means. Does it mean use the logical size, or the physical size?

[Simon Fraser (smfr)]

Comment on attachment 328662 [details]
WIP

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

>> Source/WebCore/platform/graphics/ImageBuffer.h:108
>> +    String toDataURL(const String& mimeType, std::optional<double> quality = std::nullopt, ScaleBehavior = Scaled) const;
> 
> It's hard to wrap my head around what "Scaled" means. Does it mean use the logical size, or the physical size?

Maybe better terminology would be around "preserve resolution" or something.

[Blaze Burg]

(In reply to Simon Fraser (smfr) from comment #4)
> Comment on attachment 328662 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=328662&action=review
> 
> > Source/WebCore/platform/graphics/ImageBuffer.h:108
> > +    String toDataURL(const String& mimeType, std::optional<double> quality = std::nullopt, ScaleBehavior = Scaled) const;
> 
> It's hard to wrap my head around what "Scaled" means. Does it mean use the
> logical size, or the physical size?

The existing uses are that Scaled is the logical size and Unscaled is the internal size. I was going to introduce my own enum, then I noticed this other one being used in the exact same way, so I used it instead.

[Blaze Burg]

Created attachment 329844 [details]
Proposed Fix

[Blaze Burg]

Created attachment 329846 [details]
Patch

[Blaze Burg]

webkit-patch is messing up my patches, but the correct one is up for review now.

[Blaze Burg]

Per smfr, will put up new patch with enum class PreserveResolution { Yes, No }

[Joseph Pecoraro]

Comment on attachment 329844 [details]
Proposed Fix

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

Inspector parts look good to me, someone else should probably review the WebCore portions.

> LayoutTests/inspector/page/hidpi-snapshot-size.html:8
> +    window.snapshotElement = document.getElementById('element-to-snapshot');

Style: Some single quotes in this test that could be double quotes.

> LayoutTests/inspector/page/hidpi-snapshot-size.html:16
> +    // This function is adapted from an answer on the following page:
> +    // https://stackoverflow.com/questions/15327959/get-height-and-width-dimensions-from-base64-png/41152378#41152378

I don't really think we need to link to the stack overflow. Does this gain us anything?

If we want to carry over the explanation of what this is doing we can attribute it and reformat + improve it here. For example:

    A PNG file starts with the bytes 89 50 4E 47 0D 01 1A 0A, followed by the
    IHDR chunk that contains the size. IHDR’s content starts with a 4-byte width
    and a 4-byte height, so a PNG’s width and height are always bytes 16-24.

> LayoutTests/inspector/page/hidpi-snapshot-size.html:28
> +    let suite = InspectorTest.createAsyncSuite("Page.snapshot");

This tests Page.snapshotNode and maybe could get an even better name.

> LayoutTests/inspector/page/hidpi-snapshot-size.html:41
> +            // Peek into the data URL's raw PNG data to get the size. For various reasons,
> +            // setting the 'src' attribute of an <img> element is unreliable under WKTR.

I don't find this comment useful.

> LayoutTests/inspector/page/hidpi-snapshot-size.html:58
> +<p id="element-to-snapshot" style="height: 198px; width: 198px; border: 1px solid black;">Test for DOM.snapshotNode on HiDPI systems.</p>

Typo: Says "DOM.snapshotNode" but test "Page.snapshotNode"

[Blaze Burg]

Comment on attachment 329844 [details]
Proposed Fix

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

>> LayoutTests/inspector/page/hidpi-snapshot-size.html:41
>> +            // setting the 'src' attribute of an <img> element is unreliable under WKTR.
> 
> I don't find this comment useful.

I spent a lot of time trying to do it that way, and I want to remember why it was a dead end. =)

[Simon Fraser (smfr)]

(In reply to Brian Burg from comment #12)
> Comment on attachment 329844 [details]
> Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=329844&action=review
> 
> >> LayoutTests/inspector/page/hidpi-snapshot-size.html:41
> >> +            // setting the 'src' attribute of an <img> element is unreliable under WKTR.
> > 
> > I don't find this comment useful.
> 
> I spent a lot of time trying to do it that way, and I want to remember why
> it was a dead end. =)

You make it sound like setting the src doesn't work. It does, but you probably have to listen for the 'load' event to fix the test.

[Blaze Burg]

Comment on attachment 329844 [details]
Proposed Fix

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

>>>> LayoutTests/inspector/page/hidpi-snapshot-size.html:41
>>>> +            // setting the 'src' attribute of an <img> element is unreliable under WKTR.
>>> 
>>> I don't find this comment useful.
>> 
>> I spent a lot of time trying to do it that way, and I want to remember why it was a dead end. =)
> 
> You make it sound like setting the src doesn't work. It does, but you probably have to listen for the 'load' event to fix the test.

I did try that approach (listen for 'load') but the width/height properties of the image element did not match what would be shown when running the test manually under mini browser or Safari. I didn't have time to further investigate why that is.

[Blaze Burg]

Created attachment 330491 [details]
Proposed Fix (review feedback applied)

[Simon Fraser (smfr)]

Comment on attachment 330491 [details]
Proposed Fix (review feedback applied)

ImageBuffer bits look OK.

[Joseph Pecoraro]

Comment on attachment 330491 [details]
Proposed Fix (review feedback applied)

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

r=me since smfr is happy with the rest

> LayoutTests/inspector/page/hidpi-snapshot-size.html:17
> +    // IHDR chunk that contains the size. IHDRâs content starts with a 4-byte width
> +    // and a 4-byte height, so a PNGâs width and height are always bytes 16-24.

Might want to cleanup these single quotes: "IHDRâs" and "PNGâs"

> LayoutTests/inspector/page/hidpi-snapshot-size.html:35
> +            let targetElementId = await WI.domTreeManager.querySelector(documentNode.id, "#element-to-snapshot");

Nit: I'd name this `targetNodeId` instead of ElementId.

[Blaze Burg]

Committed r226425: <https://trac.webkit.org/changeset/226425>