Fix #237 and #256: handle subscription errors

[zolkis]

Signed-off-by: Zoltan Kis [email protected]

---

Preview | Diff

[zolkis]

Notes:

• Added Subscription interface
• the stop() method name was selected to be aligned with start() and stop() from similar interfaces (e.g. in Generic Sensors)
• also, the name stop() makes it possible to add a start() method if we want to support temporary stop/start functionality for subscriptions.

Now since stop() replaces unsubscribeEvent() and unobserveProperty(), it has to inherit passing InteractionOptions and needs to return a Promise.

[zolkis]

Also, factored out the selecting the unsubscribe Form for a given subscribe Form, which is marked non-normative.
Help is welcome on improving it.

[relu91]

Thanks @zolkis. I left some comments they are not really changes but suggestions. Feel free to ignore them .

On the other hand, now that I am seeing this new API defined I think we are trying too hard to merge two different API styles. The Subscription objects feels like EventTarget but it is not, since the event callbacks are passed to subscribeEvent/observeEvent method. On the other hand, we'd like to have a promise style for starting/stopping the underlying process. I know that I voted for this style, but it does not feel right. Futhermore, this paragraph from Web Platform Design Principles struck me:

For recurring events, it could be convenient to create a custom pair of APIs to "register"/"unregister", "subscribe"/"unsubscribe", etc., 
that take a callback and get invoked multiple times until paired cancel API is called.

Instead, use the existing event registration pattern and separate API controls to start/stop the underlying process (since event listeners should not have side-effects).

Since you are already suggesting that in the future we might add also the startmethod, we could go directly with an API similar to GenericSensor API. This means, that Subscription has the start method and the subscribeEvent/observeProperty will not return a Promise. What do you think?

[relu91]

I'd rephrase a little bit:

Whenever the underlying platform receives an error notification or detects other errors (i.e. connection errors) for this subscriptions...

Mentioning only notifications it seems that the errors are only generated at the application level (also the unrecoverable). Even if later on the steps explains what to do with unrecoverable error, the source of this errors is not clear.

[zolkis]

We could just say

Whenever the underlying platform detects errors (i.e. connection errors) for this subscription

[relu91]

I think it is a fair algorithm, even if we really need a change in TD spec. Meanwhile, I think we can get away with the note, and this solution.

We can improve it a little bit if instead of having a result array, we just record the maximum scored form, and if we find someone with higher score update the variable.

[zolkis]

Also, since it is specifically informative, implementations are free to use a different algorithm.

[zolkis]

About the array, it permits a single pass over an unordered set. Keep in mind that contributions for match level could come from different sources. While this algorithm could be optimized in that regard, I deliberately kept it this way to draw attention to this aspect (order of Forms should not affect the outcome of the search). Implementations could choose their algorithm anyway.

Also, feel free to propose an alternative algorithm, there probably is a better one, this was just a quick one.

[danielpeintner]

On the other hand, now that I am seeing this new API defined I think we are trying too hard to merge two different API styles. The Subscription objects feels like EventTarget but it is not, since the event callbacks are passed to subscribeEvent/observeEvent method. On the other hand, we'd like to have a promise style for starting/stopping the underlying process. I know that I voted for this style, but it does not feel right. Futhermore, this paragraph from Web Platform Design Principles struck me:

@relu91 can you sketch the WebIDL how you would like to see it so that we have a better idea how it might look like... thanks!

[zolkis]

On the other hand, now that I am seeing this new API defined I think we are trying too hard to merge two different API styles. The Subscription objects feels like EventTarget but it is not, since the event callbacks are passed to subscribeEvent/observeEvent method. On the other hand, we'd like to have a promise style for starting/stopping the underlying process. I know that I voted for this style, but it does not feel right.

I think there are legitimate uses of a subscribe mechanism, see for instance the Cookie Store API which uses both events and subscribe (with Promise).

I have evaluated the option with returning the Subscription object that has a start() method, in 2 scenarios:

• with Promise, and start() is invoked automatically by subscribe()
• without Promise, one should explicitly start() the subscription.

However, the latter doesn't play well with the semantics of a subscribe() function.

So either I would leave it as it is now, or include a start() method, but let the subscribe() invoke it, returning start()'s Promise.
But it's a complication, since start() should be void if we follow the Generic Sensor design (but that one has events so it's fine its start() is void). In our case, start() would need to return a Promise.

So in the end I think this proposal is the minimal one.

[danielpeintner]

I am ok with the current WebIDL changes.

More/other improvements can come later...