Skip to content

[WebDriver BiDi] Add screenshot origin/format tests#42804

Merged
OrKoN merged 4 commits intomasterfrom
jrandolf/screenshot-origin
Nov 24, 2023
Merged

[WebDriver BiDi] Add screenshot origin/format tests#42804
OrKoN merged 4 commits intomasterfrom
jrandolf/screenshot-origin

Conversation

@jrandolf-google
Copy link
Contributor

@jrandolf-google jrandolf-google commented Oct 27, 2023

@jrandolf-google jrandolf-google force-pushed the jrandolf/screenshot-origin branch from ebdb006 to fd6b01e Compare October 27, 2023 12:05
@jrandolf-google jrandolf-google requested a review from OrKoN October 27, 2023 12:06
@jrandolf-google jrandolf-google force-pushed the jrandolf/screenshot-origin branch from fd6b01e to c044e0a Compare October 27, 2023 12:08
Copy link
Contributor

@lutien lutien left a comment

Choose a reason for hiding this comment

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

I see that the target for this PR is another PR and not master, could you maybe change it? It's problematic because the changes from the target PR cause the unit test failures here.

Also, I guess we should test the combination of origin=document and clip, especially interesting would be the case when the target area is outside of viewport. What do you think?

@jrandolf-google jrandolf-google force-pushed the jrandolf/screenshot-origin branch 2 times, most recently from 0147918 to 8c03723 Compare October 31, 2023 12:06
@jrandolf-google jrandolf-google force-pushed the jrandolf/viewport_unchanged branch from d825448 to d6f4c4d Compare October 31, 2023 12:42
@jrandolf-google jrandolf-google force-pushed the jrandolf/screenshot-origin branch from 8c03723 to e85f834 Compare October 31, 2023 12:48
@jrandolf-google jrandolf-google force-pushed the jrandolf/viewport_unchanged branch from d6f4c4d to 9578b49 Compare October 31, 2023 13:17
@jrandolf-google jrandolf-google force-pushed the jrandolf/screenshot-origin branch 5 times, most recently from e6cd773 to 2915ebb Compare November 10, 2023 10:43
@jrandolf-google jrandolf-google force-pushed the jrandolf/viewport_unchanged branch 2 times, most recently from 896bb92 to 35849b0 Compare November 10, 2023 11:41
@whimboo
Copy link
Contributor

whimboo commented Nov 13, 2023

@jrandolf mind resolving the conflict for this PR before I do the next review? Tests for origin look fine so far and pass all in Firefox. Thanks.

@jrandolf-google jrandolf-google force-pushed the jrandolf/viewport_unchanged branch 3 times, most recently from ef12994 to aca7be3 Compare November 15, 2023 15:31
Base automatically changed from jrandolf/viewport_unchanged to master November 16, 2023 11:03
@jrandolf-google jrandolf-google force-pushed the jrandolf/screenshot-origin branch from 2915ebb to 8f81a79 Compare November 16, 2023 19:09
@jrandolf-google
Copy link
Contributor Author

@jrandolf mind resolving the conflict for this PR before I do the next review? Tests for origin look fine so far and pass all in Firefox. Thanks.

@whimboo Done.

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Some remaining comments. Otherwise it seems to be fine now.

@jrandolf-google jrandolf-google force-pushed the jrandolf/screenshot-origin branch 2 times, most recently from f1a570a to 9d302f2 Compare November 17, 2023 10:26
@jrandolf-google jrandolf-google force-pushed the jrandolf/screenshot-origin branch from 9d302f2 to 4b3b12f Compare November 17, 2023 10:51
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Thank you for the update @jrandolf! It looks all fine, just some nits.

def __init__(
self, element: Mapping[str, Any]
):
def __init__(self, element: Mapping[str, Any]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we missed the call to dict.__init__() here as well, but up to you to fix it now or later.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jrandolf please follow up on this once you are back

OrKoN and others added 3 commits November 24, 2023 10:00
…n.py

Co-authored-by: Henrik Skupin <mail@hskupin.info>
…t__.py

Co-authored-by: Henrik Skupin <mail@hskupin.info>
@OrKoN OrKoN enabled auto-merge (squash) November 24, 2023 09:59
@OrKoN OrKoN merged commit a8bb3e3 into master Nov 24, 2023
@OrKoN OrKoN deleted the jrandolf/screenshot-origin branch November 24, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants