Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12335087:
Implement the Stream registry in content (Closed)
|
Created:
7 years, 10 months ago by Zachary Kuznia Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement the Stream registry in content
This is the first part of the content side of the Streams api.
See:
https://bugs.webkit.org/show_bug.cgi?id=110194
https://dvcs.w3.org/hg/streams-api/raw-file/tip/Overview.htm
BUG=171585
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187230
Patch Set 1 #
Total comments: 24
Patch Set 2 : Code review fixes #
Total comments: 16
Patch Set 3 : Code review fixes #
Total comments: 12
Patch Set 4 : COde review fixes #
Total comments: 12
Patch Set 5 : Code review fixes #Patch Set 6 : Allow one reader at a time #Patch Set 7 : Code review fixes #
Total comments: 11
Patch Set 8 : nits + Unittest #
Total comments: 14
Patch Set 9 : Fix unittests #
Total comments: 11
Patch Set 10 : Code review fixes #
Messages
Total messages: 29 (0 generated)
Please take a look. Thanks, -Zach
Did the first (very shallow) review. Do you have a design note or something so that we can see what modules we'll have in chromium/WebKit at a glance? That'll help reviewing. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... File content/browser/streams/stream.cc (right): https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.cc:90: (size_t)buf_size < remaining_bytes ? buf_size : remaining_bytes; static_cast<size_t>(buf_size) https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.cc:95: } nit: no { } necessary for one-line body https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.cc:101: void Stream::SpaceAvailable() { I prefer naming these OnSpaceAvailable and OnDataAvailable (I mean if they sound good in native English speakers too) https://codereview.chromium.org/12335087/diff/1/content/browser/streams/stream.h File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:34: // Create a stream useable from the |security_origin| style-nit: Create -> Creates, end comment with '.' (here and elsewhere) https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:36: const GURL& url); style-nit: can you break the line after 'registry,' (Per style-guide: If parameters don't fit in one line put each parameter in different line in definition) https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:52: // returns false if there is no data available, but the stream is not closed. What is returned when the stream is closed? https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:58: void SetStreamUrl(GURL url) { stream_url_ = url; } nit: const GURL& https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:59: GURL stream_url() { return stream_url_; } could return const GURL& as well? https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:61: GURL& security_origin() { return security_origin_; } GURL& -> const GURL& https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:69: ~Stream(); virtual style-nit: can we move this after line 64? (ctor/dtor should come before other methods) https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... File content/browser/streams/stream_context.h (right): https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream_context.h:26: StreamContextDeleter> { nit: while we're in content/ I think we can use something like content::BrowserThread::DeleteOnIOThread (defined in content/public/browser/browser_thread.h) https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... File content/browser/streams/stream_registry.h (right): https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream_registry.h:18: class StreamRegistry { Can we make it inherit from base::NonThreadSafe and have thread checks in each method?
Currently, the only design notes I have are the prototype implementations, and meta-bug. The Chrome side consists of three parts: This CL. Resource Handler for blob: URLs that are attached to Streams Chrome implementation of WebBlobRegistry I can upload the unified Chrome patch separately, if that would help. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... File content/browser/streams/stream.cc (right): https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.cc:90: (size_t)buf_size < remaining_bytes ? buf_size : remaining_bytes; On 2013/02/26 07:37:55, kinuko wrote: > static_cast<size_t>(buf_size) Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.cc:95: } On 2013/02/26 07:37:55, kinuko wrote: > nit: no { } necessary for one-line body Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.cc:101: void Stream::SpaceAvailable() { On 2013/02/26 07:37:55, kinuko wrote: > I prefer naming these OnSpaceAvailable and OnDataAvailable > (I mean if they sound good in native English speakers too) Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/stream.h File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:34: // Create a stream useable from the |security_origin| On 2013/02/26 07:37:55, kinuko wrote: > style-nit: Create -> Creates, end comment with '.' (here and elsewhere) Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:36: const GURL& url); On 2013/02/26 07:37:55, kinuko wrote: > style-nit: can you break the line after 'registry,' > > (Per style-guide: If parameters don't fit in one line put each parameter in > different line in definition) Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:52: // returns false if there is no data available, but the stream is not closed. On 2013/02/26 07:37:55, kinuko wrote: > What is returned when the stream is closed? Improved the comment. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:58: void SetStreamUrl(GURL url) { stream_url_ = url; } On 2013/02/26 07:37:55, kinuko wrote: > nit: const GURL& Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:59: GURL stream_url() { return stream_url_; } On 2013/02/26 07:37:55, kinuko wrote: > could return const GURL& as well? Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:61: GURL& security_origin() { return security_origin_; } On 2013/02/26 07:37:55, kinuko wrote: > GURL& -> const GURL& Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream.h:69: ~Stream(); On 2013/02/26 07:37:55, kinuko wrote: > virtual > > style-nit: can we move this after line 64? (ctor/dtor should come before other > methods) Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... File content/browser/streams/stream_context.h (right): https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream_context.h:26: StreamContextDeleter> { On 2013/02/26 07:37:55, kinuko wrote: > nit: while we're in content/ I think we can use something like > content::BrowserThread::DeleteOnIOThread (defined in > content/public/browser/browser_thread.h) Done. https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... File content/browser/streams/stream_registry.h (right): https://codereview.chromium.org/12335087/diff/1/content/browser/streams/strea... content/browser/streams/stream_registry.h:18: class StreamRegistry { On 2013/02/26 07:37:55, kinuko wrote: > Can we make it inherit from base::NonThreadSafe and have thread checks in each > method? Done.
https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream.h:26: // A stream that sends data from an existing ResourceHandler to an internal URL nit: probably best not to mention ResourceHandler here. this system is independent of the details of ResourceHandler. https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream.h:28: // original URL as long as there is data available. It can be read from from nit: "from from" https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream.h:60: void SetStreamUrl(const GURL& url) { stream_url_ = url; } nit: set_stream_url actually, since the class name is Stream, perhaps the property doesn't have to repeat the substring "Stream"? can this just be set_url() and url() methods? https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream.h:61: const GURL& stream_url() { return stream_url_; } nit: getters probably should be 'const' methods https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... File content/browser/streams/stream_context.h (right): https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream_context.h:15: struct StreamContextDeleter; nit: "c"lass before "s"truct is how we typically sort these https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... File content/browser/streams/stream_registry.cc (right): https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream_registry.cc:27: GURL url(std::string(chrome::kBlobScheme) + ":" + nit: it feels like this URL generation should be done by Blob code, so that it stays in sync with the way the Blob code generates its URLs. https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream_registry.cc:39: } else { nit: no need for else after return https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... File content/browser/streams/stream_registry.h (right): https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream_registry.h:36: void OnStreamConsumed(Stream* stream); this just looks like a variation of UnregisterStream. maybe the caller of OnStreamConsumed should just call UnregisterStream(stream->url()) instead?
https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream.h:26: // A stream that sends data from an existing ResourceHandler to an internal URL On 2013/02/27 06:18:59, darin wrote: > nit: probably best not to mention ResourceHandler here. this system is > independent of the details of ResourceHandler. Changed to arbitrary source. https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream.h:28: // original URL as long as there is data available. It can be read from from On 2013/02/27 06:18:59, darin wrote: > nit: "from from" Done. https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream.h:60: void SetStreamUrl(const GURL& url) { stream_url_ = url; } On 2013/02/27 06:18:59, darin wrote: > nit: set_stream_url > > actually, since the class name is Stream, perhaps the property doesn't have to > repeat the substring "Stream"? can this just be set_url() and url() methods? Done. https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream.h:61: const GURL& stream_url() { return stream_url_; } On 2013/02/27 06:18:59, darin wrote: > nit: getters probably should be 'const' methods Done. https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... File content/browser/streams/stream_context.h (right): https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream_context.h:15: struct StreamContextDeleter; On 2013/02/27 06:18:59, darin wrote: > nit: "c"lass before "s"truct is how we typically sort these Done. https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... File content/browser/streams/stream_registry.cc (right): https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream_registry.cc:27: GURL url(std::string(chrome::kBlobScheme) + ":" + On 2013/02/27 06:18:59, darin wrote: > nit: it feels like this URL generation should be done by Blob code, so that it > stays in sync with the way the Blob code generates its URLs. I'm going to remove this code from this CL, since it won't be needed to support Streams in WebKit and XHR. I'll look for a better way to share the code with blobs when I add in support for extension mime type handling. https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream_registry.cc:39: } else { On 2013/02/27 06:18:59, darin wrote: > nit: no need for else after return Done. https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... File content/browser/streams/stream_registry.h (right): https://codereview.chromium.org/12335087/diff/5001/content/browser/streams/st... content/browser/streams/stream_registry.h:36: void OnStreamConsumed(Stream* stream); On 2013/02/27 06:18:59, darin wrote: > this just looks like a variation of UnregisterStream. maybe the caller of > OnStreamConsumed should just call UnregisterStream(stream->url()) instead? Done.
I'm still not clear on why a Stream needs to have multiple readers and multiple writers. https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:13: #include "content/browser/download/byte_stream.h" nit: There should probably be a TODO to factor byte_stream.h out of the download system. (It doesn't make sense for streams to depend on the download system.) I recommend sync'ing up with rdsmith@ on this since he authored the class. https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... File content/browser/streams/stream_context.h (right): https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream_context.h:43: friend class base::DeleteHelper<StreamContext>; nit: looks like you repeated the DeleteHelper<StreamContext> friend. all of these friend decls shouldn't be required.
On 2013/03/07 07:54:03, darin wrote: > I'm still not clear on why a Stream needs to have multiple readers and multiple > writers. > There should only ever be one writer. There can be multiple readers if the stream has multiple StreamReader/FileReaders attached to it. In theory, these readers should not be reading at the same time, but this isn't specified in the w3c spec. > https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... > File content/browser/streams/stream.h (right): > > https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... > content/browser/streams/stream.h:13: #include > "content/browser/download/byte_stream.h" > nit: There should probably be a TODO to factor byte_stream.h out of the download > system. (It doesn't make sense for streams to depend on the download system.) > I recommend sync'ing up with rdsmith@ on this since he authored the class. > Added a TODO to byte_stream.h and filed a bug: https://code.google.com/p/chromium/issues/detail?id=180833 > https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... > File content/browser/streams/stream_context.h (right): > > https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... > content/browser/streams/stream_context.h:43: friend class > base::DeleteHelper<StreamContext>; > nit: looks like you repeated the DeleteHelper<StreamContext> friend. all of > these friend decls shouldn't be required. Fixed.
On Thu, Mar 7, 2013 at 12:10 AM, <zork@chromium.org> wrote: > On 2013/03/07 07:54:03, darin wrote: > >> I'm still not clear on why a Stream needs to have multiple readers and >> > multiple > >> writers. >> > > > There should only ever be one writer. There can be multiple readers if the > stream has multiple StreamReader/FileReaders attached to it. In theory, > these > readers should not be reading at the same time, but this isn't specified > in the > w3c spec. > > > If there can only be one writer, then Stream should only allow one writer, right? In the case of multiple readers, I had been thinking in terms of a Stream that works more like a socket or pipe. The data is removed from the Stream when read by a consumer. I can see how that wouldn't work if we said that a Blob is a Stream, which I think is part of the goal of the W3C spec. Hmm... -Darin > > https://codereview.chromium.**org/12335087/diff/10001/** > content/browser/streams/**stream.h<https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/stream.h> > >> File content/browser/streams/**stream.h (right): >> > > > https://codereview.chromium.**org/12335087/diff/10001/** > content/browser/streams/**stream.h#newcode13<https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/stream.h#newcode13> > >> content/browser/streams/**stream.h:13: #include >> "content/browser/download/**byte_stream.h" >> nit: There should probably be a TODO to factor byte_stream.h out of the >> > download > >> system. (It doesn't make sense for streams to depend on the download >> system.) >> > > I recommend sync'ing up with rdsmith@ on this since he authored the >> class. >> > > > Added a TODO to byte_stream.h and filed a bug: > https://code.google.com/p/**chromium/issues/detail?id=**180833<https://code.g... > > > > https://codereview.chromium.**org/12335087/diff/10001/** > content/browser/streams/**stream_context.h<https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/stream_context.h> > >> File content/browser/streams/**stream_context.h (right): >> > > > https://codereview.chromium.**org/12335087/diff/10001/** > content/browser/streams/**stream_context.h#newcode43<https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/stream_context.h#newcode43> > >> content/browser/streams/**stream_context.h:43: friend class >> base::DeleteHelper<**StreamContext>; >> nit: looks like you repeated the DeleteHelper<StreamContext> friend. all >> of >> these friend decls shouldn't be required. >> > > Fixed. > > https://codereview.chromium.**org/12335087/<https://codereview.chromium.org/1... >
Sorry for my belated review. https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:8: #include <deque> not used? https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:53: // returns false if there is no data available, but the stream is not nit: returns -> Returns https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:54: // finalized, and true otherwise. This behavior still confuses me a bit. So we return true 1. when we have data and also 2. when the stream is closed and has no data? Can we make the return value have a single meaning, say by adding another out parameter etc? https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:60: void SetUrl(const GURL& url) { url_ = url; } nit: set_url() should be fine for simple inline setter Do we need this setter btw? It looks the StreamRegistry could be in a troubled state if the value's changed later. https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:85: StreamRegistry* registry_; This one's ref-counted while StreamRegistry is not. Isn't there a case where a Stream instance could outlive registry? https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... File content/browser/streams/stream.cc (right): https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream.cc:15: // Start throttling the connection at about 1MB nit: please end comment with '.' https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... File content/browser/streams/stream_registry.cc (right): https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:13: #include "content/public/common/url_constants.h" Some of the includes look unnecessary https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:21: DCHECK(CalledOnValidThread()); nit: the dtor of NonThreadSafe will check this https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:30: scoped_refptr<Stream> StreamRegistry::GetStream(const GURL& url) { nit: DCHECK(CalledOnValidThread()); ? https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:34: } style-nit: no need of { } for one-line body https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:43: streams_[url] = stream; Is this ok not to indicate any errors if (!stream) ?
On 2013/03/07 08:32:49, darin wrote: > On Thu, Mar 7, 2013 at 12:10 AM, <mailto:zork@chromium.org> wrote: > > > On 2013/03/07 07:54:03, darin wrote: > > > >> I'm still not clear on why a Stream needs to have multiple readers and > >> > > multiple > > > >> writers. > >> > > > > > > There should only ever be one writer. There can be multiple readers if the > > stream has multiple StreamReader/FileReaders attached to it. In theory, > > these > > readers should not be reading at the same time, but this isn't specified > > in the > > w3c spec. > > > > > > > If there can only be one writer, then Stream should only allow one writer, > right? > > In the case of multiple readers, I had been thinking in terms of a Stream > that works more like a socket or pipe. The data is removed from the Stream > when read by a consumer. I can see how that wouldn't work if we said that > a Blob is a Stream, which I think is part of the goal of the W3C spec. > Hmm... > > -Darin I'll change it so there can only be one write observer. Is there any additional way I should enforce only one writer? This should work like a pipe as it is written; data is removed from the Stream once read, and cannot be read by any other source. The buffer size received from ByteStream can't be controller, though, so it needs to be kept around if the current read doesn't consume all the bytes.
On Thu, Mar 7, 2013 at 5:32 PM, Darin Fisher <darin@chromium.org> wrote: > > > On Thu, Mar 7, 2013 at 12:10 AM, <zork@chromium.org> wrote: > >> On 2013/03/07 07:54:03, darin wrote: >> >>> I'm still not clear on why a Stream needs to have multiple readers and >>> >> multiple >> >>> writers. >>> >> >> >> There should only ever be one writer. There can be multiple readers if >> the >> stream has multiple StreamReader/FileReaders attached to it. In theory, >> these >> readers should not be reading at the same time, but this isn't specified >> in the >> w3c spec. >> >> >> > If there can only be one writer, then Stream should only allow one writer, > right? > > In the case of multiple readers, I had been thinking in terms of a Stream > that works more like a socket or pipe. The data is removed from the Stream > when read by a consumer. I can see how that wouldn't work if we said that > a Blob is a Stream, which I think is part of the goal of the W3C spec. > Hmm... > If this is only to satisfy the ambiguity in the spec (which is getting some push back) and if this changeset primarily wants to focus on fixing the QuickOffice issue could we start with an assumption that we only allow single reader/writer at a time? -Darin > > > >> >> https://codereview.chromium.**org/12335087/diff/10001/** >> content/browser/streams/**stream.h<https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/stream.h> >> >>> File content/browser/streams/**stream.h (right): >>> >> >> >> https://codereview.chromium.**org/12335087/diff/10001/** >> content/browser/streams/**stream.h#newcode13<https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/stream.h#newcode13> >> >>> content/browser/streams/**stream.h:13: #include >>> "content/browser/download/**byte_stream.h" >>> nit: There should probably be a TODO to factor byte_stream.h out of the >>> >> download >> >>> system. (It doesn't make sense for streams to depend on the download >>> system.) >>> >> >> I recommend sync'ing up with rdsmith@ on this since he authored the >>> class. >>> >> >> >> Added a TODO to byte_stream.h and filed a bug: >> https://code.google.com/p/**chromium/issues/detail?id=**180833<https://code.g... >> >> >> >> https://codereview.chromium.**org/12335087/diff/10001/** >> content/browser/streams/**stream_context.h<https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/stream_context.h> >> >>> File content/browser/streams/**stream_context.h (right): >>> >> >> >> https://codereview.chromium.**org/12335087/diff/10001/** >> content/browser/streams/**stream_context.h#newcode43<https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/stream_context.h#newcode43> >> >>> content/browser/streams/**stream_context.h:43: friend class >>> base::DeleteHelper<**StreamContext>; >>> nit: looks like you repeated the DeleteHelper<StreamContext> friend. >>> all of >>> these friend decls shouldn't be required. >>> >> >> Fixed. >> >> https://codereview.chromium.**org/12335087/<https://codereview.chromium.org/1... >> > >
https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... File content/browser/streams/stream.cc (right): https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream.cc:15: // Start throttling the connection at about 1MB On 2013/03/07 08:35:40, kinuko wrote: > nit: please end comment with '.' Done. https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... File content/browser/streams/stream_registry.cc (right): https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:13: #include "content/public/common/url_constants.h" On 2013/03/07 08:35:40, kinuko wrote: > Some of the includes look unnecessary Removed. https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:21: DCHECK(CalledOnValidThread()); On 2013/03/07 08:35:40, kinuko wrote: > nit: the dtor of NonThreadSafe will check this Done. https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:30: scoped_refptr<Stream> StreamRegistry::GetStream(const GURL& url) { On 2013/03/07 08:35:40, kinuko wrote: > nit: DCHECK(CalledOnValidThread()); ? Done. https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:34: } On 2013/03/07 08:35:40, kinuko wrote: > style-nit: no need of { } for one-line body Done. https://codereview.chromium.org/12335087/diff/14001/content/browser/streams/s... content/browser/streams/stream_registry.cc:43: streams_[url] = stream; On 2013/03/07 08:35:40, kinuko wrote: > Is this ok not to indicate any errors if (!stream) ? Added a return value.
Can you also address my comments made on patch set 3? (The changeset gets updated while I'm commenting) https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s...
> If this is only to satisfy the ambiguity in the spec (which is getting some > push back) and if this changeset primarily wants to focus on fixing the > QuickOffice issue could we start with an assumption that we only allow > single reader/writer at a time? Sounds good to me. Changed to allow only one reader at a time.
https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:8: #include <deque> On 2013/03/07 08:35:40, kinuko wrote: > not used? Done. https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:53: // returns false if there is no data available, but the stream is not On 2013/03/07 08:35:40, kinuko wrote: > nit: returns -> Returns Done. https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:54: // finalized, and true otherwise. On 2013/03/07 08:35:40, kinuko wrote: > This behavior still confuses me a bit. So we return true 1. when we have data > and also 2. when the stream is closed and has no data? Can we make the return > value have a single meaning, say by adding another out parameter etc? Changed to return an enum that matches ByteStream. https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:60: void SetUrl(const GURL& url) { url_ = url; } On 2013/03/07 08:35:40, kinuko wrote: > nit: set_url() should be fine for simple inline setter > > Do we need this setter btw? It looks the StreamRegistry could be in a troubled > state if the value's changed later. Removed. https://codereview.chromium.org/12335087/diff/10001/content/browser/streams/s... content/browser/streams/stream.h:85: StreamRegistry* registry_; On 2013/03/07 08:35:40, kinuko wrote: > This one's ref-counted while StreamRegistry is not. Isn't there a case where a > Stream instance could outlive registry? There should not be such a case. Other Stream references should be cleaned up before the registry at shutdown.
Some more nits. Can we have some unittests sometime? https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream.h:28: // it along to another client to continue processing the stream. Should we also update the comment? https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream.h:59: // and STREAM_COMPLETE if the stream is finalized and all data has been read. Thanks, this looks much clearer. https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... File content/browser/streams/stream_context.cc (right): https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream_context.cc:15: static const char* kStreamContextKeyName = "content_stream_context"; nit: maybe put this line in an anonymous namespace and remove static? (more c++'ish) https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... File content/browser/streams/stream_registry.h (right): https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream_registry.h:13: #include "content/browser/streams/stream.h" nit: I think you can use forward decl and move this include to .cc. https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream_registry.h:27: // Clones a stream. Can you comment about the return value?
https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream.h:44: // is already a reader. It'd be nice to note that this doesn't exactly match with the spec'ed behavior.
Added unittests. https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream.h:28: // it along to another client to continue processing the stream. On 2013/03/07 09:35:12, kinuko wrote: > Should we also update the comment? Done. https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream.h:44: // is already a reader. On 2013/03/07 09:41:23, kinuko wrote: > It'd be nice to note that this doesn't exactly match with the spec'ed behavior. This shouldn't impact the behavior in the Stream spec, as this case can be handled in the StreamReader/FileReader https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... File content/browser/streams/stream_context.cc (right): https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream_context.cc:15: static const char* kStreamContextKeyName = "content_stream_context"; On 2013/03/07 09:35:12, kinuko wrote: > nit: maybe put this line in an anonymous namespace and remove static? (more > c++'ish) Done. https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... File content/browser/streams/stream_registry.h (right): https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream_registry.h:13: #include "content/browser/streams/stream.h" On 2013/03/07 09:35:12, kinuko wrote: > nit: I think you can use forward decl and move this include to .cc. Done. https://codereview.chromium.org/12335087/diff/24001/content/browser/streams/s... content/browser/streams/stream_registry.h:27: // Clones a stream. On 2013/03/07 09:35:12, kinuko wrote: > Can you comment about the return value? Done.
Thanks for adding unittests! https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream.h:43: // Set the reader of this stream. Returns true on success, or false if there nit: Set -> Sets https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream.h:47: // Remove the read observer. |observer| must be the current observer. nit: Remove -> Removes https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream.h:72: ~Stream(); maybe we'd better make this virtual? https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... File content/browser/streams/stream_unittest.cc (right): https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream_unittest.cc:12: Can we put this entire code in namespace content? (And then we can remove content:: prefixes) https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream_unittest.cc:16: : producing_seed_key_(0) { nit: looks like this can be in one line https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream_unittest.cc:25: // by ValidateIOBuffer. This ValidateIOBuffer doesn't exist? https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream_unittest.cc:130: new base::TestSimpleTaskRunner()); Not used? (Here and below)
https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream.h:43: // Set the reader of this stream. Returns true on success, or false if there On 2013/03/08 07:04:19, kinuko wrote: > nit: Set -> Sets Done. https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream.h:47: // Remove the read observer. |observer| must be the current observer. On 2013/03/08 07:04:19, kinuko wrote: > nit: Remove -> Removes Done. https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream.h:72: ~Stream(); On 2013/03/08 07:04:19, kinuko wrote: > maybe we'd better make this virtual? Done. https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... File content/browser/streams/stream_unittest.cc (right): https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream_unittest.cc:12: On 2013/03/08 07:04:19, kinuko wrote: > Can we put this entire code in namespace content? (And then we can remove > content:: prefixes) Done. https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream_unittest.cc:16: : producing_seed_key_(0) { On 2013/03/08 07:04:19, kinuko wrote: > nit: looks like this can be in one line Done. https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream_unittest.cc:25: // by ValidateIOBuffer. On 2013/03/08 07:04:19, kinuko wrote: > This ValidateIOBuffer doesn't exist? Fixed the comment. https://codereview.chromium.org/12335087/diff/30001/content/browser/streams/s... content/browser/streams/stream_unittest.cc:130: new base::TestSimpleTaskRunner()); On 2013/03/08 07:04:19, kinuko wrote: > Not used? (Here and below) Done.
https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... File content/browser/streams/stream_unittest.cc (right): https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream_unittest.cc:191: scoped_refptr<Stream> stream2 = registry_->GetStream(url2); If I read the code correctly currently if we clone a stream its reader/writer are also shared, so say if one clones a stream while it's being read by a reader the cloned stream cannot have another reader. Is this correct? If so is it ok/intentional in your entire code design?
On 2013/03/08 07:37:22, kinuko wrote: > https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... > File content/browser/streams/stream_unittest.cc (right): > > https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... > content/browser/streams/stream_unittest.cc:191: scoped_refptr<Stream> stream2 = > registry_->GetStream(url2); > If I read the code correctly currently if we clone a stream its reader/writer > are also shared, so say if one clones a stream while it's being read by a reader > the cloned stream cannot have another reader. Is this correct? If so is it > ok/intentional in your entire code design? Yes, that's correct. Cloning will be used to create public URLs for the StreamReader/FileReader, and should still reference the same object.
ok, lgtm https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... File content/browser/streams/stream_registry.cc (right): https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream_registry.cc:18: DCHECK(CalledOnValidThread()); nit: DCHECK(stream) also here maybe
LGTM modulo nits https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream.h:45: // is already a reader. so in this version of the code, you can only have one reader? sorry for being nit-picky about this, but is this just a temporary thing? if not, then it seems like the API should just support one reader for now, in which case you could probably simplify things a bit (e.g., no need for RemoveReadObserver, and SetReadObserver could just CHECK(false) if called twice, or maybe read observer could be a constructor arg?). https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... File content/browser/streams/stream_registry.cc (right): https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream_registry.cc:25: std::map<GURL, scoped_refptr<Stream> >::iterator stream = streams_.find(url); nit: use a const_iterator since you don't need to mutate the map? https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream_registry.cc:38: } else { nit: no need for else after return https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... File content/browser/streams/stream_registry.h (right): https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream_registry.h:40: std::map<GURL, scoped_refptr<Stream> > streams_; nit: I recommend creating a typedef for this std::map. That way, you don't have to repeat the template args when declaring iterators.
https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... File content/browser/streams/stream.h (right): https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream.h:45: // is already a reader. On 2013/03/09 00:43:05, darin wrote: > so in this version of the code, you can only have one reader? sorry for being > nit-picky about this, but is this just a temporary thing? if not, then it seems > like the API should just support one reader for now, in which case you could > probably simplify things a bit (e.g., no need for RemoveReadObserver, and > SetReadObserver could just CHECK(false) if called twice, or maybe read observer > could be a constructor arg?). This is to allow the use case where a StreamReader/FileReader reads a fixed number of bytes. When it's done, it removes itself as a reader, and the next reader can attach to read another chunk of bytes. https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... File content/browser/streams/stream_registry.cc (right): https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream_registry.cc:18: DCHECK(CalledOnValidThread()); On 2013/03/08 10:16:52, kinuko wrote: > nit: DCHECK(stream) also here maybe Done. https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream_registry.cc:25: std::map<GURL, scoped_refptr<Stream> >::iterator stream = streams_.find(url); On 2013/03/09 00:43:05, darin wrote: > nit: use a const_iterator since you don't need to mutate the map? Done. https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream_registry.cc:38: } else { On 2013/03/09 00:43:05, darin wrote: > nit: no need for else after return Done. https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... File content/browser/streams/stream_registry.h (right): https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/s... content/browser/streams/stream_registry.h:40: std::map<GURL, scoped_refptr<Stream> > streams_; On 2013/03/09 00:43:05, darin wrote: > nit: I recommend creating a typedef for this std::map. That way, you don't have > to repeat the template args when declaring iterators. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zork@chromium.org/12335087/17002
Message was sent while issue was closed.
Change committed as 187230
On Sun, Mar 10, 2013 at 6:32 PM, <zork@chromium.org> wrote: > > https://codereview.chromium.**org/12335087/diff/43001/** > content/browser/streams/**stream.h<https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/stream.h> > File content/browser/streams/**stream.h (right): > > https://codereview.chromium.**org/12335087/diff/43001/** > content/browser/streams/**stream.h#newcode45<https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/stream.h#newcode45> > content/browser/streams/**stream.h:45: // is already a reader. > On 2013/03/09 00:43:05, darin wrote: > >> so in this version of the code, you can only have one reader? sorry >> > for being > >> nit-picky about this, but is this just a temporary thing? if not, >> > then it seems > >> like the API should just support one reader for now, in which case you >> > could > >> probably simplify things a bit (e.g., no need for RemoveReadObserver, >> > and > >> SetReadObserver could just CHECK(false) if called twice, or maybe read >> > observer > >> could be a constructor arg?). >> > > This is to allow the use case where a StreamReader/FileReader reads a > fixed number of bytes. When it's done, it removes itself as a reader, > and the next reader can attach to read another chunk of bytes. > > > Ah, makes sense. Thanks for the explanation. > https://codereview.chromium.**org/12335087/diff/43001/** > content/browser/streams/**stream_registry.cc<https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/stream_registry.cc> > File content/browser/streams/**stream_registry.cc (right): > > https://codereview.chromium.**org/12335087/diff/43001/** > content/browser/streams/**stream_registry.cc#newcode18<https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/stream_registry.cc#newcode18> > content/browser/streams/**stream_registry.cc:18: > DCHECK(CalledOnValidThread()); > > On 2013/03/08 10:16:52, kinuko wrote: > >> nit: DCHECK(stream) also here maybe >> > > Done. > > > https://codereview.chromium.**org/12335087/diff/43001/** > content/browser/streams/**stream_registry.cc#newcode25<https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/stream_registry.cc#newcode25> > content/browser/streams/**stream_registry.cc:25: std::map<GURL, > scoped_refptr<Stream> >::iterator stream = streams_.find(url); > On 2013/03/09 00:43:05, darin wrote: > >> nit: use a const_iterator since you don't need to mutate the map? >> > > Done. > > > https://codereview.chromium.**org/12335087/diff/43001/** > content/browser/streams/**stream_registry.cc#newcode38<https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/stream_registry.cc#newcode38> > content/browser/streams/**stream_registry.cc:38: } else { > On 2013/03/09 00:43:05, darin wrote: > >> nit: no need for else after return >> > > Done. > > > https://codereview.chromium.**org/12335087/diff/43001/** > content/browser/streams/**stream_registry.h<https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/stream_registry.h> > File content/browser/streams/**stream_registry.h (right): > > https://codereview.chromium.**org/12335087/diff/43001/** > content/browser/streams/**stream_registry.h#newcode40<https://codereview.chromium.org/12335087/diff/43001/content/browser/streams/stream_registry.h#newcode40> > content/browser/streams/**stream_registry.h:40: std::map<GURL, > scoped_refptr<Stream> > streams_; > On 2013/03/09 00:43:05, darin wrote: > >> nit: I recommend creating a typedef for this std::map. That way, you >> > don't have > >> to repeat the template args when declaring iterators. >> > > Done. > > https://codereview.chromium.**org/12335087/<https://codereview.chromium.org/1... > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
