| Summary: | [Readable Streams API] Align queue with spec for ReadableStreamDefaultController | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Romain Bellessort <romain.wkt> | ||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Romain Bellessort <romain.wkt> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | calvaris, commit-queue, webkit-bug-importer, youennf | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Romain Bellessort
2017-10-09 08:44:42 PDT
Created attachment 323178 [details]
Patch
Comment on attachment 323178 [details]
Patch
Why did you decide to remove the queue object and subtitute it with two attributes?
(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. (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. Created attachment 323301 [details]
Patch
(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. 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 Created attachment 323402 [details]
Patch
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 Created attachment 323413 [details]
Patch
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 Created attachment 323515 [details]
Patch
Created attachment 323530 [details]
Patch
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 Created attachment 323646 [details]
Patch
Comment on attachment 323646 [details] Patch Clearing flags on attachment: 323646 Committed r223279: <https://trac.webkit.org/changeset/223279> All reviewed patches have been landed. Closing bug. |