Create blobs from Disk Cache entries.

net/base/upload_disk_cache_entry_element_reader.h

+// Copyright 2015 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.
+
+#ifndef NET_BASE_UPLOAD_DISK_CACHE_ENTRY_ELEMENT_READER_H_
+#define NET_BASE_UPLOAD_DISK_CACHE_ENTRY_ELEMENT_READER_H_
+
+#include "base/basictypes.h"
+#include "base/compiler_specific.h"
+#include "base/memory/weak_ptr.h"
+#include "net/base/upload_element_reader.h"
+
+namespace disk_cache {
+class Entry;
+}
+
+namespace net {
+
+// An UploadElementReader implementation for disk_cache::Entry objects.
+// |entry| should outlive this class because this class does not take the
+// ownership of the data.

[mmenke, 2015/06/16 18:51:18]

Think the second sentence should be more assertive, and should be above the
constructor, as it's a constraint placed on the caller for the method.  Also,
there's nothing named |entry| here.

Just something like "The caller keeps ownership of |disk_cache_entry|, and is
responsible for it outlives the UploadDiskCacheEntryElementReader."

[gavinp, 2015/06/16 22:28:01]

Done. This text was copied directly from upload_bytes_element_reader.h, so I
updated the text there as well.

+class NET_EXPORT UploadDiskCacheEntryElementReader

[mmenke, 2015/06/16 18:51:18]

include net_export

[gavinp, 2015/06/16 22:28:01]

Done. Also in the file and bytes upload element headers.

+    : public UploadElementReader {
+ public:
+  UploadDiskCacheEntryElementReader(disk_cache::Entry* disk_cache_entry,
+                                    int disk_cache_stream_index,
+                                    int range_offset,
+                                    int range_length);

[mmenke, 2015/06/16 18:51:18]

These are fine as ints, rather than int64s?  Not sufficiently familiar with the
cache to know if there are any potential problems here.

[gavinp, 2015/06/16 22:28:01]

The disk_cache interface uses ints, and so I chose to expose that here rather
than perform type conversions.

Why it doesn't use int64, or uint64 as many of the file interfaces do is a
historic question. It would be good to change, because all cache implementations
end up having a terribly frustrating number of static casts between integer
types; carefully choosing our interface to match the underlying interfaces being
used would remove these increasing cache code readability and the readability of
users, as your comment shows.

+  ~UploadDiskCacheEntryElementReader() override;
+
+  int range_offset() const { return range_offset_; }
+  int range_length() const { return range_length_; }

[mmenke, 2015/06/16 18:51:19]

+_for_testing / _for_tests

[gavinp, 2015/06/16 22:28:01]

Done.

+
+  // UploadElementReader overrides:
+  const UploadDiskCacheEntryElementReader* AsDiskCacheEntryReader()

[mmenke, 2015/06/16 18:51:18]

These methods make me sad.  :(

The other UploadElementReader subclasses have them so extensions can poke at
them.  Should we let extensions poke at these uploads, too?  If not, this should
be rename to *ForTests or *ForTesting

[gavinp, 2015/06/16 22:28:01]

Done. I don't think we want to let extensions poke at them.

+      const override;
+  int Init(const CompletionCallback& callback) override;

[mmenke, 2015/06/16 18:51:18]

I generally feel like you should forward declare and include stuff
(CompletionCallback, uint64_t, IOBuffer), even if they're used in overridden
methods, though, admittedly, the other element reader classes don't do this.

For CompletionCallback, that's particularly the case, as you're depending on the
fact that it isn't forward declared in upload_element_reader.h, so it can be
invoked in the cc file without declaring completion_callback.h.

[gavinp, 2015/06/16 22:28:01]

I'm happy to do this; I was of the impression that for overrides this was
frowned upon practice.

Done.
I've added the include.

+  uint64_t GetContentLength() const override;
+  uint64_t BytesRemaining() const override;
+  bool IsInMemory() const override;
+  int Read(IOBuffer* buf,
+           int buf_length,
+           const CompletionCallback& callback) override;
+
+ private:
+  void OnReadCompleted(const CompletionCallback& callback, int result);
+
+  disk_cache::Entry* const disk_cache_entry_;
+  const int disk_cache_stream_index_;
+
+  const int range_offset_;
+  const int range_length_;
+
+  int offset_;

[mmenke, 2015/06/16 18:51:18]

Should document these.  It's not clear, in particular, what "offset_" and
"range_offset_" are relative to.

Or could come up with more self explanatory names.  I think something like
"range_start_, range_end_, read_position_", all being relative to the start of
the entry would be simpler, though they don't match the format of the input
arguments.

[michaeln, 2015/06/16 21:52:54]

+1 comments &| massaging the names, wasn't obvsious on a first readthru

[gavinp, 2015/06/16 22:28:01]

I've done this. I left the inline methods used in tests (with their new
_for_tests) names following the format of the input arguments. I left the input
arguments as they were, for consistency with the other readers.

+
+  base::WeakPtrFactory<UploadDiskCacheEntryElementReader> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(UploadDiskCacheEntryElementReader);
+};
+
+}  // namespace net
+
+#endif  // NET_BASE_UPLOAD_DISK_CACHE_ENTRY_ELEMENT_READER_H_