Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LibWeb: Update "create and initialize a document" to currrent spec #2971

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gjf7
Copy link

@gjf7 gjf7 commented Dec 19, 2024

  1. Update "create and initialize a document" to currrent spec
  2. Add noopener-allow-popups value to OpenerPolicyValue

@gjf7 gjf7 marked this pull request as ready for review December 19, 2024 04:35
@gjf7 gjf7 changed the title Update "create and initialize a document" to currrent spec LibWeb: Update "create and initialize a document" to currrent spec Dec 19, 2024
@AtkinsSJ
Copy link
Member

This is failing some tests but it's hard to tell which. Can you rebase on master? #2950 should make the output a bit more readable.

@gjf7
Copy link
Author

gjf7 commented Dec 19, 2024

This is failing some tests but it's hard to tell which. Can you rebase on master? #2950 should make the output a bit more readable.

Hi there, could u approve the ci again? Thanks

// 1. If either of sourceOrigin or destinationOrigin have a scheme that is not an HTTP(S) scheme
// and the user agent considers it necessary for sourceOrigin and destinationOrigin to be isolated
// from each other (for implementation-defined reasons), optionally set swapGroup to true.
if (!source_origin.scheme()->starts_with_bytes("http"sv) || !destination_origin.scheme()->starts_with_bytes("http"sv)) {
Copy link
Author

Choose a reason for hiding this comment

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

Oh. The return value of scheme() is Optional.

I tried using Optional's value_or() with an empty String as follows:

if (!source_origin.scheme().value_or(String {}).starts_with_bytes("http"sv) || 
    !destination_origin.scheme().value_or(String {}).starts_with_bytes("http"sv)) {
    swap_group = true;
}

However, I'm getting a verification error at runtime:
CopyVERIFICATION FAILED: m_has_value at /Users/haochen/Project/ladybird/AK/Optional.h:317

As I'm new to C++, I'm not sure about the best way to handle this. @AtkinsSJ, could you suggest the proper way to check Optional in this context?

Copy link
Member

Choose a reason for hiding this comment

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

I would probably do this:

if (source_origin.scheme.has_value() &&
    (!source_origin.scheme()->starts_with_bytes("http"sv) || !destination_origin.scheme()->starts_with_bytes("http"sv))) {
    // ...
}

Copy link
Member

Choose a reason for hiding this comment

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

Oh scratch that, I missed that it's two different variables. 😅
But anyway foo.has_value() && !foo->starts_with_bytes("http"sv) is the check for each one.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! I've updated the code. Please let me know if any other changes are needed to merge this PR.

@gjf7 gjf7 requested a review from AtkinsSJ December 20, 2024 06:13
@gjf7 gjf7 force-pushed the feat/haochen branch 2 times, most recently from 710ae61 to 4fe0e8a Compare December 21, 2024 03:07
gjf7 added 2 commits December 22, 2024 10:52
Corresponds to whatwg/html#10394. Since we haven't implemented browsing context group switch logic due to opener policy, just add noopener-allow-popups to OpenerPolicyValue for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants