Test file errors in downloads.

content/test/test_file_error_injector.h

+// Copyright (c) 2011 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 CONTENT_TEST_TEST_FILE_ERROR_INJECTOR_H_
+#define CONTENT_TEST_TEST_FILE_ERROR_INJECTOR_H_
+#pragma once
+
+#include "base/memory/ref_counted.h"
+#include "net/base/net_errors.h"
+
+namespace content {
+
+// Test helper for injecting errors into |DownloadFile| operations.
+// All errors must be injected before download starts.
+class TestFileErrorInjector
+    : public base::RefCountedThreadSafe<TestFileErrorInjector> {

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

I'd like some documentation as to why this class is RefCountedThreadSafe<>. 
That might be more appropriate in the implementation file than here, but it's
declared here, so you should as least have a reference to the explanation.  And
if you can explain (in terms that are restricted to what's known by content
customers) why we're RCTS here, that's better.

[ahendrickson, 2012/03/01 09:17:32]

Done.

+ public:
+  enum FileOperationCode {
+    FILE_OPERATION_INITIALIZE,
+    FILE_OPERATION_WRITE,
+    FILE_OPERATION_RENAME
+  };
+
+  // Structure that encapsulates the information needed to inject a file error.
+  struct FileErrorInfo {
+    int file_index;  // 0-based index into created DownloadFile instances.

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

I don't like references to DownloadFile in this file since this is part of the
content/chrome interface and DownloadFile is not.  Can you phrase it some other
way?

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

I don't like 0 based indices, because they presume that there's some specified
order (i.e. no racing) to download initiation.  Maybe you could identify files
by target name?  Do we know the URL on the file thread?  I'm looking for some
other key than initiation order, which seems fragile to me.

[ahendrickson, 2012/03/01 09:17:32]

Done.

[ahendrickson, 2012/03/01 09:17:32]

There will be multiple calls with the same URL, and we don't have access to the
uniquified file names to distinguish them by.

The tests that use this class will only download one file at a time, so that the
order of downloads is guaranteed.

[ahendrickson, 2012/03/01 09:21:15]

Now using the URL as the distinguishing value, allowing multiple downloads to be
uniquely identified as long as we don't have two with the same URL downloading
at once.

+    FileOperationCode code;  // Operation to affect.
+    int operation_instance;  // 0-based count of operation calls.

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

These two options strike me as having some redundancy--i.e. the
operation_instance fully specifies the operation to inject the error into, and
the code is an extra restriction.  Document what happens when these conflict?

[ahendrickson, 2012/03/01 09:17:32]

No, they're not redundant.  The code specifies the function to inject an error
into (Initialize, write, etc.).  The instance says how many calls to wait for
before inserting an error.

[Randy Smith (Not in Mondays), 2012/03/01 20:28:24]

Ah, got it.  The key point I was missing was that the operation count that
operation_instance was matched against was per operation type.  You might want
to modify the comment above.  "... operation calls with code code" or something
like it.

[ahendrickson, 2012/03/02 21:58:47]

Done.

+    net::Error net_error;  // Error to inject.
+  };
+
+  TestFileErrorInjector() {}
+
+  // Creates an instance.
+  static scoped_refptr<TestFileErrorInjector> Create();

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

Could you specify whether this has singleton semantics?  If so, this should
probably be termed "Get()".  If not, what happens if you create multiple
instances of this class?

[ahendrickson, 2012/03/01 09:17:32]

Hmm, I suppose it should have singleton semantics.

It would definitely have unexpected behavior (from the test writer's
perspective) if it were called more than once.

Changed.

+
+  // Adds an error.  Must be called before InjectErrors().

[cbentzel, 2012/02/28 14:51:41]

Nit: |InjectErrors()|

[ahendrickson, 2012/03/01 09:17:32]

Done.

+  virtual bool AddError(const FileErrorInfo& error_info) = 0;
+
+  // Injects the errors.  Can be called only once.

[cbentzel, 2012/02/28 14:51:41]

Nit: Comment more about how this impacts the DownloadFileManager, the fact that
it needs to already exist, etc.

[Randy Smith (Not in Mondays), 2012/02/28 22:06:13]

In an ideal world, comments in this file would not refer to the
DownloadFileManager, since this file is part of the content/chrome interface and
the DFM is not.  I.e. try to find a way to include the information Chris is
requesting without refering to the DFM.

[ahendrickson, 2012/03/01 09:17:32]

Done.

[ahendrickson, 2012/03/01 09:17:32]

Done.

+  virtual bool InjectErrors() = 0;
+
+  // Get information about the DownloadFiles created.
+  virtual size_t CurrentFileCount() const = 0;
+  virtual size_t FileCreationCount() const = 0;

[cbentzel, 2012/02/28 14:51:41]

Nit: Perhaps TotalFileCount to match CurrentFileCount?

[ahendrickson, 2012/03/01 09:17:32]

Done.

+  virtual bool HasFile(int file_index) const = 0;
+
+  // Debugging only.

[cbentzel, 2012/02/28 14:51:41]

This isn't needed - or at least needed for public code since never used.

[ahendrickson, 2012/03/01 09:17:32]

It's used now.

+  static std::string DebugString(FileOperationCode code);
+
+ protected:
+  virtual ~TestFileErrorInjector() {}
+
+ private:
+  friend class base::RefCountedThreadSafe<TestFileErrorInjector>;
+
+  DISALLOW_COPY_AND_ASSIGN(TestFileErrorInjector);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_TEST_TEST_FILE_ERROR_INJECTOR_H_