Create blobs from Disk Cache entries.

net/base/upload_disk_cache_entry_element_reader.cc

+// 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.
+
+#include "net/base/upload_disk_cache_entry_element_reader.h"
+
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/disk_cache/disk_cache.h"
+
+namespace net {
+
+UploadDiskCacheEntryElementReader::UploadDiskCacheEntryElementReader(
+    disk_cache::Entry* disk_cache_entry,
+    int disk_cache_stream_index,
+    int range_offset,
+    int range_length)
+    : disk_cache_entry_(disk_cache_entry),
+      disk_cache_stream_index_(disk_cache_stream_index),
+      range_offset_(range_offset),
+      range_length_(range_length),
+      offset_(range_offset_),
+      weak_factory_(this) {
+  DCHECK_LE(range_offset_ + range_length,

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

Should we also DCHECK both of these are >= 0?

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

Done.

+            disk_cache_entry_->GetDataSize(disk_cache_stream_index_));

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

nit:  include base/logging.h

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

Done.

+}
+
+UploadDiskCacheEntryElementReader::~UploadDiskCacheEntryElementReader() {
+}
+
+const UploadDiskCacheEntryElementReader*
+UploadDiskCacheEntryElementReader::AsDiskCacheEntryReader() const {
+  return this;
+}
+
+int UploadDiskCacheEntryElementReader::Init(
+    const CompletionCallback& callback) {

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

If there's a pending read from entry, it's fine if we just start a new one,
right?  I assume so, but want to double check.

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

Yes.

The behaviour that we will not call the callback on the Read() if the reader has
Init() called while the Read is pending is copied from UploadFileElementReader,
which behaves this way without documentation.

+  weak_factory_.InvalidateWeakPtrs();
+  offset_ = range_offset_;
+  return OK;
+}
+
+uint64_t UploadDiskCacheEntryElementReader::GetContentLength() const {
+  return range_length_;
+}
+
+uint64_t UploadDiskCacheEntryElementReader::BytesRemaining() const {
+  const int amount_read = offset_ - range_offset_;

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

nit:  Basically no code in chrome or net consts stack variables (Except
compile-time constants).  Goes for this whole file.

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

i don't think that's true and i think its helpful delcare them as const, maybe
name them kSomething to make it even more clear that the value won't be changing
in the method body.

[mmenke, 2015/06/16 22:22:47]

Per style guide, kSomething is only for compile-time constants, which this is
not.

And also, while I can't speak for chrome in general, I do anywhere from 5 to 20
reviews most weeks, and no one does this.  It's not present in pre-existing
code, and no one adds it (And not due to me commenting on it).  It's just not
the prevailing style, which the style guide makes clear one should always
follow.

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

In this case, it's moot, because this line doesn't exist anymore. I think
mmenke's point is more correct with things like "const int bytes_to_read..." on
line 60 below?

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

Done.

[gavinp, 2015/06/16 23:08:04]

I have gone ahead and removed most const from automatic variables, except those
that were compile time constants which have kConstantNamesLikeThis. I am happy
to leave it like this.

However, I can't resist a heated debate about our coding standard. So, I'm
putting my asbestos underwear on and weighing in.

I think that as far as the style guide goes, the consistency rule is covered in
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Existing_Non...
. I think it's stretching that rule to apply it, as written, to something as big
as "chrome" or even something smaller like "net." I interpret "local" here to
mean something narrower than all of net; so I was comfortable with the variation
from consistency that this unit test had. I interpret mmenke's statement to mean
that he disagrees about the scope of the consistency argument.

There's also an argument against sprinkling const around stack variables to be
found in our rule about using const (note constexpr is banned in chromium,
awkwardly since we have the kConstTypeName rule which could be enforced with
constexpr, oh well; but on the other hand, it seems we could add a clang rule
for this...).

See
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Use_of_const ,
which says to use const "wherever it makes sense", less than helpful in a guide
we're referring to when we potentially disagree. Looking deeper, the only thing
listed among "Cons" for the use of const is that it is viral and can make it
hard to use library functions in particular. This is not a concern with stack
variables. Many of the benefits listed in "Pros" do apply; compiler type
checking, helps users reading the code by telling them about things that won't
happen to particular variables. But then, awkwardly, the "Decision" text appears
to describe what "makes sense" means, and lists three cases. Naturally, it fails
to discuss stack variables.

One other potential point for guidance is the discussion of naming const
variables,
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names
. The last sentence contains the restriction on kConstNames  that mmenke
referred to; but perhaps most hopefully for the argument in favour of const
stack variables is that it contemplates local scope specifically, and
specifically mentions that local variables that are const may have "usual
variable naming rules."

Read most strictly, that rule says that variables which are compile-time
constants may be named with the usual variable naming rules. However, since it
contemplates variables that are "const" with the usual variable naming rules,
I'd say this text goes a bit further and says that it is consistent with Google
coding rules to name "const" variables with the usual variable naming rules,
whether they are compile time const or not.

So as I read the standard, the consistency rule doesn't win here. However, if
you believe in a broader consistency rule than I do, then you might believe
otherwise.

[michaeln, 2015/06/17 00:57:36]

ok, if you say so :)

[mmenke, 2015/06/17 01:24:35]

Note that if net/ globally follows a convention, then any subset of net locally
does as well.  I could scope my comment to any of (upload_element_readers,
upload code, net/base, net/, chrome/), and it would still be the case that const
is not generally used for variables on the stack, even if they aren't modified.

My opinion is that peppering just one subset of the code with consts (Namely,
the subset that you have written) harms readability, due to adding more visual
noise.  If we used const uniformly for stack allocated variables that don't
change, that wouldn't be a problem, as people would be used to the extra noise. 
If you want to try and convince the chrome tear or the network team to start
using const everywhere, be my guest - it's not something I'd argue against.  One
person going on a crusade to const all stack variables in just the code he
happens to touch, however, I strongly oppose.

+  return range_length_ - amount_read;
+}
+
+bool UploadDiskCacheEntryElementReader::IsInMemory() const {
+  return false;
+}
+
+int UploadDiskCacheEntryElementReader::Read(
+    IOBuffer* buf,
+    int buf_length,
+    const CompletionCallback& callback) {

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

DCHECK(!callback.is_null())?

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

Done.

+  const int bytes_to_read =
+      std::min(buf_length, static_cast<int>(BytesRemaining()));

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

need to include the header for min (algorithm?)

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

Done. It's algorithm.

+
+  CompletionCallback new_callback =
+      base::Bind(&UploadDiskCacheEntryElementReader::OnReadCompleted,
+                 weak_factory_.GetWeakPtr(), callback);

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

include base/bind.h

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

Suggest making this callback a class member variable instead, for two reasons:

1)  It lets us DCHECK it's NULL in Read.
2)  It means if it points to anything, we'll be deleting it on the same thread
it's passed to us.

Know the other ElementReader subclasses don't work that way.  If you prefer it
as-is, not something I feel that strongly about.  You will need to clear the
callback in Init.

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

I'm leaving this as is, absent further reviewer input. (Other reviewers: chime
in if you agree with mmenke!)

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

Done.

+
+  const int result = disk_cache_entry_->ReadData(
+      disk_cache_stream_index_, offset_, buf, bytes_to_read, new_callback);
+  if (result == ERR_IO_PENDING)
+    return ERR_IO_PENDING;
+  if (result > 0)
+    offset_ += result;
+  return result;
+}
+
+void UploadDiskCacheEntryElementReader::OnReadCompleted(
+    const CompletionCallback& callback,
+    int result) {
+  if (result > 0)
+    offset_ += result;
+  callback.Run(result);
+}
+
+}  // namespace net