Bug 178082 – [Readable Streams API] Align queue with spec for ReadableStreamDefaultController

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Romain Bellessort]

Align ReadableStreamDefaultController handling of queue with latest spec. This will in particular fix some consistency issues, especially with rounding errors. Also applies to WritableStream.

[Romain Bellessort]

Created attachment 323178 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 323178 [details]
Patch

Why did you decide to remove the queue object and subtitute it with two attributes?

[Romain Bellessort]

(In reply to Xabier Rodríguez Calvar from comment #2)
> Comment on attachment 323178 [details]
> Patch
> 
> Why did you decide to remove the queue object and subtitute it with two
> attributes?


I thought current implementation was based on a previous version of the spec and that, while implementing latest behavior for queue, it would be better to align the whole implementation (this in particular makes it easier to compare spec and implementation). 

Are there any drawbacks to replacing the queue object (with two attributes) by two attributes directly owned by the controller? (the need to define both attributes in WebCoreBuiltinNames.h is a significant drawback?) Of course if it is more appropriate, I can prepare a patch that preserves the queue object and only aligns with new behavior.

[Xabier Rodríguez Calvar]

(In reply to Romain Bellessort from comment #3)
> I thought current implementation was based on a previous version of the spec
> and that, while implementing latest behavior for queue, it would be better
> to align the whole implementation (this in particular makes it easier to
> compare spec and implementation). 
> 
> Are there any drawbacks to replacing the queue object (with two attributes)
> by two attributes directly owned by the controller? (the need to define both
> attributes in WebCoreBuiltinNames.h is a significant drawback?) Of course if
> it is more appropriate, I can prepare a patch that preserves the queue
> object and only aligns with new behavior.

There are any drawbacks in changing that. Af the moment Youenn and I decided that we preferred this implementation because it does not make too much sense having a queue with two attributes in the original object instead of having just a queue object with those two attributes.

This said, IMHO, I think the implementation would be clearer if you kept it as you could call @newQueue or make @resetQueue create a object with the right reset attributes.

[Romain Bellessort]

Created attachment 323301 [details]
Patch

[Romain Bellessort]

(In reply to Xabier Rodríguez Calvar from comment #4)
> (In reply to Romain Bellessort from comment #3)
> > I thought current implementation was based on a previous version of the spec
> > and that, while implementing latest behavior for queue, it would be better
> > to align the whole implementation (this in particular makes it easier to
> > compare spec and implementation). 
> > 
> > Are there any drawbacks to replacing the queue object (with two attributes)
> > by two attributes directly owned by the controller? (the need to define both
> > attributes in WebCoreBuiltinNames.h is a significant drawback?) Of course if
> > it is more appropriate, I can prepare a patch that preserves the queue
> > object and only aligns with new behavior.
> 
> There are any drawbacks in changing that. Af the moment Youenn and I decided
> that we preferred this implementation because it does not make too much
> sense having a queue with two attributes in the original object instead of
> having just a queue object with those two attributes.
> 
> This said, IMHO, I think the implementation would be clearer if you kept it
> as you could call @newQueue or make @resetQueue create a object with the
> right reset attributes.

Ok I see the point. I've uploaded a new patch which keeps queue as an object but implements the new behavior. In addition, I've refactored queue for ReadableByteStreamController, so that both queues are implemented in the same way.

[WebKit Commit Bot]

Comment on attachment 323301 [details]
Patch

Rejecting attachment 323301 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 323301, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ayoutTests/imported/w3c/ChangeLog
error: Error building trees

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog
You have both LayoutTests/imported/w3c and LayoutTests/imported/w3c/ChangeLog
error: Error building trees

Failed to run "['git', 'commit', '--all', '-F', '-']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/4822078

[Romain Bellessort]

Created attachment 323402 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323402 [details]
Patch

Rejecting attachment 323402 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 323402, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Xabier Rodriguez Calvar found in /Volumes/Data/EWS/WebKit/LayoutTests/imported/w3c/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/LayoutTests/imported/w3c/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/4823306

[Romain Bellessort]

Created attachment 323413 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323413 [details]
Patch

Rejecting attachment 323413 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 323413, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
    -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 223175 = f5e3797fe588fd54021993c447e477c2fc70ed4b
r223176 = 283af2e6841e7767eabb507a2b77271755074c16
r223177 = 35546d106cccda22c9f65cb741526f2e3674b1e9
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.

Full output: http://webkit-queues.webkit.org/results/4823931

[Romain Bellessort]

Created attachment 323515 [details]
Patch

[Romain Bellessort]

Created attachment 323530 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323530 [details]
Patch

Rejecting attachment 323530 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 323530, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
LayoutTests
:040000 040000 9dd96414628e6f08c6f8af5fb8a68f48513d96fb ad6233fe766704b81d65aac6b91abb82de0aa53b M	Source
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/4842589

[Romain Bellessort]

Created attachment 323646 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323646 [details]
Patch

Clearing flags on attachment: 323646

Committed r223279: <https://trac.webkit.org/changeset/223279>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34973793>