Remove DeferredPromise::sessionID()
Created attachment 376017 [details] Patch
Comment on attachment 376017 [details] Patch Attachment 376017 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12894156 New failing tests: imported/w3c/web-platform-tests/fetch/api/request/request-consume-empty.html http/wpt/fetch/request-stream-empty.html http/wpt/fetch/fetch-as-blob-worker.html imported/w3c/web-platform-tests/fetch/api/request/request-init-003.sub.html imported/w3c/web-platform-tests/fetch/api/response/response-consume-empty.html media/track/track-in-band-metadata-display-order.html imported/w3c/web-platform-tests/fetch/api/request/request-consume.html imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.html imported/w3c/web-platform-tests/fetch/api/response/response-consume.html http/wpt/fetch/fetch-as-blob.html imported/w3c/web-platform-tests/mimesniff/mime-types/parsing.any.worker.html
Created attachment 376038 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 376045 [details] Patch
Comment on attachment 376045 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376045&action=review Not sure I understand 100%. > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:69 > + promise->resolveCallbackValueWithNewlyCreated<IDLInterface<Blob>>([&](auto& context) { > + return blobFromData(context.sessionID(), data, length, contentType); > + }); Would be more obvious that it’s safe if we captured a local variable containing the session ID rather than a reference to the context object itself. A little less elegant, but easier to be sure there’s no object lifetime problem. Unless session IDs can change over time?
(In reply to Darin Adler from comment #5) > Comment on attachment 376045 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376045&action=review > > Not sure I understand 100%. The lambda is only called once we are sure the promise can be actually resolved. It can only be resolved if the context is valid. This has two benefits: - It only creates the blob if needed - It passes the context, which can be used for creating the C++ object or for getting the session ID. We could also try to stick the session ID into FetchBodyConsumer but this would require to pipe it from FetchRequest/FetchResponse up to FetchBodyConsumer, and we would anyway get it initially from the context. It seems nicer to get it from the promise itself when actually useful. > > Source/WebCore/Modules/fetch/FetchBodyConsumer.cpp:69 > > + promise->resolveCallbackValueWithNewlyCreated<IDLInterface<Blob>>([&](auto& context) { > > + return blobFromData(context.sessionID(), data, length, contentType); > > + }); > > Would be more obvious that it’s safe if we captured a local variable > containing the session ID rather than a reference to the context object > itself. A little less elegant, but easier to be sure there’s no object > lifetime problem. > > Unless session IDs can change over time? The session ID cannot change. The lambda is called synchronously, since we need to call the JS built-in resolve function with the JS value. I can change the lambda to capture [&data, &length, &contentType].
Created attachment 376264 [details] Patch
> This has two benefits: > - It only creates the blob if needed For instance, we currently allocate a vector of char to create the blob that will be the value given to the promise. In some cases, we create this value for nothing. With the current patch, the blob will not be created if not needed. > > Unless session IDs can change over time? > > The session ID cannot change. The session ID can change in theory. I believe this happens in practice for testing only. The implementation is probably not working super well since we are storing the session ID in various places that we would need to update (workers, IDB/SW/Cache connections...) if changing the page/document session ID.
Comment on attachment 376264 [details] Patch Clearing flags on attachment: 376264 Committed r248897: <https://trac.webkit.org/changeset/248897>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54504452>