Hide implementation details: SkFrontBufferedStream

Hide implementation details: SkFrontBufferedStream

R=reed@google.com

Committed: https://code.google.com/p/skia/source/detail?r=12256

[scroggo, 2013-11-12 15:00:59]

On 2013/10/01 19:01:56, scroggo wrote:

Mike, any thoughts? This just hides the implementation details of
SkFrontBufferedStream in a cpp file, as per your earlier suggestion.

[reed1, 2013-11-12 15:58:45]

lgtm w/ class/namespace nit

https://codereview.chromium.org/25581002/diff/1/include/utils/SkFrontBuffered...
File include/utils/SkFrontBufferedStream.h (right):

https://codereview.chromium.org/25581002/diff/1/include/utils/SkFrontBuffered...
include/utils/SkFrontBufferedStream.h:21: namespace SkFrontBufferedStream {
class is what we usually use here for skia, though I know that namespace also
works.

If/when this functionality becomes "std", perhaps we just move this factory into
SkSteam.h, inside SkStream itself, which already has some factories (e.g.
NewFromFile).

[scroggo, 2013-11-12 17:28:57]

https://codereview.chromium.org/25581002/diff/1/include/utils/SkFrontBuffered...
File include/utils/SkFrontBufferedStream.h (right):

https://codereview.chromium.org/25581002/diff/1/include/utils/SkFrontBuffered...
include/utils/SkFrontBufferedStream.h:21: namespace SkFrontBufferedStream {
On 2013/11/12 15:58:45, reed1 wrote:
> class is what we usually use here for skia, though I know that namespace also
> works.
> 
> If/when this functionality becomes "std", perhaps we just move this factory
into
> SkSteam.h, inside SkStream itself, which already has some factories (e.g.
> NewFromFile).

Done.

[reed1, 2013-11-12 17:41:02]

https://codereview.chromium.org/25581002/diff/34001/include/utils/SkFrontBuff...
File include/utils/SkFrontBufferedStream.h (right):

https://codereview.chromium.org/25581002/diff/34001/include/utils/SkFrontBuff...
include/utils/SkFrontBufferedStream.h:25: *  @param stream SkStream to buffer.
If NULL, NULL is returned. After
Not sure the punctuation is correct in the 2nd/3rd sentences.

1. Does it really mean that if we fail to create a bufferstream, that we have
somehow 'ruined' the base stream?

2. Do we *promise" to buffer exactly bufferSize? Can we buffer more or less?

3. What if we fail to allocate the requested buffer? Does that mean the caller
should unref their stream?

[scroggo, 2013-11-12 19:46:54]

https://codereview.chromium.org/25581002/diff/34001/include/utils/SkFrontBuff...
File include/utils/SkFrontBufferedStream.h (right):

https://codereview.chromium.org/25581002/diff/34001/include/utils/SkFrontBuff...
include/utils/SkFrontBufferedStream.h:25: *  @param stream SkStream to buffer.
If NULL, NULL is returned. After
On 2013/11/12 17:41:02, reed1 wrote:
> Not sure the punctuation is correct in the 2nd/3rd sentences.

I don't see the punctuation error. "If NULL, NULL is returned" could be
clarified though. Updated.

> 
> 1. Does it really mean that if we fail to create a bufferstream, that we have
> somehow 'ruined' the base stream?

It should not (though at the moment, the only way for failure is if there is no
input stream, or if we throw). Updated the comments.

> 
> 2. Do we *promise" to buffer exactly bufferSize? Can we buffer more or less?

There's probably no reason to put an upper bound on what will be buffered, but
this should be the minimum. Updated the comments.

> 
> 3. What if we fail to allocate the requested buffer? Does that mean the caller
> should unref their stream?

We throw.

[reed1, 2013-11-12 20:41:37]

Future ideas:

1. take ownership, don't ref
2. return NULL if we can't alloc the buffer instead of throwing
3. consider a variant where the caller provides the storage (no need to check
for failure)

lgtm

[scroggo, 2013-11-12 20:53:11]

Committed patchset #5 manually as r12256 (presubmit successful).