Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
178473022:
MTP Streaming: Readahead Buffer (Closed)
|
Created:
6 years, 9 months ago by tommycli Modified:
6 years, 9 months ago Reviewers:
vandebo (ex-Chrome) CC:
chromium-reviews, tzik, Lei Zhang, nhiroki, tommycli, Greg Billock, kinuko+watch Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMTP Streaming: Readahead Buffer
BUG=110119
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255210
Patch Set 1 : #Patch Set 2 : #
Total comments: 22
Patch Set 3 : #
Total comments: 20
Patch Set 4 : address vandebo comments (except reusable bufs) #Patch Set 5 : #Patch Set 6 : this patchset allows memory reuse #
Total comments: 10
Patch Set 7 : #
Total comments: 10
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : change kDesiredNumberOfBuffers to unsigned #
Messages
Total messages: 27 (0 generated)
vandebo: I've tested this and it works, but the code feels kind of janky. Improvement suggestions?
https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:52: // We are waiting for an underlying read to complete, so save the request. DCHECK(!pending_read.get()); https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:64: : buf(buf), buf_len(buf_len), callback(callback) { http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... Multiple lines - I know git cl format does it this way... https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:72: Probably want a while loop here - there might only be one byte left in the current buffer with a request for 2MB, which should consume both buffers. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:96: scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(kBufferSize)); Why not make a DrainableIOBuffer here instead of copying the data in the completion callback? https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:103: if (result != net::ERR_IO_PENDING) { If we get ERR_IO_PENDING, you'll leave the request hanging https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:116: underlying_error_ = result; Doesn't this leave request hanging? You probably want a FinishRequest method that you can call anywhere you encounter an error condition. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:123: Should this kick off another read if buffers_.size < kDesiredNumberOfBuffers ? https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h (right): https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:19: ReadaheadFileStreamReader(webkit_blob::FileStreamReader* underlying); explicit https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:19: ReadaheadFileStreamReader(webkit_blob::FileStreamReader* underlying); nit: underlying -> source ? (throughout) https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:66: scoped_ptr<Request> pending_read_; Since you only have a pointer to Request, can you just declare it (class Request;) or does the templatization require the full declaration?
underlying->source made interpatch diff looks horrific, sry https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:52: // We are waiting for an underlying read to complete, so save the request. On 2014/03/03 22:24:54, vandebo wrote: > DCHECK(!pending_read.get()); Done. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:64: : buf(buf), buf_len(buf_len), callback(callback) { On 2014/03/03 22:24:54, vandebo wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... > > Multiple lines - I know git cl format does it this way... Done. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:72: On 2014/03/03 22:24:54, vandebo wrote: > Probably want a while loop here - there might only be one byte left in the > current buffer with a request for 2MB, which should consume both buffers. Done. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:96: scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(kBufferSize)); On 2014/03/03 22:24:54, vandebo wrote: > Why not make a DrainableIOBuffer here instead of copying the data in the > completion callback? Added a comment. DrainableIOBuffer just wraps the buffer without a copy right? https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:103: if (result != net::ERR_IO_PENDING) { On 2014/03/03 22:24:54, vandebo wrote: > If we get ERR_IO_PENDING, you'll leave the request hanging As intended. If we get ERR_IO_PENDING, OnFinishReadFromSource doesn't happen until later. If there isn't enough buffer to complete the request, it gets saved in pending_read_. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:116: underlying_error_ = result; On 2014/03/03 22:24:54, vandebo wrote: > Doesn't this leave request hanging? You probably want a FinishRequest method > that you can call anywhere you encounter an error condition. Done. I didn't use a FinishRequest, as I couldn't find another place to use it. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:123: On 2014/03/03 22:24:54, vandebo wrote: > Should this kick off another read if buffers_.size < kDesiredNumberOfBuffers ? The ConsumeFromBuffer call within this if-block should kick off another read if necessary. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h (right): https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:19: ReadaheadFileStreamReader(webkit_blob::FileStreamReader* underlying); On 2014/03/03 22:24:54, vandebo wrote: > explicit Done. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:19: ReadaheadFileStreamReader(webkit_blob::FileStreamReader* underlying); On 2014/03/03 22:24:54, vandebo wrote: > nit: underlying -> source ? (throughout) Done. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:66: scoped_ptr<Request> pending_read_; On 2014/03/03 22:24:54, vandebo wrote: > Since you only have a pointer to Request, can you just declare it (class > Request;) or does the templatization require the full declaration? Done.
Take or leave my comments, not sure all of them apply together. https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:96: scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(kBufferSize)); On 2014/03/04 00:39:40, tommycli wrote: > On 2014/03/03 22:24:54, vandebo wrote: > > Why not make a DrainableIOBuffer here instead of copying the data in the > > completion callback? > > Added a comment. DrainableIOBuffer just wraps the buffer without a copy right? Right. I figured that out, but forgot to remove my comment. The comment you added isn't necessary. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:25: struct ReadaheadFileStreamReader::Request { You only have one of these... There's no need to allocate and free it, so you could make it a class member instead of a scoped_ptr. As a class member, the only reason to but it into a struct is to bundle the things up, but you don't really need that. If you just inline this class's members, you can use |buf| or callback.is_null() as an indicator of a pending request. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:36: const int buf_len; Instead of storing the IOBuffer and length and wrapping it in a DrainableIOBuffer each time you use it, you could just wrap it in a DrainableIOBuffer immediately and store that as the class member. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:55: if (source_.get() && !source_has_pending_read_ && push these conditionals down into ReadFromSource(IfNeeded) https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:60: // Consume from our existing buffers and return immediately if possible. Maybe push lines 60-66 into ConsumeFromBuffer and have it return ERR_IO_PENDING if there's nothing to do? https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:83: // |buf| continues to exist after |sink| goes out of scope. This comment doesn't seem necessary, since sink lives to the end of this method. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:88: net::DrainableIOBuffer* head_buffer = buffers_.front().get(); If you're calling the destination sink, might as well call this source. Or source_buffer if you're concerned about confusion with source_ https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:101: buffers_.pop(); This is unfortunate - we're going to free a meg of memory and then allocate it again. Not sure it's worth a lot of gymnastics to prevent it, but if there's something simple... https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:141: if (pending_read_.get()) { If you push lines 65 and 66 into ComsumeFromBuffer, you can get rid of this special case. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h (right): https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:33: int ConsumeFromBuffer(net::IOBuffer* buf, int buf_len); Out of the context of this comment, this name is confusing, it's easy to thing that you're consuming from |buf|... FillFromCache ? https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:36: // OnFinishReadFromUnderlying when it completes (either synchronously or nit: ...FromSource
https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/50001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:96: scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(kBufferSize)); On 2014/03/04 18:51:32, vandebo wrote: > On 2014/03/04 00:39:40, tommycli wrote: > > On 2014/03/03 22:24:54, vandebo wrote: > > > Why not make a DrainableIOBuffer here instead of copying the data in the > > > completion callback? > > > > Added a comment. DrainableIOBuffer just wraps the buffer without a copy right? > > Right. I figured that out, but forgot to remove my comment. The comment you > added isn't necessary. Done. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:25: struct ReadaheadFileStreamReader::Request { On 2014/03/04 18:51:32, vandebo wrote: > You only have one of these... > > There's no need to allocate and free it, so you could make it a class member > instead of a scoped_ptr. > > As a class member, the only reason to but it into a struct is to bundle the > things up, but you don't really need that. > > If you just inline this class's members, you can use |buf| or callback.is_null() > as an indicator of a pending request. Done. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:36: const int buf_len; On 2014/03/04 18:51:32, vandebo wrote: > Instead of storing the IOBuffer and length and wrapping it in a > DrainableIOBuffer each time you use it, you could just wrap it in a > DrainableIOBuffer immediately and store that as the class member. Done. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:55: if (source_.get() && !source_has_pending_read_ && On 2014/03/04 18:51:32, vandebo wrote: > push these conditionals down into ReadFromSource(IfNeeded) Done. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:60: // Consume from our existing buffers and return immediately if possible. On 2014/03/04 18:51:32, vandebo wrote: > Maybe push lines 60-66 into ConsumeFromBuffer and have it return ERR_IO_PENDING > if there's nothing to do? Done. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:83: // |buf| continues to exist after |sink| goes out of scope. On 2014/03/04 18:51:32, vandebo wrote: > This comment doesn't seem necessary, since sink lives to the end of this method. Done. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:88: net::DrainableIOBuffer* head_buffer = buffers_.front().get(); On 2014/03/04 18:51:32, vandebo wrote: > If you're calling the destination sink, might as well call this source. Or > source_buffer if you're concerned about confusion with source_ Done. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:101: buffers_.pop(); On 2014/03/04 18:51:32, vandebo wrote: > This is unfortunate - we're going to free a meg of memory and then allocate it > again. Not sure it's worth a lot of gymnastics to prevent it, but if there's > something simple... Last patchset adds something to reuse the just-exhausted buffer, but not sure if it's legit. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:141: if (pending_read_.get()) { On 2014/03/04 18:51:32, vandebo wrote: > If you push lines 65 and 66 into ComsumeFromBuffer, you can get rid of this > special case. Done. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h (right): https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:33: int ConsumeFromBuffer(net::IOBuffer* buf, int buf_len); On 2014/03/04 18:51:32, vandebo wrote: > Out of the context of this comment, this name is confusing, it's easy to thing > that you're consuming from |buf|... > > FillFromCache ? Done. https://codereview.chromium.org/178473022/diff/70001/chrome/browser/media_gal... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:36: // OnFinishReadFromUnderlying when it completes (either synchronously or On 2014/03/04 18:51:32, vandebo wrote: > nit: ...FromSource Done.
https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:150: DCHECK_NE(net::ERR_IO_PENDING, result); What happens if you exhaust the current buffer and then get an EOF on the replacement request? I think you just need to move 154 and 155 above this and then not DCHECK? https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h (right): https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:36: // asynchronously). |reuse| may an exhausted buffer for reuse, or NULL. may [be] an https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:51: std::queue<scoped_refptr<net::DrainableIOBuffer>> buffers_; space between >'s https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:58: bool source_has_pending_read_; nit: move up to line 46 https://codereview.chromium.org/178473022/diff/110001/net/base/io_buffer.h File net/base/io_buffer.h (right): https://codereview.chromium.org/178473022/diff/110001/net/base/io_buffer.h#ne... net/base/io_buffer.h:163: void SetSize(int size); I don't think we want to add this. If that means no reuse, so be it.
https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:150: DCHECK_NE(net::ERR_IO_PENDING, result); On 2014/03/04 21:55:12, vandebo wrote: > What happens if you exhaust the current buffer and then get an EOF on the > replacement request? I think you just need to move 154 and 155 above this and > then not DCHECK? Done. https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h (right): https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:36: // asynchronously). |reuse| may an exhausted buffer for reuse, or NULL. On 2014/03/04 21:55:12, vandebo wrote: > may [be] an Removed since we removed the buffer reuse. https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:51: std::queue<scoped_refptr<net::DrainableIOBuffer>> buffers_; On 2014/03/04 21:55:12, vandebo wrote: > space between >'s Done. https://codereview.chromium.org/178473022/diff/110001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:58: bool source_has_pending_read_; On 2014/03/04 21:55:12, vandebo wrote: > nit: move up to line 46 Done. https://codereview.chromium.org/178473022/diff/110001/net/base/io_buffer.h File net/base/io_buffer.h (right): https://codereview.chromium.org/178473022/diff/110001/net/base/io_buffer.h#ne... net/base/io_buffer.h:163: void SetSize(int size); On 2014/03/04 21:55:12, vandebo wrote: > I don't think we want to add this. If that means no reuse, so be it. Done.
https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:18: const int kDesiredNumberOfBuffers = 2; // So we are always one buffer ahead. nit: two spaces before a comment. https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:46: pending_sink_buffer_ = new net::DrainableIOBuffer(buf, buf_len); You don't actually use pending_sink_buffer_ as a DrainableIOBuffer. Two options: just store the IO buffer and len, change FinishRead... to take a DrainableIOBuffer. https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:113: if (result != net::ERR_IO_PENDING) { nit: predominate style in this directory is no {}'s on one line if's https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:128: buffers_.push(drainable_buffer); This should probably call ReadFromSourceIfNeeded(). I think it all works out fine right now because kDesiredNumberOfBuffers is only 2, but if it were set to 3, things wouldn't work right and it doesn't hurt anything to call it here. It's also easier to understand. https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h (right): https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:48: int64 current_offset_; Unused
https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc (right): https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:18: const int kDesiredNumberOfBuffers = 2; // So we are always one buffer ahead. On 2014/03/05 17:31:31, vandebo wrote: > nit: two spaces before a comment. Done. https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:46: pending_sink_buffer_ = new net::DrainableIOBuffer(buf, buf_len); On 2014/03/05 17:31:31, vandebo wrote: > You don't actually use pending_sink_buffer_ as a DrainableIOBuffer. Two > options: just store the IO buffer and len, change FinishRead... to take a > DrainableIOBuffer. Done. https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:113: if (result != net::ERR_IO_PENDING) { On 2014/03/05 17:31:31, vandebo wrote: > nit: predominate style in this directory is no {}'s on one line if's Done. https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.cc:128: buffers_.push(drainable_buffer); On 2014/03/05 17:31:31, vandebo wrote: > This should probably call ReadFromSourceIfNeeded(). I think it all works out > fine right now because kDesiredNumberOfBuffers is only 2, but if it were set to > 3, things wouldn't work right and it doesn't hurt anything to call it here. > It's also easier to understand. Done. https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... File chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h (right): https://codereview.chromium.org/178473022/diff/130001/chrome/browser/media_ga... chrome/browser/media_galleries/fileapi/readahead_file_stream_reader.h:48: int64 current_offset_; On 2014/03/05 17:31:31, vandebo wrote: > Unused Done.
fixed up drainable io buffer usage
LGTM
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/178473022/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/178473022/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel Retried try job too often on linux_rel for step(s) telemetry_perf_unittests, telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/178473022/260001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by tommycli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/178473022/260001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommycli@chromium.org/178473022/260001
Message was sent while issue was closed.
Change committed as 255210 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
