Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(39)

Issue 23717055: Add a buffered SkStream class. (Closed)

Created:
7 years, 3 months ago by scroggo
Modified:
7 years, 2 months ago
CC:
skia-review_googlegroups.com
Visibility:
Public.

Description

Add a buffered SkStream class. This is used by Android to buffer an input stream which may not otherwise be able to rewind. Add a test for the new class. R=bungeman@google.com, mtklein@google.com, reed@google.com Committed: https://code.google.com/p/skia/source/detail?r=11488

Patch Set 1 #

Total comments: 16

Patch Set 2 : Respond to comments #

Total comments: 2

Patch Set 3 : Comments, check for NULL dst #

Total comments: 7

Patch Set 4 : SkBufferedStream -> SkFrontBufferedStream #

Total comments: 1

Patch Set 5 : Free buffer when done with it. #

Total comments: 2

Patch Set 6 : Rename test #

Total comments: 10

Patch Set 7 : Respond to comments. #

Patch Set 8 : Refactor read() #

Total comments: 4

Patch Set 9 : Respond to comments #

Patch Set 10 : Fix a bug #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+387 lines, -0 lines) Patch
M gyp/tests.gyp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M gyp/utils.gyp View 1 2 3 2 chunks +2 lines, -0 lines 0 comments Download
A include/utils/SkFrontBufferedStream.h View 1 2 3 4 5 6 7 8 1 chunk +77 lines, -0 lines 0 comments Download
A src/utils/SkFrontBufferedStream.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +147 lines, -0 lines 1 comment Download
A tests/FrontBufferedStreamTest.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +160 lines, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
scroggo
7 years, 3 months ago (2013-09-17 18:30:57 UTC) #1
mtklein
Test seems to be missing from the patchset? https://codereview.chromium.org/23717055/diff/1/include/utils/SkBufferedStream.h File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/1/include/utils/SkBufferedStream.h#newcode42 include/utils/SkBufferedStream.h:42: SkStream* ...
7 years, 3 months ago (2013-09-17 19:18:32 UTC) #2
scroggo
> Test seems to be missing from the patchset? Yes... Included in the next patch ...
7 years, 3 months ago (2013-09-17 20:12:51 UTC) #3
reed1
7 years, 3 months ago (2013-09-17 20:21:09 UTC) #4
reed1
https://codereview.chromium.org/23717055/diff/2001/include/utils/SkBufferedStream.h File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/2001/include/utils/SkBufferedStream.h#newcode11 include/utils/SkBufferedStream.h:11: class SkBufferedStream : public SkStreamRewindable { We might consider ...
7 years, 3 months ago (2013-09-17 20:23:02 UTC) #5
scroggo
Latest patch checks for NULL dst, which can happen in skip(), and adds tests for ...
7 years, 3 months ago (2013-09-17 20:41:19 UTC) #6
bungeman-skia
https://codereview.chromium.org/23717055/diff/13001/include/utils/SkBufferedStream.h File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/13001/include/utils/SkBufferedStream.h#newcode11 include/utils/SkBufferedStream.h:11: class SkBufferedStream : public SkStreamRewindable { So I'm a ...
7 years, 3 months ago (2013-09-17 22:19:11 UTC) #7
scroggo
New patch up for review. https://codereview.chromium.org/23717055/diff/13001/include/utils/SkBufferedStream.h File include/utils/SkBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/13001/include/utils/SkBufferedStream.h#newcode11 include/utils/SkBufferedStream.h:11: class SkBufferedStream : public ...
7 years, 3 months ago (2013-09-18 15:15:44 UTC) #8
bungeman-skia
lgtm with the test name changed https://codereview.chromium.org/23717055/diff/22001/gyp/tests.gyp File gyp/tests.gyp (right): https://codereview.chromium.org/23717055/diff/22001/gyp/tests.gyp#newcode36 gyp/tests.gyp:36: '../tests/BufferedStreamTest.cpp', FrontBufferedStreamTest?
7 years, 3 months ago (2013-09-18 15:35:02 UTC) #9
scroggo
https://codereview.chromium.org/23717055/diff/22001/gyp/tests.gyp File gyp/tests.gyp (right): https://codereview.chromium.org/23717055/diff/22001/gyp/tests.gyp#newcode36 gyp/tests.gyp:36: '../tests/BufferedStreamTest.cpp', On 2013/09/18 15:35:03, bungeman1 wrote: > FrontBufferedStreamTest? Done.
7 years, 3 months ago (2013-09-18 15:37:59 UTC) #10
reed1
https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBufferedStream.h File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBufferedStream.h#newcode29 include/utils/SkFrontBufferedStream.h:29: static SkFrontBufferedStream* Create(SkStream* stream, size_t bufferSize); Why does this ...
7 years, 3 months ago (2013-09-18 18:25:54 UTC) #11
mtklein
Little things and some ideas about how to break up read(). https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBufferedStream.h File include/utils/SkFrontBufferedStream.h (right): ...
7 years, 3 months ago (2013-09-19 18:56:52 UTC) #12
mtklein
Little things and some ideas about how to break up read(). https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBufferedStream.h File include/utils/SkFrontBufferedStream.h (right): ...
7 years, 3 months ago (2013-09-19 18:56:52 UTC) #13
scroggo
https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBufferedStream.h File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/32001/include/utils/SkFrontBufferedStream.h#newcode29 include/utils/SkFrontBufferedStream.h:29: static SkFrontBufferedStream* Create(SkStream* stream, size_t bufferSize); On 2013/09/18 18:25:54, ...
7 years, 2 months ago (2013-09-25 17:36:01 UTC) #14
mtklein
Thanks Leon. This is exactly what I was thinking. Very much LGTM. https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBufferedStream.h File include/utils/SkFrontBufferedStream.h ...
7 years, 2 months ago (2013-09-26 15:36:21 UTC) #15
scroggo
https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBufferedStream.h File include/utils/SkFrontBufferedStream.h (right): https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBufferedStream.h#newcode59 include/utils/SkFrontBufferedStream.h:59: SkFrontBufferedStream(); On 2013/09/26 15:36:21, mtklein wrote: > Seems unneeded ...
7 years, 2 months ago (2013-09-26 17:26:03 UTC) #16
scroggo
On 2013/09/26 17:26:03, scroggo wrote: > https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBufferedStream.h > File include/utils/SkFrontBufferedStream.h (right): > > https://codereview.chromium.org/23717055/diff/48001/include/utils/SkFrontBufferedStream.h#newcode59 > ...
7 years, 2 months ago (2013-09-26 21:15:29 UTC) #17
reed1
lgtm -- pending consensus by ben and mike
7 years, 2 months ago (2013-09-26 21:22:13 UTC) #18
scroggo
7 years, 2 months ago (2013-09-26 21:35:46 UTC) #19
Message was sent while issue was closed.
Committed patchset #10 manually as r11488 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698