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

Rename |is root| to |include parent id| #726

Merged
merged 2 commits into from
Jun 13, 2024
Merged

Rename |is root| to |include parent id| #726

merged 2 commits into from
Jun 13, 2024

Conversation

OrKoN
Copy link
Contributor

@OrKoN OrKoN commented Jun 7, 2024

|is root| makes it look like it indicates whether the context is a root of the context tree (parent id === null) but it actually indicates whether the |parent id| needs to be included into the resulting info object.


Preview | Diff

index.bs Outdated
@@ -2419,7 +2419,7 @@ To <dfn>get the browsing context info</dfn> given |context|,
1. Let |context info| be a [=/map=] matching the
<code>browsingContext.Info</code> production with the <code>context</code>
field set to |context id|, the <code>parent</code> field set to |parent id|
if |is root| is <code>true</code>, or unset otherwise, the <code>url</code>
if |is root| is <code>false</code>, or unset otherwise, the <code>url</code>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we need |is root|? it |is root| being true the same as |parent id| being null?

Copy link
Contributor

@whimboo whimboo Jun 11, 2024

Choose a reason for hiding this comment

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

AFAIR we only want / have to include the parent id of a browsing context for the top-most browsing context that we want to get the browsing context info from. For frames we build up the tree and there is no need to have the extra parent id field.

Note that "is root" doesn't necessarily mean the top-level browsing context but rather the root browsing context from which we retrieve the browsing context information. Therefore, I don't see any issue with the current approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, that clears this up then, thanks!

@OrKoN OrKoN requested a review from jgraham June 11, 2024 10:29
index.bs Outdated
@@ -2419,7 +2419,7 @@ To <dfn>get the browsing context info</dfn> given |context|,
1. Let |context info| be a [=/map=] matching the
<code>browsingContext.Info</code> production with the <code>context</code>
field set to |context id|, the <code>parent</code> field set to |parent id|
if |is root| is <code>true</code>, or unset otherwise, the <code>url</code>
if |is root| is <code>false</code>, or unset otherwise, the <code>url</code>
Copy link
Contributor

@whimboo whimboo Jun 11, 2024

Choose a reason for hiding this comment

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

AFAIR we only want / have to include the parent id of a browsing context for the top-most browsing context that we want to get the browsing context info from. For frames we build up the tree and there is no need to have the extra parent id field.

Note that "is root" doesn't necessarily mean the top-level browsing context but rather the root browsing context from which we retrieve the browsing context information. Therefore, I don't see any issue with the current approach.

@OrKoN OrKoN changed the title fix parent context id Rename |is root| to |include parent id| Jun 11, 2024
@OrKoN OrKoN requested a review from whimboo June 11, 2024 14:23
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.

This change looks fine and indeed makes it much easier to understand what this flag is actually used for.

Can you please also update the WebDriver BiDi client and the wpt tests for the updated name? Thanks.

@OrKoN OrKoN merged commit a86a0c7 into main Jun 13, 2024
5 checks passed
@OrKoN OrKoN deleted the orkon/parent-id branch June 13, 2024 13:41
github-actions bot added a commit that referenced this pull request Jun 13, 2024
SHA: a86a0c7
Reason: push, by OrKoN

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants