Eliminate copies of encoded image data

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

Index: third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp b/third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
index 3ea9aa0300bc6dd4d3d38fe71a80f148f303e972..427291bfb90acf00d438dd902590863a2705c1e7 100644
--- a/third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/FastSharedBufferReaderTest.cpp
@@ -29,6 +29,7 @@
  */
 
 #include "platform/image-decoders/FastSharedBufferReader.h"
+#include "platform/image-decoders/SharedBufferSegmentReader.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -53,7 +54,8 @@ TEST(FastSharedBufferReaderTest, nonSequentialReads)
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, sizeof(referenceData));
 
-    FastSharedBufferReader reader(data);
+    RefPtr<SharedBufferSegmentReader> segmentReader = adoptRef(new SharedBufferSegmentReader(data));
+    FastSharedBufferReader reader(segmentReader);

[Peter Kasting, 2016/03/23 02:42:58]

This pattern happening repeatedly makes me wonder if FastSharedBufferReader
should take |data| directly.  But maybe that would screw up other callers that
aren't creating a temporary SharedBufferSegmentReader just for this.

[f(malita), 2016/03/23 16:41:58]

nit: segmentReader.release()

(here and below)

[scroggo_chromium, 2016/03/24 13:59:45]

The typical caller (outside of tests) already has a SegmentReader. We could add
a second constructor that takes a SharedBuffer, though (similar to your proposal
for ImageDecoder). I'm not sure that's appropriate for a call that's only used
in tests, though.

[scroggo_chromium, 2016/03/24 13:59:45]

Done.

[Peter Kasting, 2016/03/24 22:05:43]

Up to you.  I'm not opposed to another constructor with a "// Used by tests."
sort of comment on it, especially if we minimize added code by having one
constructor delegate to the other (yay C++11).  Seems like a reasonably small
cost to simplify these calls, but we could live without it as well.

[scroggo_chromium, 2016/03/25 01:05:10]

Acknowledged. Left out the constructor for now.

 
     // Read size is prime such there will be a segment-spanning
     // read eventually.
@@ -72,7 +74,8 @@ TEST(FastSharedBufferReaderTest, readBackwards)
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, sizeof(referenceData));
 
-    FastSharedBufferReader reader(data);
+    RefPtr<SharedBufferSegmentReader> segmentReader = adoptRef(new SharedBufferSegmentReader(data));
+    FastSharedBufferReader reader(segmentReader);
 
     // Read size is prime such there will be a segment-spanning
     // read eventually.
@@ -91,7 +94,8 @@ TEST(FastSharedBufferReaderTest, byteByByte)
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, sizeof(referenceData));
 
-    FastSharedBufferReader reader(data);
+    RefPtr<SharedBufferSegmentReader> segmentReader = adoptRef(new SharedBufferSegmentReader(data));
+    FastSharedBufferReader reader(segmentReader);
     for (size_t i = 0; i < sizeof(referenceData); ++i) {
         ASSERT_EQ(referenceData[i], reader.getOneByte(i));
     }
@@ -107,8 +111,9 @@ TEST(FastSharedBufferReaderTest, readAllOverlappingLastSegmentBoundary)
     RefPtr<SharedBuffer> data = SharedBuffer::create();
     data->append(referenceData, dataSize);
 
+    RefPtr<SharedBufferSegmentReader> segmentReader = adoptRef(new SharedBufferSegmentReader(data));
     char buffer[dataSize];
-    FastSharedBufferReader reader(data);
+    FastSharedBufferReader reader(segmentReader);
     reader.getConsecutiveData(0, dataSize, buffer);
 
     ASSERT_FALSE(memcmp(buffer, referenceData, dataSize));