Bug 110841

Created attachment 190195 [details] Patch

This is to allow support of the Stream API. The complete version is at: https://bugs.webkit.org/show_bug.cgi?id=110194

Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

Comment on attachment 190195 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190195&action=review > Source/Platform/chromium/public/WebBlobRegistry.h:56 > + // Registers a stream URL referring to the specified stream data. I'm a little confused by these changes. Can you say more about why we need to differentiate streams and blobs at this layer? (Given that this interface is the WebBlobRegistry, it seems a bit awkward to add stream concepts.) Maybe what is going on here is that you need the ability to define a Blob that does not have WebBlobData yet? And, then you need to be able to asynchronously supply that data in chunks, as it arrives from XMLHttpRequest? Have you looked into ways of avoiding the round-trip through the renderer to support responseType="stream"? Maybe we could inform the content/browser/loader/ system to accumulate the data for us, so we don't have to echo it back to the browser process? nit: What stream data is the comment referring to?

(In reply to comment #4) > (From update of attachment 190195 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190195&action=review > > > Source/Platform/chromium/public/WebBlobRegistry.h:56 > > + // Registers a stream URL referring to the specified stream data. > > I'm a little confused by these changes. Can you say more about why we need to differentiate streams and blobs at this layer? (Given that this interface is the WebBlobRegistry, it seems a bit awkward to add stream concepts.) > I can move these functions to a WebStreamRegistry instead, if that would make more sense. I put it here because of the conceptual relation to blobs, as you mention. The differentiation between a blob and a stream at this point is to indicate that it should allow access before the data is finalized. > Maybe what is going on here is that you need the ability to define a Blob that does not have WebBlobData yet? And, then you need to be able to asynchronously supply that data in chunks, as it arrives from XMLHttpRequest? Have you looked into ways of avoiding the round-trip through the renderer to support responseType="stream"? Maybe we could inform the content/browser/loader/ system to accumulate the data for us, so we don't have to echo it back to the browser process? As you say, the idea is pretty much to register a "blob:" URL for a stream, and then chunk data to it. It's currently setup to match the BlobRegistry (or StreamRegistry) implementation I have. As to skipping the round trip with XHR, I'd like to make the initial implementation of this layer require minimal refactoring so that I can get tests in place, and then optimize. > > nit: What stream data is the comment referring to? I'll fix that comment. I originally sent the first chunk of data with the creation message.

Created attachment 190426 [details] Patch

Comment on attachment 190426 [details] Patch Attachment 190426 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/16774588 New failing tests: svg/as-image/img-preserveAspectRatio-support-2.html

> Maybe what is going on here is that you need the ability to define > a Blob that does not have WebBlobData yet? At the level of the interface between renderer and browser, we basically do have that ability, it's just not exposed to webcore in the WebBlobRegistry interface. Blobs currently come into existence fully formed and so that all the interface lets you do. There's pretty good reason to have blobs be incrementally built up (not just in the context of Streams). Would it help to extend the WebBlobRegistry api to more closely match the IPC messages (which do allow incremental building: StartBuilding, AddStuff, FinishBuilding). I think that could live in parallel with the existing 'fully formed' registry interface. Also, heads up... I've got some refactoring in the works in this area that I'm part way thru landing. These changes scrap using URLs as identifiers for WebCore::Blob instances, and instead coins GUID string identifiers which map to the underlying blob data (such that many Blob instances can refer to the same data). Switching around what the identifier identifies has made this challenging to land so I've been breaking off small parts and whittling down on it. WK - https://codereview.chromium.org/11192017/ CR - https://codereview.chromium.org/11410019/ The next chunks for me to land are sitting in here and here. https://bugs.webkit.org/show_bug.cgi?id=108851 https://codereview.chromium.org/12330162/ The meta bug I'm using to track these changes is here. https://code.google.com/p/chromium/issues/detail?id=174200

(In reply to comment #8) > > Maybe what is going on here is that you need the ability to define > > a Blob that does not have WebBlobData yet? > > At the level of the interface between renderer and browser, we basically do have that ability, it's just not exposed to webcore in the WebBlobRegistry interface. Blobs currently come into existence fully formed and so that all the interface lets you do. There's pretty good reason to have blobs be incrementally built up (not just in the context of Streams). > > Would it help to extend the WebBlobRegistry api to more closely match the IPC messages (which do allow incremental building: StartBuilding, AddStuff, FinishBuilding). I think that could live in parallel with the existing 'fully formed' registry interface. At this point, I don't think exposing those functions would help, as we would still have to add an extra set of APIs to change the semantics of the blob to match streams. > > > > Also, heads up... I've got some refactoring in the works in this area that I'm part way thru landing. These changes scrap using URLs as identifiers for WebCore::Blob instances, and instead coins GUID string identifiers which map to the underlying blob data (such that many Blob instances can refer to the same data). Switching around what the identifier identifies has made this challenging to land so I've been breaking off small parts and whittling down on it. > > WK - https://codereview.chromium.org/11192017/ > CR - https://codereview.chromium.org/11410019/ > > The next chunks for me to land are sitting in here and here. > https://bugs.webkit.org/show_bug.cgi?id=108851 > https://codereview.chromium.org/12330162/ > > The meta bug I'm using to track these changes is here. > https://code.google.com/p/chromium/issues/detail?id=174200 Thanks for the heads up! I'll look through those to figure out how it will effect my patches.

Comment on attachment 190426 [details] Patch Clearing review flag from this Chromium-specific patch.

Chromium-specific. Closing.