Skip to content

Add formats to captureScreenshot#561

Merged
jrandolf-google merged 1 commit intomainfrom
jrandolf/formats
Oct 5, 2023
Merged

Add formats to captureScreenshot#561
jrandolf-google merged 1 commit intomainfrom
jrandolf/formats

Conversation

@jrandolf-google
Copy link
Contributor

@jrandolf-google jrandolf-google commented Sep 28, 2023

This PR implements screenshot formats.

Closes #383

FAQ

Do implementations need to support all formats?

No, if the implementation is unable to capture a screenshot of a context for any reason, then return error with error code unsupported operation.


Preview | Diff

@jrandolf-google
Copy link
Contributor Author

@jgraham @gsnedders PTAL. We internally discussed what formats should be considered. I suggested we use the ones proposed here, but @OrKoN questioned whether they are compatible with other browsers. Considering implementations can throw unsupported operation, I think these should be okay.

Alternatively, we can make this a string and state it must be a image mime type. This is more flexible in case you folks have other requirements such as proprietary formats.

For example,

? format: string | browsingContext.ScreenshotFormat;
...

browsingContext.ScreenshotFormat = {
  type: string
} & Extensible

We could then change the algorithm to allow implementation-specific formats when the format is not one of the predefined types (webp, png, and jpeg).

@OrKoN
Copy link
Contributor

OrKoN commented Sep 28, 2023

I think if there will be no agreement that we need to support a shared set of the formats, we can probably have a vendor prefixed extension for the screenshot types in chromium-bidi.

@OrKoN
Copy link
Contributor

OrKoN commented Sep 28, 2023

@jrandolf please link and see comments on #383

@jrandolf-google jrandolf-google force-pushed the jrandolf/formats branch 2 times, most recently from 3c8cd2b to 536afce Compare September 29, 2023 10:18
@jrandolf-google jrandolf-google force-pushed the jrandolf/formats branch 2 times, most recently from 55d0c6a to a532870 Compare September 29, 2023 10:40
@jrandolf-google jrandolf-google force-pushed the jrandolf/formats branch 2 times, most recently from 1063fae to 430d6a9 Compare September 29, 2023 10:44
@jrandolf-google jrandolf-google force-pushed the jrandolf/formats branch 5 times, most recently from 113a35b to 572ce95 Compare September 29, 2023 12:14
Copy link
Contributor

@OrKoN OrKoN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one change required here, I think.

@jrandolf-google jrandolf-google force-pushed the jrandolf/formats branch 2 times, most recently from 8943139 to 08a647e Compare October 4, 2023 11:37
@jrandolf-google jrandolf-google removed the request for review from gsnedders October 4, 2023 13:08
@jrandolf-google
Copy link
Contributor Author

@gsnedders @jgraham Is it ready to land?

@jrandolf-google jrandolf-google merged commit 8808a94 into main Oct 5, 2023
@jrandolf-google jrandolf-google deleted the jrandolf/formats branch October 5, 2023 11:49
github-actions bot added a commit that referenced this pull request Oct 5, 2023
SHA: 8808a94
Reason: push, by jrandolf

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
jrandolf-google added a commit that referenced this pull request Oct 5, 2023
github-actions bot added a commit that referenced this pull request Oct 5, 2023
SHA: 073f489
Reason: push, by jrandolf

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@whimboo
Copy link
Contributor

whimboo commented Oct 26, 2023

@jrandolf are you going to add new wpt tests for this feature? CC @OrKoN.

@whimboo whimboo added module-browsingContext Browsing Context module needs-tests labels Oct 26, 2023
@jrandolf-google
Copy link
Contributor Author

Done: web-platform-tests/wpt#42804

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"browsingContext.captureScreenshot" should support different image formats

5 participants