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

Use PresentationSourceType instead of url='_self' #6

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

takumif
Copy link
Collaborator

@takumif takumif commented Sep 21, 2022

No description provided.

@takumif
Copy link
Collaborator Author

takumif commented Sep 21, 2022

Mark PTAL

@@ -156,11 +154,21 @@ partial interface PresentationConnection {
};

dictionary PresentationSource {
readonly USVString url;
required PresentationSourceType type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be readonly as you shouldn't be able to change the source type on an existing connection.

Copy link
Collaborator Author

@takumif takumif Sep 26, 2022

Choose a reason for hiding this comment

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

It turns out that "Dictionaries must not be used as the type of an attribute" (spec), so we can't have PresentationSource be an attribute of PresentationConnection.

So I've added the fields directly to the PresentationConnection interface. We could wrap them into a separate interface but that feels like complication without much benefit right now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From a WebIDL point of view, I do think there is benefit in declaring the mirroring parameters once in an interface, so those declarations don't have to be repeated.

Copy link
Collaborator Author

@takumif takumif Oct 24, 2022

Choose a reason for hiding this comment

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

I agree that the repetition isn't great.

PresentationConnection and PresentationSource are an interface and a dictionary respectively with basically the same set of fields, to achieve:

  1. Not requiring the user to call a ctor for creating a PresentationSource
  2. Working around WebIDL's restriction that a dictionary cannot be an interface attribute

I'm not sure if we can get rid of the duplication while maintaining the above two properties.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check the WebIDL spec to see what our options are here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thus far I'm not seeing a cleaner way to do it. Implicit conversion from a JS object can be done with dictionaries but not interfaces. So we'd want PresentationSource to be a dictionary so that websites would be able to pass a JS object in rather than having to call a ctor.

However a dictionary cannot be used as an attribute nor can an interface inherit from a dictionary (or vice versa), so it seems to me that the parameters need to declared once each in a dictionary and an interface.

explainer.md Show resolved Hide resolved
explainer.md Outdated Show resolved Hide resolved
explainer.md Show resolved Hide resolved
@@ -156,11 +154,21 @@ partial interface PresentationConnection {
};

dictionary PresentationSource {
readonly USVString url;
required PresentationSourceType type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

From a WebIDL point of view, I do think there is benefit in declaring the mirroring parameters once in an interface, so those declarations don't have to be repeated.

explainer.md Outdated Show resolved Hide resolved
explainer.md Outdated
readonly attribute PresentationSourceType type;

// Used if `type` is "url".
readonly attribute USVString? url;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this can change to optional since it's mandatory in the current spec, as that would risk breaking current script.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, changed. I suppose it can be an empty string rather than null when not used.

@@ -156,11 +154,21 @@ partial interface PresentationConnection {
};

dictionary PresentationSource {
readonly USVString url;
required PresentationSourceType type;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you check the WebIDL spec to see what our options are here?

@takumif
Copy link
Collaborator Author

takumif commented Nov 16, 2022

The only change I made was to readonly attribute USVString url; in PresentationConnection. Sorry for all the noise from my attempt to rebase the branch.

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