Do not consolidate data in BMPImageDecoder

Source/platform/image-decoders/FastSharedBufferReader.h

 /*
  * Copyright (C) 2015 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 28 matching lines @@
 namespace blink {
 
 // This class is used by image decoders to avoid memory consolidation and
 // therefore minimizes the cost of memory copying when the decoders
 // repeatedly read from a buffer that is continually growing due to network
 // traffic.
 class PLATFORM_EXPORT FastSharedBufferReader {
 public:
     FastSharedBufferReader(PassRefPtr<SharedBuffer> data);
 
+    void setData(SharedBuffer*);

[Peter Kasting, 2015/09/02 22:08:54]

Is it really correct that the constructor takes a PassRefPtr but this takes a
raw pointer?  I don't know Blink's pointer type semantics but that looks
strange.

[scroggo_chromium, 2015/09/02 23:01:07]

Me neither...
Agreed. ImageDecoder::setData also takes a raw pointer, and this method is
called by that one. We could wrap that pointer...

[Peter Kasting, 2015/09/02 23:48:44]

Definitely at least figure out precisely what the code is doing today and why. 
That way we'll know whether this difference is important.

[scroggo_chromium, 2015/09/03 22:04:19]

I just talked this over with a couple of people, and here is my understanding:

m_data is a RefPtr<SharedBuffer>. As is written in this patch set, setData takes
a raw pointer. When I do the assignment to m_data, that increases the ref count
so the FastSharedBufferReader has a reference.

If I used a PassRefPtr<SharedBuffer> instead, passing the raw pointer (from
BMPImageReader::setData) to FastSharedBufferReader::setData implicitly creates a
PassRefPtr, which increases the ref count. When that gets assigned to m_data,
that does *not* increase the ref count, but it does tell the PassRefPtr not to
decrement it (which it would do otherwise - this is important, because it means
if data == m_data, so we do not do the assignment, the PassRefPtr *does*
decrement it).

So, both do essentially the same thing. The PassRefPtr approach increments the
pointer earlier (and therefore adds some unnecessary churn, in the case where it
is the same as m_data). That said, I think consistency is better, and I
understand we generally do not want to use raw pointers. The latest patch set
makes this parameter a PassRefPtr.

+
     // Returns a consecutive buffer that carries the data starting
     // at |dataPosition| with |length| bytes.
     // This method returns a pointer to a memory segment stored in
     // |m_data| if such a consecutive buffer can be found.
     // Otherwise copies into |buffer| and returns it.
     // Caller must ensure there are enough bytes in |m_data| and |buffer|.
     const char* getConsecutiveData(size_t dataPosition, size_t length, char* buffer);
 
     // Wraps SharedBuffer::getSomeData().
     size_t getSomeData(const char*& someData, size_t dataPosition);
@@ skipping 20 matching lines @@
     const char* m_segment;
     size_t m_segmentLength;
 
     // Data position in |m_data| pointed to by |m_segment|.
     size_t m_dataPosition;
 };
 
 } // namespace blink
 
 #endif