It appears to capture to 1x on a Retina display.
<rdar://problem/33803377>
Created attachment 328662 [details] WIP
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.
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?
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.
(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.
Created attachment 329844 [details] Proposed Fix
Created attachment 329846 [details] Patch
webkit-patch is messing up my patches, but the correct one is up for review now.
Per smfr, will put up new patch with enum class PreserveResolution { Yes, No }
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"
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. =)
(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.
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.
Created attachment 330491 [details] Proposed Fix (review feedback applied)
Comment on attachment 330491 [details] Proposed Fix (review feedback applied) ImageBuffer bits look OK.
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.
Committed r226425: <https://trac.webkit.org/changeset/226425>