Summary: | [Streams API] Vended promise capabilities should not need @resolve/@reject fields | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | benjamin, calvaris, commit-queue, darin | ||||
Priority: | P2 | ||||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | 150627 | ||||||
Bug Blocks: | |||||||
Attachments: |
|
Description
youenn fablet
2015-11-03 03:14:03 PST
Created attachment 264683 [details]
Patch
Comment on attachment 264683 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=264683&action=review > Source/WebCore/Modules/streams/WritableStreamInternals.js:52 > + const shouldApplyBackpressure = stream.@queue.size > stream.@strategy.highWaterMark; > + if (shouldApplyBackpressure && stream.@state === @streamWritable) { > stream.@state = @streamWaiting; > stream.@readyPromiseCapability = @newPromiseCapability(Promise); > - } else { > + } > + if (!shouldApplyBackpressure && stream.@state === @streamWaiting) { I personally dislike this option. I think it was better to use the functions to create the vended promises because we are closer to the spec and it is less error prone in case we have to change anything. Instead of this patch I would have functions to create vended promise capabilities as I did in earlier versions of patch for bug 150627. (In reply to comment #2) > Comment on attachment 264683 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=264683&action=review > > > Source/WebCore/Modules/streams/WritableStreamInternals.js:52 > > + const shouldApplyBackpressure = stream.@queue.size > stream.@strategy.highWaterMark; > > + if (shouldApplyBackpressure && stream.@state === @streamWritable) { > > stream.@state = @streamWaiting; > > stream.@readyPromiseCapability = @newPromiseCapability(Promise); > > - } else { > > + } > > + if (!shouldApplyBackpressure && stream.@state === @streamWaiting) { > > I personally dislike this option. I think it was better to use the functions > to create the vended promises because we are closer to the spec and it is > less error prone in case we have to change anything. Instead of this patch I > would have functions to create vended promise capabilities as I did in > earlier versions of patch for bug 150627. These specific changes make the implementation closer to the spec and removes the fact that we were resolving an already resolved promise. These changes should be done no matter whether we remove @resolve/@reject for vended promises. (In reply to comment #3) > These specific changes make the implementation closer to the spec and > removes the fact that we were resolving an already resolved promise. > > These changes should be done no matter whether we remove @resolve/@reject > for vended promises. You're right. I missed that from the spec that it was causing that the promises were vended more than once. Comment on attachment 264683 [details] Patch Clearing flags on attachment: 264683 Committed r191956: <http://trac.webkit.org/changeset/191956> All reviewed patches have been landed. Closing bug. |