Skip to content

Conversation

navin772
Copy link
Member

@navin772 navin772 commented Aug 5, 2025

User description

🔗 Related Issues

Fixes #16128

💥 What does this PR do?

Adds default as a possible value for the Samesite attribute.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • New feature (non-breaking change which adds functionality and tests!)

PR Type

Enhancement


Description

  • Add Default value to SameSite enum for cookies

  • Implements cookie specification compliance across language bindings


Diagram Walkthrough

flowchart LR
  A["SameSite Enum"] --> B["Add Default Value"]
  B --> C["Cookie Spec Compliance"]
Loading

File Walkthrough

Relevant files
Enhancement
Cookie.cs
Add Default to SameSite enum                                                         

dotnet/src/webdriver/BiDi/Network/Cookie.cs

  • Add Default enum value to SameSite enumeration
+2/-1     

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Aug 5, 2025
Copy link
Contributor

qodo-merge-pro bot commented Aug 5, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

16128 - Partially compliant

Compliant requirements:

• Add default as a possible value for the SameSite attribute (for dotnet binding)

Non-compliant requirements:

• Implement cookie specification compliance across language bindings (only dotnet shown)
• Support for dotnet, py, java, and js language bindings (only dotnet implementation visible)

Requires further human verification:

• Verification that similar changes are implemented in py, java, and js bindings
• Testing that the Default value works correctly with actual cookie operations

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Missing Tests

The PR adds a new enum value but doesn't include any tests to verify the Default value works correctly in cookie operations or serialization scenarios.

    Default
}

Copy link
Contributor

qodo-merge-pro bot commented Aug 5, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@navin772 navin772 added C-py Python Bindings C-java Java Bindings C-nodejs JavaScript Bindings labels Aug 5, 2025
@navin772 navin772 requested a review from nvborisenko August 5, 2025 14:51
Copy link
Member

@nvborisenko nvborisenko left a comment

Choose a reason for hiding this comment

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

.net part is correct, thanks.

@navin772
Copy link
Member Author

navin772 commented Aug 6, 2025

Failing js fedcm test is expected due to https://issues.chromium.org/issues/425801332

@navin772 navin772 merged commit 0d9ee19 into SeleniumHQ:trunk Aug 6, 2025
30 of 31 checks passed
@navin772 navin772 deleted the add-default-cookie-type branch August 6, 2025 05:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-dotnet .NET Bindings C-java Java Bindings C-nodejs JavaScript Bindings C-py Python Bindings Review effort 2/5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🚀 Feature]: Add default as a value for SameSite cookie
4 participants