Add experiment to measure time to hash extension content as we read it

net/url_request/url_request_file_job_unittest.cc

+// Copyright (c) 2014 The Chromium Authors. All rights reserved.

[rvargas (doing something else), 2014/04/08 16:53:45]

I think we don't use (c) anynmore

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/url_request/url_request_file_job.h"
+
+#include "base/files/scoped_temp_dir.h"
+#include "base/run_loop.h"
+#include "base/strings/stringprintf.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "net/base/net_util.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+namespace {
+
+// A URLRequestFileJob for testing OnSeekComplete / OnReadComplete callbacks.
+class URLRequestFileJobWithCallbacks : public URLRequestFileJob {
+ public:
+  URLRequestFileJobWithCallbacks(
+      URLRequest* request,
+      NetworkDelegate* network_delegate,
+      const base::FilePath& file_path,
+      const scoped_refptr<base::TaskRunner>& file_task_runner)
+      : URLRequestFileJob(request,
+                          network_delegate,
+                          file_path,
+                          file_task_runner),
+        seek_position_(0) {}

[rvargas (doing something else), 2014/04/08 16:53:45]

nit: {
}
(this is not a single line anymore)

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Done.

+
+  int64 seek_position() { return seek_position_; }
+  const std::vector<std::string>& data_chunks() { return data_chunks_; }
+
+ protected:
+  virtual ~URLRequestFileJobWithCallbacks() {}
+
+  virtual void OnSeekComplete(int64 result) OVERRIDE {
+    ASSERT_EQ(seek_position_, 0);
+    seek_position_ = result;
+  }
+
+  virtual void OnReadComplete(IOBuffer* buf, int result) OVERRIDE {
+    data_chunks_.push_back(std::string(buf->data(), result));
+  }
+
+  int64 seek_position_;
+  std::vector<std::string> data_chunks_;
+};
+
+// A URLRequestJobFactory that will return URLRequestFileJobWithCallbacks
+// instances for file:// scheme URLs.
+class CallbacksJobFactory : public URLRequestJobFactory {
+ public:
+  class JobObserver {
+   public:
+    virtual void OnJobCreated(URLRequestFileJobWithCallbacks* job) = 0;
+  };
+
+  CallbacksJobFactory(const base::FilePath& path, JobObserver* observer)
+      : path_(path), observer_(observer) {}

[rvargas (doing something else), 2014/04/08 16:53:45]

nit: {
}
(this is not a single line anymore)

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Done.

+
+  virtual ~CallbacksJobFactory() {}
+
+  virtual URLRequestJob* MaybeCreateJobWithProtocolHandler(
+      const std::string& scheme,
+      URLRequest* request,
+      NetworkDelegate* network_delegate) const OVERRIDE {
+    URLRequestFileJobWithCallbacks* job = new URLRequestFileJobWithCallbacks(
+        request,
+        network_delegate,
+        path_,
+        base::MessageLoop::current()->message_loop_proxy());
+    observer_->OnJobCreated(job);
+    return job;
+  }
+
+  virtual bool IsHandledProtocol(const std::string& scheme) const OVERRIDE {
+    return scheme == "file";
+  }
+
+  virtual bool IsHandledURL(const GURL& url) const OVERRIDE {
+    return IsHandledProtocol(url.scheme());
+  }
+
+  virtual bool IsSafeRedirectTarget(const GURL& location) const OVERRIDE {
+    return false;
+  }
+
+ private:
+  base::FilePath path_;
+  JobObserver* observer_;
+};
+
+// Helper function to create a file in |directory| named |filename| filled with
+// |content|. Returns true on succes and fills in |path| with the full path to
+// the file. If |directory| does not exist yet, it will be created.
+bool CreateTempFileWithContent(const std::string& filename,
+                               const std::string& content,
+                               base::ScopedTempDir* directory,
+                               base::FilePath* path) {
+  if (directory == NULL || path == NULL || !IsStringASCII(filename))
+    return false;
+  if (!directory->IsValid() && !directory->CreateUniqueTempDir())
+    return false;
+  *path = directory->path().AppendASCII(filename);
+
+  base::File file(*path,
+                  base::File::FLAG_OPEN_ALWAYS | base::File::FLAG_APPEND);

[rvargas (doing something else), 2014/04/08 16:53:45]

Why not CREATE and WRITE? Appending to a file seems like unexpected behavior for
a test. It's better to fail earlier.

+  int bytes_written = file.WriteAtCurrentPos(content.c_str(), content.length());

[rvargas (doing something else), 2014/04/08 16:53:45]

file.Write(0, ...) ? (assuming CREATE and WRITE)

[rvargas (doing something else), 2014/04/08 16:53:45]

should check that the file is valid before using it. (may crash otherwise)

+  if (bytes_written <= 0 || (unsigned)bytes_written != content.length())
+    return false;
+  file.Close();

[rvargas (doing something else), 2014/04/08 16:53:45]

no need to close.

btw, this whole block should probably be replaced with base::WriteFile()... and
maybe base::CreateTemporaryFileInDir()

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Ah, much simpler, thanks for the tip. Done. (this obsoletes the fixes for the
above 3 comments). 

+
+  return true;
+}
+
+// A superclass for tests of the OnSeekComplete / OnReadComplete functions of
+// URLRequestFileJob.
+class URLRequestFileJobEventsTest : public testing::Test {

[rvargas (doing something else), 2014/04/08 16:53:45]

nit: I think this class is too long to have an inline implementation (which is
easier to write but harder to read)

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Good point. I've moved the JobObserverImpl and Range classes out to be
standalone since they didn't really need to be nested classes, and switched to
separating declaration/definition for the rest of the class. 

+ public:
+  URLRequestFileJobEventsTest() {}
+
+  class JobObserverImpl : public CallbacksJobFactory::JobObserver {
+   public:
+    virtual void OnJobCreated(URLRequestFileJobWithCallbacks* job) OVERRIDE {
+      jobs_.push_back(job);
+    }
+
+    typedef std::vector<scoped_refptr<URLRequestFileJobWithCallbacks> > JobList;
+
+    const JobList& jobs() { return jobs_; }
+
+   protected:
+    JobList jobs_;
+  };
+
+  // A simple holder for start/end used in http range requests.
+  struct Range {
+    int start;
+    int end;
+
+    Range() {
+      start = 0;
+      end = 0;
+    }
+
+    Range(int start, int end) {
+      this->start = start;
+      this->end = end;
+    }
+  };
+
+ protected:
+  // This creates a file with |content| as the contents, and then creates and
+  // runs a URLRequestFileJobWithCallbacks job to get the contents out of it,
+  // and makes sure that the callbacks observed the correct bytes. If a Range
+  // is provided, this function will add the appropriate Range http header to
+  // the request and verify that only the bytes in that range (inclusive) were
+  // observed.
+  void RunRequest(const std::string& content, const Range* range) {
+    base::ScopedTempDir directory;
+    base::FilePath path;
+    ASSERT_TRUE(
+        CreateTempFileWithContent("file.dat", content, &directory, &path));
+    CallbacksJobFactory factory(path, &observer_);
+    context_.set_job_factory(&factory);
+
+    TestURLRequest request(
+        FilePathToFileURL(path), DEFAULT_PRIORITY, &delegate_, &context_);
+    if (range) {
+      ASSERT_GE(range->start, 0);
+      ASSERT_GE(range->end, 0);
+      ASSERT_LT((unsigned)range->end, content.length());

[rvargas (doing something else), 2014/04/08 16:53:45]

nit:c++ cast

[rvargas (doing something else), 2014/04/08 16:53:45]

+ assert start <= end ?

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Done.

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Done.

+      std::string range_value =
+          base::StringPrintf("bytes=%d-%d", range->start, range->end);
+      request.SetExtraRequestHeaderByName(
+          HttpRequestHeaders::kRange, range_value, true /*overwrite*/);
+    }
+    request.Start();
+
+    base::RunLoop loop;
+    loop.Run();
+
+    EXPECT_FALSE(delegate_.request_failed());
+    int expected_length =
+        range ? (range->end - range->start + 1) : content.length();
+    EXPECT_EQ(delegate_.bytes_received(), expected_length);
+
+    std::string expected_content;
+    if (range) {
+      expected_content.insert(0, content, range->start, expected_length);
+    } else {
+      expected_content = content;
+    }
+    EXPECT_TRUE(delegate_.data_received() == expected_content);

[rvargas (doing something else), 2014/04/08 16:53:45]

Doesn't this beeps and prints garbage when it fails?

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Nope- notice I used EXPECT_TRUE on single boolean expression instead of
EXPECT_EQ with 2 parameters. I probably had to learn to do this the hard way
already, after locally producing the beeps and garbage you mention. =)

[rvargas (doing something else), 2014/04/09 23:48:50]

yes, I missed that!

+
+    ASSERT_EQ(observer_.jobs().size(), 1u);
+    ASSERT_EQ(observer_.jobs().at(0)->seek_position(),
+              range ? range->start : 0);
+
+    std::string observed_content;
+    const std::vector<std::string>& chunks =
+        observer_.jobs().at(0)->data_chunks();
+    for (std::vector<std::string>::const_iterator i = chunks.begin();
+         i != chunks.end();
+         ++i) {
+      observed_content.append(*i);
+    }
+    EXPECT_EQ(expected_content, observed_content);
+    EXPECT_TRUE(expected_content == observed_content);
+  }
+
+  JobObserverImpl observer_;
+  TestURLRequestContext context_;
+  TestDelegate delegate_;
+};
+
+// Helper function to make a character array filled with |size| bytes of
+// test content.
+std::string MakeContentOfSize(int size) {
+  std::string result;
+  for (int i = 0; i < size; i++) {
+    result.append(1, static_cast<char>(i % 256));

[rvargas (doing something else), 2014/04/08 16:53:45]

does it make sense to preallocate the string (considering this is a test)?
dunno. Just thinking out loud.

[asargent_no_longer_on_chrome, 2014/04/09 20:59:59]

Yes, good idea. Done. 

+  }
+  return result;
+}
+
+TEST_F(URLRequestFileJobEventsTest, TinyFile) {
+  RunRequest(std::string("hello world"), NULL);
+}
+
+TEST_F(URLRequestFileJobEventsTest, SmallFile) {
+  RunRequest(MakeContentOfSize(17 * 1024), NULL);
+}
+
+TEST_F(URLRequestFileJobEventsTest, BigFile) {
+  RunRequest(MakeContentOfSize(3 * 1024 * 1024), NULL);
+}
+
+TEST_F(URLRequestFileJobEventsTest, Range) {
+  // Use a 15KB content file and read a range chosen somewhat arbitrarily but
+  // not aligned on any likely page boundaries.
+  int size = 15 * 1024;
+  Range range(1701, (6 * 1024) + 3);
+  RunRequest(MakeContentOfSize(size), &range);
+}
+
+}  // namespace

[rvargas (doing something else), 2014/04/08 16:53:45]

nit: do you mind keeping the actual tests outside of the namespaces? 

Do you need the net namespace at all? Seems like there would be very few
explicit net:: otherwise.

+}  // namespace net