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

Move unregister() to PushRegistration. #97

Merged
merged 1 commit into from Dec 15, 2014

Conversation

mounirlamouri
Copy link
Member

This is also changing the type of returned Promise and how it is
expected to behave.

@mounirlamouri
Copy link
Member Author

Fixes issue #94.

<li>Return <var>promise</var> and continue the following steps asynchronously.
</li>
<li>If the <a>webapp</a> is not registered, resolve <var>promise</var> with
<code>false</code> and terminate these steps.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we were still considering whether this should return true, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

How so?

Copy link
Contributor

Choose a reason for hiding this comment

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

For the case where unregister is called twice. The desired result was achieved: you are unregistered. That sounds more like true than false.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree. The entire idea of returning a boolean is to point that nothing actually happen. This is also what we do for ServiceWorker.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's weird, but I don't feel too strongly about it and aligning with sw is good. lgtm

This is also changing the type of returned Promise and how it is
expected to behave.
mvano added a commit that referenced this pull request Dec 15, 2014
Move unregister() to PushRegistration.
@mvano mvano merged commit 5afefce into w3c:gh-pages Dec 15, 2014
@mounirlamouri
Copy link
Member Author

Thanks! \o/

@martinthomson
Copy link
Member

Why do you need to know if there was something to unregister? If there is a use case, I'd be interested in learning of it.

@mounirlamouri
Copy link
Member Author

Do we really need a use case to expose an information that can anyway be accessed by other means?

@martinthomson
Copy link
Member

Even more so then. If you have other means of getting this information, I think that raises the bar. Can you justify why this returns a boolean over Promise<undefined>?

@mounirlamouri
Copy link
Member Author

Because then you can easily check if the call was a no-op or not... If you have a pushRegistration instance, you can just call pushRegistration.unregister().then(function(b) {...}); and use b to know whether the unregistration happened instead of doing navigator.push.getRegistration().then(function(r) { return r ? r.unregister() : null; }).then(function() { ... });.

@martinthomson
Copy link
Member

Why do you need to know if the call was a no-op?

@mounirlamouri
Copy link
Member Author

Should we wait for the need to arise before exposing that simple boolean? I'm not sure. Exposing this doesn't cost us anything and could help developers. Is there an implementation concern?

@martinthomson
Copy link
Member

we don't add API points without need usually, that's all

@mvano
Copy link
Contributor

mvano commented Dec 15, 2014

If we later decided that we wanted the boolean, there would be some subtlety in the backward compatibility. Authors would need to distinguish between (false) and (undefined or zero arguments).

@martinthomson
Copy link
Member

Yes, this closes certain avenues if we later determine that information like this is useful. That's not a reason to add features.

momchil-velikov pushed a commit to eth-sri/BlinkER that referenced this pull request Jan 14, 2015
Following recent spec change:
w3c/push-api#97

BUG=440958

Review URL: https://codereview.chromium.org/801393002

git-svn-id: svn://svn.chromium.org/blink/trunk@187164 bbb929c8-8fbe-4397-9dbb-9b2b20218538
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.

None yet

3 participants