yutakahirano / fetch-with-streams

Fetch API integrated with Streams.
66 stars 8 forks source link

Use IsDisturbed(ReadableStream) #48

Closed yutakahirano closed 9 years ago

yutakahirano commented 9 years ago

This fixes #45. The link should be modified after IsDisturbed is specified on the streams spec.

annevk commented 9 years ago

I see, the "passed flag" is a renaming of the "used flag". Perhaps we should call it "obsolete flag"? I don't think "passed" is very clear.

Also, we need to change Request's used predicate, since it needs to check for IsDisturbed.

I also think the "locked flag" can go, since we were going to define an abstract operation in Streams for text() etc. that would mark the stream disturbed immediately.

yutakahirano commented 9 years ago

@annevk Thanks, I uploaded a revised one.

I see, the "passed flag" is a renaming of the "used flag". Perhaps we should call it "obsolete flag"? I don't think "passed" is very clear.

It came from https://github.com/slightlyoff/ServiceWorker/issues/452 and yes that should be renamed. I think it's ok to call it as "used flag".

Also, we need to change Request's used predicate, since it needs to check for IsDisturbed.

We will, but currently Request doesn't have its body stream. used flag emulates the effect. Hence I'm using the flag for now.

I also think the "locked flag" can go, since we were going to define an abstract operation in Streams for text() etc. that would mark the stream disturbed immediately.

Fixed.

yutakahirano commented 9 years ago

I changed my mind: I decoupled disturbed flag from used flag. The former emulates IsDisturbed(stream) and the latter is defined in https://etherpad.mozilla.org/streams-f2f-july L45 and L46.

yutakahirano commented 9 years ago

Addressed review comments. My current understanding is:

Please correct me if I'm wrong.

annevk commented 9 years ago

That matches my understanding.

domenic commented 9 years ago

Mine as well.