Move gif image decoder to SkCodec

third_party/WebKit/Source/platform/image-decoders/SegmentStreamTest.cpp

+// Copyright (c) 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "platform/image-decoders/SegmentStream.h"
+
+#include <memory>
+#include "platform/SharedBuffer.h"
+#include "platform/wtf/PtrUtil.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+// SegmentStream has 4 accessors which do not alter state:
+// - isCleared()
+// - isAtEnd()
+// - getPosition()
+// - getLength()
+//
+// For every operation which changes state we can test:
+// - the operation completed as expected,
+// - the accessors did not change, and/or
+// - the accessors changed in the way we expected.
+//
+// There are actually 2 more accessors:
+// - hasPosition()
+// - hasLength()
+// but these should always return true to indicate that we can call getLength()
+// for example. So let's not add them to every state changing operation and add
+// needless complexity.
+
+namespace {
+
+constexpr size_t kBufferAllocationSize = 20;
+constexpr size_t kInsideBufferPosition = 10;
+constexpr size_t kPastEndOfBufferPosition = 30;
+
+testing::AssertionResult IsCleared(const blink::SegmentStream&);
+testing::AssertionResult IsAtEnd(const blink::SegmentStream&);
+testing::AssertionResult PositionIsZero(const blink::SegmentStream&);
+testing::AssertionResult PositionIsInsideBuffer(const blink::SegmentStream&);
+testing::AssertionResult PositionIsAtEndOfBuffer(const blink::SegmentStream&);
+testing::AssertionResult LengthIsZero(const blink::SegmentStream&);
+testing::AssertionResult LengthIsAllocationSize(const blink::SegmentStream&);
+
+// We want to keep the buffer alive for the same duration as the SegmentStream

[scroggo_chromium, 2017/04/25 15:31:41]

I'm not sure this comment is necessary. I guess it's the motivation for this
struct? Maybe a clearer comment is

"This struct ties the lifetimes of its buffer and segment_stream"?

But I don't think you need to, anyway. You use the buffer to create a
SharedBuffer (which copies it), and then you never use it again. So I don't
think you need this struct at all.

[cblume, 2017/04/26 10:09:43]

You are right. I didn't realize SharedBuffer copied the buffer contents. I wish
it didn't. I would prefer if it was like a rope and the owner of the original
buffers had to keep their lifetime in check. Oh well.

Anyway, now that I realize it copies the buffer contents you are right that we
don't need this type. This will simplify things a bunch. I'll go through and
clean it up.

+struct ReadyToReadSegmentStream {
+  ReadyToReadSegmentStream() = default;

[scroggo_chromium, 2017/04/25 15:31:41]

I think this would be clearer if you keep the defaults together and the deletes
together.

+  ReadyToReadSegmentStream(const ReadyToReadSegmentStream&) = delete;
+  ReadyToReadSegmentStream(ReadyToReadSegmentStream&&) = default;
+  ReadyToReadSegmentStream& operator=(const ReadyToReadSegmentStream&) = delete;
+  ReadyToReadSegmentStream& operator=(ReadyToReadSegmentStream&&) = default;
+
+  std::unique_ptr<char[]> buffer;
+  blink::SegmentStream segment_stream;
+};
+
+void CreateReadableSegmentStream(ReadyToReadSegmentStream& readable_stream);

[scroggo_chromium, 2017/04/25 15:31:40]

Many of the above methods are pretty clear about what they do, but this one is a
mystery to me. Maybe do one or more of the following?

- put the definition here
- add a comment
- better name
  - It is called "Create", but it doesn't create anything (it modifies a passed
in object)
  - does it put a particular amount into the buffer?

[cblume, 2017/04/26 10:09:46]

Done.

+
+}  // namespace
+
+namespace blink {
+
+TEST(SegmentStreamTest, DefaultConstructorShouldSetIsCleared) {
+  SegmentStream segment_stream;
+
+  ASSERT_TRUE(IsCleared(segment_stream));
+}
+
+TEST(SegmentStreamTest, DefaultConstructorShouldSetIsAtEnd) {
+  SegmentStream segment_stream;
+
+  ASSERT_TRUE(IsAtEnd(segment_stream));
+}
+
+TEST(SegmentStreamTest, DefaultContructorShouldClearPosition) {
+  SegmentStream segment_stream;
+
+  ASSERT_TRUE(PositionIsZero(segment_stream));
+}
+
+TEST(SegmentStreamTest, DefaultConstructorShouldHaveZeroLength) {
+  SegmentStream segment_stream;
+
+  ASSERT_TRUE(LengthIsZero(segment_stream));
+}
+
+TEST(SegmentStreamTest, SetReaderShouldUnsetIsCleared) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  ASSERT_FALSE(IsCleared(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, SetReaderShouldUnsetIsAtEnd) {

[scroggo_chromium, 2017/04/25 15:31:41]

"SetReaderShouldUnsetIsAtEnd" seems misleading - it only unsets IsAtEnd because
the position was 0 and the reader is non-null with a size > 0 (I assume).

[cblume, 2017/04/26 10:09:49]

I think the position being 0 doesn't matter. But you have a really good point
about the size > 0.

The Google style is to name test functions like:
<function being tested>Should<expected behavior>
When there is a special case for that function, the name is followed by:
When<special condition>
https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b...

Below here, you will see:
SetReaderShouldSetIsAtEndWhenSetToNull
this function ^^ is the special case of the function we are commenting on here.

The test we are commenting on here should handle the scenario where there is a
typical reader being set.
A position of 0 should be fine. If we do position 10 & reader size 20 we'll want
call to SetReader after the position is set. But they should both be testing the
same case.

Reader size > 0 is interesting though. If we set the position to 20 and then
call SetReader for the first time with a buffer of size 5 then we are still at
the end.
I'll add a special case test for this.

+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, SetReaderShouldNotChangePosition) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, SetReaderShouldUpdateLength) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  ASSERT_TRUE(LengthIsAllocationSize(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, SetReaderShouldSetIsClearedWhenSetToNull) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_FALSE(IsCleared(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.SetReader(nullptr);
+  ASSERT_TRUE(IsCleared(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, SetReaderShouldSetIsAtEndWhenSetToNull) {

[scroggo_chromium, 2017/04/25 15:31:40]

Some of these tests seem like they could be combined. e.g.
SetReaderShouldUnsetIsAtEnd is the same as the first ASSERT here. Do we really
need both?

I would imagine the argument for having both is that each test should test only
one thing? And this test only duplicates the first one because it sets up (and
verifies) the state for the actual thing it wants to test?

As a counter-argument, separating them means
- duplicated code
- more code to look through
  - harder (at least IMO) to verify that all things were tested

I don't necessarily think all the SegmentStreamTests should be combined into
one, but maybe this would be better with one Test that covers isAtEnd?

TEST(SegmentStreamTest, IsAtEnd) {
  ReadyToReadSegmentStream readable_stream;
  ASSERT_TRUE(IsAtEnd(readable_stream.segment_stream)); //
DefaultConstructorShouldSetIsAtEnd

  CreateReadableSegmentStream(readable_stream);
  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream)); //
SetReaderShouldUnsetIsAtEnd

  readable_stream.segment_stream.SetReader(nullptr);
  ASSERT_TRUE(IsAtEnd(readable_stream.segment_stream)); // This one

  ... // ReadShouldUpdateIsAtEnd, etc
}

Then we can find all the IsAtEnd tests and quickly see if we've missed
something.

(Or have I missed some testing guideline?)

[cblume, 2017/04/26 10:09:44]

I completely agree.
Ideally, only one test fails when one thing goes wrong. That way the test can
point directly to the problem. So you are right about why I did this.

What I *wish* we had is a way for one test to depend on another test. For
example, if test A failed don't even run test B because test B needs to be able
to assume A's behavior works. Unfortunately, that isn't what we have. So my
idealist separation can't provide that benefit.

I actually started writing changes to gtest but couldn't find an email group to
see if they would accept it or if I had missed something. What I wanted to do
was create a sort of ASSUME_TRUE(OtherTest(segment_stream)); which would leave
the segment_stream in the state I want.

I think there might be a happy medium here. I could probably create
OtherTestHelper() so we reduce code duplication & still have any given test
failure point to the correct reason for failure. There would be multiple test
failures when 1 bad change happens. But at least they would all point to the
same reason.

Additionally, an alternative that follows your idea is to have 1 SetReader with
multiple tests IsAtEnd, IsCleared, ...
That would probably fit better since the setup would be the same for all of
those. And since the idealistic one-test-per-failure-condition can't happen,
might as well make it easy to read.

I'll start with this last idea (similar to yours) and pull out setup where I
can.

[cblume, 2017/04/26 18:17:52]

I began working on this and realized it is not what we want.

Having 1 test that checks multiple behaviors (since the setup is the same)
violates a few things.
For one, it makes it hard to follow the naming guidelines. I could name the test
SetReader...HasCorrectBehavior??
But more clearly, from
https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b...
adding new behavior should mean not needing to change any existing tests. It
specifically calls out this example where one method tests multiple behaviors.
Check the second paragraph.

So I think we want to keep this long, exhaustive list.

But I think I can still refactor it a bit to reuse the common setup. That should
make things easier. And I might be able to do it in a way that clearly states
"This test failed when trying to do the setup. The real failure is over there."
I'll work on this now.

[cblume, 2017/04/27 03:07:41]

I have cleaned this up as much as I think makes sense.

The tests are in the form:
<setup>
__blank_line__
<thing_being_tested>
__blank_line__
<assert expected result>

The bulk majority of the tests only need 1 line for each of those sections now.
The ones which don't are doing something unique.

+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.SetReader(nullptr);
+  ASSERT_TRUE(IsAtEnd(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, SetReaderShouldNotChangePositionWhenSetToNull) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  const size_t amount_read = readable_stream.segment_stream.read(
+      read_buffer.get(), kInsideBufferPosition);
+  ASSERT_EQ(amount_read, kInsideBufferPosition);

[scroggo_chromium, 2017/04/25 15:31:41]

I think the expected value goes first (isn't that why you changed the order of
https://codereview.chromium.org/2565323003/diff/1620001/third_party/WebKit/So...
?)

[cblume, 2017/04/26 10:09:49]

Oops, yeah. I caused this test to fail to see the results:
Value of: kInsideBufferPosition
  Actual: 10
Expected: amount_read - 1
Which is: 9

I'll go make sure the expected value comes first on all these tests.

+  const size_t pre_nulled_position =
+      readable_stream.segment_stream.getPosition();
+  ASSERT_EQ(amount_read, pre_nulled_position);
+
+  readable_stream.segment_stream.SetReader(nullptr);
+  ASSERT_EQ(readable_stream.segment_stream.getPosition(), pre_nulled_position);
+}
+
+TEST(SegmentStreamTest, SetReaderShouldClearLengthWhenSetToNull) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  readable_stream.segment_stream.SetReader(nullptr);
+  ASSERT_TRUE(LengthIsZero(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, ReadShouldConsumeBuffer) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);

[scroggo_chromium, 2017/04/25 15:31:40]

Why not put this on the stack?

  char read_buffer[kInsideBufferPosition];

Same for others.

[cblume, 2017/04/26 10:09:47]

Done.

+  const size_t amount_read = readable_stream.segment_stream.read(
+      read_buffer.get(), kInsideBufferPosition);
+  ASSERT_EQ(amount_read, kInsideBufferPosition);
+}
+
+TEST(SegmentStreamTest, ReadShouldNotClear) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  readable_stream.segment_stream.read(read_buffer.get(), kInsideBufferPosition);
+  ASSERT_FALSE(IsCleared(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, ReadShouldUpdateIsAtEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kBufferAllocationSize);
+  const size_t amount_read = readable_stream.segment_stream.read(
+      read_buffer.get(), kBufferAllocationSize);
+  ASSERT_EQ(amount_read, kBufferAllocationSize);
+  ASSERT_TRUE(IsAtEnd(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, ReadShouldUpdatePosition) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  const size_t amount_read = readable_stream.segment_stream.read(
+      read_buffer.get(), kInsideBufferPosition);
+  ASSERT_EQ(readable_stream.segment_stream.getPosition(), amount_read);
+  ASSERT_TRUE(PositionIsInsideBuffer(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, ReadShouldNotChangeLength) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const size_t length = readable_stream.segment_stream.getLength();
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  readable_stream.segment_stream.read(read_buffer.get(), kInsideBufferPosition);
+  ASSERT_EQ(readable_stream.segment_stream.getLength(), length);
+}
+
+TEST(SegmentStreamTest, ReadShouldNotConsumeBufferWhenCleared) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  const auto read_buffer_as_void_ptr =
+      reinterpret_cast<void*>(read_buffer.get());
+
+  readable_stream.segment_stream.SetReader(nullptr);
+  ASSERT_TRUE(IsCleared(readable_stream.segment_stream));
+
+  ASSERT_EQ(0u, readable_stream.segment_stream.read(read_buffer_as_void_ptr,
+                                                    kInsideBufferPosition));
+}
+
+TEST(SegmentStreamTest, ReadShouldConsumeBufferWithoutGoingPastTheEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kPastEndOfBufferPosition);
+  const size_t amount_read = readable_stream.segment_stream.read(
+      read_buffer.get(), kPastEndOfBufferPosition);
+  ASSERT_EQ(amount_read, readable_stream.segment_stream.getLength());
+}
+
+TEST(SegmentStreamTest, ReadShouldSetIsAtEndWhenPastEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kPastEndOfBufferPosition);
+  readable_stream.segment_stream.read(read_buffer.get(),
+                                      kPastEndOfBufferPosition);
+  ASSERT_TRUE(IsAtEnd(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, ReadShouldTruncatePositionWhenPastEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kPastEndOfBufferPosition);
+  const size_t amount_read = readable_stream.segment_stream.read(
+      read_buffer.get(), kPastEndOfBufferPosition);
+  ASSERT_EQ(amount_read, readable_stream.segment_stream.getPosition());
+  ASSERT_TRUE(PositionIsAtEndOfBuffer(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, PeekShouldCconsumeBuffer) {

[scroggo_chromium, 2017/04/25 15:31:40]

Consume* (not Cconsume)

[cblume, 2017/04/26 10:09:49]

Thank you

+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto peek_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  const size_t amount_peeked = readable_stream.segment_stream.peek(
+      peek_buffer.get(), kInsideBufferPosition);
+  ASSERT_EQ(amount_peeked, kInsideBufferPosition);
+}
+
+TEST(SegmentStreamTest, PeekShouldNotClear) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto peek_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  readable_stream.segment_stream.peek(peek_buffer.get(), kInsideBufferPosition);
+  ASSERT_FALSE(IsCleared(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, PeekShouldNotUpdateIsAtEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+
+  const auto peek_buffer = WTF::MakeUnique<char[]>(kBufferAllocationSize);
+  const size_t amount_peeked = readable_stream.segment_stream.peek(
+      peek_buffer.get(), kBufferAllocationSize);
+  ASSERT_EQ(amount_peeked, kBufferAllocationSize);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, PeekShouldNotUpdatePosition) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+
+  const auto peek_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  readable_stream.segment_stream.peek(peek_buffer.get(), kInsideBufferPosition);
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, PeekShouldNotChangeLength) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const size_t length = readable_stream.segment_stream.getLength();
+  const auto peek_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  readable_stream.segment_stream.peek(peek_buffer.get(), kInsideBufferPosition);
+  ASSERT_EQ(readable_stream.segment_stream.getLength(), length);
+}
+
+TEST(SegmentStreamTest, PeekShouldNotConsumeBufferWhenCleared) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto peek_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  const auto peek_buffer_as_void_ptr =
+      reinterpret_cast<void*>(peek_buffer.get());
+
+  readable_stream.segment_stream.SetReader(nullptr);
+  ASSERT_TRUE(IsCleared(readable_stream.segment_stream));
+
+  ASSERT_EQ(0u, readable_stream.segment_stream.peek(peek_buffer_as_void_ptr,
+                                                    kInsideBufferPosition));
+}
+
+TEST(SegmentStreamTest, PeekShouldConsumeBufferWithoutGoingPastTheEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto peek_buffer = WTF::MakeUnique<char[]>(kPastEndOfBufferPosition);
+  const size_t amount_peeked = readable_stream.segment_stream.peek(
+      peek_buffer.get(), kPastEndOfBufferPosition);
+  ASSERT_EQ(amount_peeked, readable_stream.segment_stream.getLength());
+}
+
+TEST(SegmentStreamTest, PeekShouldNotSetIsAtEndWhenPastEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+
+  const auto peek_buffer = WTF::MakeUnique<char[]>(kPastEndOfBufferPosition);
+  readable_stream.segment_stream.peek(peek_buffer.get(),
+                                      kPastEndOfBufferPosition);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, PeekShouldNotTruncatePositionWhenPastEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+
+  const auto peek_buffer = WTF::MakeUnique<char[]>(kPastEndOfBufferPosition);
+  readable_stream.segment_stream.peek(peek_buffer.get(),
+                                      kPastEndOfBufferPosition);
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, RewindShouldNotClear) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  readable_stream.segment_stream.read(read_buffer.get(), kInsideBufferPosition);
+  ASSERT_FALSE(IsCleared(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.rewind();
+  ASSERT_FALSE(IsCleared(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, RewindShouldNotSetAtEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  readable_stream.segment_stream.read(read_buffer.get(), kInsideBufferPosition);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.rewind();
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, RewindShouldResetPosition) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  readable_stream.segment_stream.read(read_buffer.get(), kInsideBufferPosition);
+  ASSERT_TRUE(PositionIsInsideBuffer(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.rewind();
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, RewindShouldNotChangeLength) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  const auto read_buffer = WTF::MakeUnique<char[]>(kInsideBufferPosition);
+  readable_stream.segment_stream.read(read_buffer.get(), kInsideBufferPosition);
+  const size_t pre_rewind_length = readable_stream.segment_stream.getLength();
+
+  readable_stream.segment_stream.rewind();
+  ASSERT_EQ(readable_stream.segment_stream.getLength(), pre_rewind_length);
+}
+
+TEST(SegmentStreamTest, HasPositionShouldBeSupported) {
+  blink::SegmentStream segment_stream;
+  ASSERT_TRUE(segment_stream.hasPosition());
+}
+
+TEST(SegmentStreamTest, SeekShouldNotSetIsCleared) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_FALSE(IsCleared(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.seek(kInsideBufferPosition);
+  ASSERT_FALSE(IsCleared(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, SeekShouldNotSetIsAtEndWhenSeekingInsideTheBuffer) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.seek(kInsideBufferPosition);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, SeekShouldSetIsAtEndWhenSeekingToTheEndOfTheBuffer) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_FALSE(IsAtEnd(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.seek(kBufferAllocationSize);
+  ASSERT_TRUE(IsAtEnd(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, SeekShouldUpdatePosition) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.seek(kInsideBufferPosition);
+  ASSERT_EQ(readable_stream.segment_stream.getPosition(),
+            kInsideBufferPosition);
+}
+
+TEST(SegmentStreamTest, SeekShouldNotTruncatePositionWhenPastEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.seek(kPastEndOfBufferPosition);
+  ASSERT_EQ(readable_stream.segment_stream.getPosition(),
+            kPastEndOfBufferPosition);
+}
+
+TEST(SegmentStreamTest, SeekShouldNotUpdateLength) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  readable_stream.segment_stream.seek(kInsideBufferPosition);
+  ASSERT_EQ(readable_stream.segment_stream.getLength(), kBufferAllocationSize);
+}
+
+TEST(SegmentStreamTest, MoveShouldNotSetCleared) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  readable_stream.segment_stream.move(kInsideBufferPosition);
+  ASSERT_FALSE(IsCleared(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, MoveShouldUpdatePosition) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  readable_stream.segment_stream.move(kInsideBufferPosition);
+  ASSERT_EQ(readable_stream.segment_stream.getPosition(),
+            kInsideBufferPosition);
+}
+
+TEST(SegmentStreamTest, MoveShouldNotTruncatePositionWhenPastEnd) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  readable_stream.segment_stream.move(kPastEndOfBufferPosition);
+  ASSERT_EQ(readable_stream.segment_stream.getPosition(),
+            kPastEndOfBufferPosition);
+}
+
+TEST(SegmentStreamTest, MoveShouldClampPositionToZeroWhenGoingNegative) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+
+  // Move the position off of 0
+  const auto read_buffer = WTF::MakeUnique<char[]>(size_t{1});
+  readable_stream.segment_stream.read(read_buffer.get(), 1);
+  ASSERT_FALSE(PositionIsZero(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.move(-2);
+  ASSERT_TRUE(PositionIsZero(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, MoveShouldNotChangeLength) {
+  ReadyToReadSegmentStream readable_stream;
+  CreateReadableSegmentStream(readable_stream);
+  ASSERT_TRUE(LengthIsAllocationSize(readable_stream.segment_stream));
+
+  readable_stream.segment_stream.move(kInsideBufferPosition);
+  ASSERT_TRUE(LengthIsAllocationSize(readable_stream.segment_stream));
+}
+
+TEST(SegmentStreamTest, HasLengthShouldBeSupported) {
+  blink::SegmentStream segment_stream;
+  ASSERT_TRUE(segment_stream.hasLength());
+}
+
+}  // namespace blink
+
+namespace {
+
+testing::AssertionResult IsCleared(const blink::SegmentStream& segment_stream) {
+  if (segment_stream.IsCleared())
+    return testing::AssertionSuccess();
+
+  return testing::AssertionFailure() << "SegmentStream is not clear";
+}
+
+testing::AssertionResult IsAtEnd(const blink::SegmentStream& segment_stream) {
+  if (segment_stream.isAtEnd())
+    return testing::AssertionSuccess();
+
+  return testing::AssertionFailure() << "SegmentStream is not at the end";
+}
+
+testing::AssertionResult PositionIsZero(
+    const blink::SegmentStream& segment_stream) {
+  if (segment_stream.getPosition() == 0ul)
+    return testing::AssertionSuccess();
+
+  return testing::AssertionFailure() << "SegmentStream position is not 0";
+}
+
+testing::AssertionResult PositionIsInsideBuffer(
+    const blink::SegmentStream& segment_stream) {
+  if (segment_stream.getPosition() == kInsideBufferPosition)
+    return testing::AssertionSuccess();
+
+  return testing::AssertionFailure()
+         << "SegmentStream position is not inside the buffer";
+}
+
+testing::AssertionResult PositionIsAtEndOfBuffer(
+    const blink::SegmentStream& segment_stream) {
+  if (segment_stream.getPosition() == kBufferAllocationSize)
+    return testing::AssertionSuccess();
+
+  return testing::AssertionFailure()
+         << "SegmentStream position is not at the end of the buffer";
+}
+
+testing::AssertionResult LengthIsZero(
+    const blink::SegmentStream& segment_stream) {
+  if (segment_stream.getLength() == 0ul)
+    return testing::AssertionSuccess();
+
+  return testing::AssertionFailure() << "SegmentStream length is not 0";
+}
+
+testing::AssertionResult LengthIsAllocationSize(
+    const blink::SegmentStream& segment_stream) {
+  if (segment_stream.getLength() == kBufferAllocationSize)
+    return testing::AssertionSuccess();
+
+  return testing::AssertionFailure()
+         << "SegmentStream length is not the allocation size";
+}
+
+// We want to keep the buffer alive for the same duration as the SegmentStream
+
+void CreateReadableSegmentStream(ReadyToReadSegmentStream& readable_stream) {
+  readable_stream.buffer = WTF::MakeUnique<char[]>(kBufferAllocationSize);

[scroggo_chromium, 2017/04/25 15:31:40]

As I mention above, you never use this buffer again. So you can just put it on
the stack. i.e.

struct ReadyToReadSegmentStream {
  ...
  char buffer[kBufferAllocationSize];
  ...
};

[cblume, 2017/04/26 10:09:49]

Done.

+
+  auto shared_buffer = blink::SharedBuffer::Create(readable_stream.buffer.get(),

[scroggo_chromium, 2017/04/25 15:31:41]

I've been advised not to use auto here, since this method returns a PassRefPtr
[1]. (Or is this okay because you're always going to move it immediately?)

[1]
https://codereview.chromium.org/2754003008/diff/1/third_party/WebKit/Source/p...

[cblume, 2017/04/26 10:09:49]

Done.

+                                                   kBufferAllocationSize);
+  auto segment_reader =

[scroggo_chromium, 2017/04/25 15:31:40]

Same concern about auto here.

[cblume, 2017/04/26 10:09:48]

Done.

+      blink::SegmentReader::CreateFromSharedBuffer(std::move(shared_buffer));
+  readable_stream.segment_stream.SetReader(segment_reader.Get());

[scroggo_chromium, 2017/04/25 15:31:40]

This makes me wonder if SetReader should take a PassRefPtr. I'm guessing it
takes a raw pointer because onSetData does?

[cblume, 2017/04/26 10:09:48]

Correct. I used a raw pointer because that is what I was given.
We aren't actually given ownership, either. So we can't just make a RefPtr out
of it. We are only given a view into someone else's RefPtr.

However, that is probably something we could change in a separate CL.

The base class ImageDecoder::SetData() takes a PassRefPtr, stores it locally,
and then calls OnSetData() with a raw pointer view into the pointer it owns. So
from within the derived image decoders, we cannot create a ref count -- it will
be deleted twice with two separate ref counts.

I suspect it could continue to pass ownership on to OnSetData() though.

But what *could* happen is create a protected RefPtr<SegmentReader>
ImageDecoder::GetData() { return data_; }

[scroggo_chromium, 2017/04/26 19:02:22]

I'm not sure I follow. Don't we already make a RefPtr out of the raw pointer
inside the SegmentStream? This is safe because this is an invasive ref-counting
system (notice SegmentReader inherits from ThreadSafeRefCounted, whose base
class includes the ref count). Creating a new RefPtr does not create a separate
ref count.

[cblume, 2017/04/26 19:16:06]

OHHHHH it is invasive!
Okay yeah, in that case I agree -- it makes sense for SetReader() to take a
PassRefPtr. I'll do that.

+}
+
+}  // namespace