Skip to content

Conversation

fernando-80
Copy link
Contributor

@fernando-80 fernando-80 commented Oct 29, 2020

Closes #67

The following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementation commitment:

@fernando-80
Copy link
Contributor Author

@nondebug @knyg @sagoston light-indicator extension PR ^^^

@marcoscaceres
Copy link
Member

Hi @fernando-80, could you kindly please join the WebApps working group? That will make the IPR bot happy.

</p>

<p>
The returned Promise will resolve <code>true</code> when setting the
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if this can be guaranteed to succeed. @nondebug, do you have an opinion about this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We typically don't get a signal that the color was changed successfully so we shouldn't claim that in the spec. I think we should reword this so we resolve when the LED color change command is sent and reject if the command couldn't be sent.

Copy link
Member

@marcoscaceres marcoscaceres Mar 3, 2021

Choose a reason for hiding this comment

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

Ok, so yeah... it shouldn't resolve with true either.

We probably need to rewrite this whole section. I'd suggest having a look at:
https://w3c.github.io/web-share/#share-method

Basically, we need to do a few checks that can cause the promise to reject... the controller has been disconnected/no longer available, etc. There might be a SecurityError if the permission has been changed before .setColor() is called. Also, what should happen if:

while (true) {
   x.setColor(randomRGB());
}

Rate limit? Drop requests on the floor?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We discussed this and couldn't come up with any compelling reasons to keep the Promise. It's unlikely that we'll receive any error indication from the device and there isn't any way we can lose permission for a Gamepad. Let's remove the promise and treat setColor as fire-and-forget.

Comment on lines +442 to +443
color has succeeded, and will reject the device API error code on
failure.
Copy link
Member

Choose a reason for hiding this comment

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

This needs to reject with a an actual DOM Exception of some kind. I think this is overly broad/underspecified tho...

Copy link
Member

@marcoscaceres marcoscaceres left a comment

Choose a reason for hiding this comment

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

Thanks @fernando-80! Some suggestions...

fernando-80 and others added 5 commits March 2, 2021 08:46
Co-authored-by: Marcos Cáceres <[email protected]>
Co-authored-by: Marcos Cáceres <[email protected]>
Co-authored-by: Marcos Cáceres <[email protected]>
Co-authored-by: Marcos Cáceres <[email protected]>
Co-authored-by: Marcos Cáceres <[email protected]>
fernando-80 and others added 3 commits March 2, 2021 08:47
Co-authored-by: Marcos Cáceres <[email protected]>
Co-authored-by: Marcos Cáceres <[email protected]>
Co-authored-by: Marcos Cáceres <[email protected]>
</p>

<p>
The returned Promise will resolve <code>true</code> when setting the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The returned Promise will resolve <code>true</code> when setting the
The returned Promise will resolve `undefined` when setting the

</p>

<pre class="idl">
dictionary GamepadLightColor {
Copy link
Member

Choose a reason for hiding this comment

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

We should investigate what existing color objects we could use from the platform here.

Copy link

@willuhmjs willuhmjs left a comment

Choose a reason for hiding this comment

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

Looks good, excited for this feature.

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.

Light Indicator

6 participants