Add ImportHistory class and RecordStorage class.

chrome/test/data/file_manager/unit_tests/import_history_unittest.js

Index: chrome/test/data/file_manager/unit_tests/import_history_unittest.js
diff --git a/chrome/test/data/file_manager/unit_tests/import_history_unittest.js b/chrome/test/data/file_manager/unit_tests/import_history_unittest.js
new file mode 100644
index 0000000000000000000000000000000000000000..15be8004591f0210b8582a1d51c80d7d03870cd3
--- /dev/null
+++ b/chrome/test/data/file_manager/unit_tests/import_history_unittest.js
@@ -0,0 +1,141 @@
+// Copyright 2014 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.
+'use strict';
+
+/** @const {string} */
+var GOOGLE_DRIVE = 'Google Drive';
+
+/** @const {string} */
+var DROPBOX = 'Dropbox';
+
+/** @type {!TestFileSystem} */

[mtomasz, 2014/10/27 00:40:11]

nit: The value is undefined, but jsdoc says it can't.

[Steve McKay, 2014/10/27 21:06:29]

So I read this as "this thing can't ever be null". Based on experience with our
internal compiled tests it can start out undefined without any compiler
complaints. But you can't then assign `undefined` to it unless it is declared
{!TestFileSystem|undefined}.

So I think this is the strictest way to declare the var.

[mtomasz, 2014/10/28 01:36:32]

Hm. I'm not really sure, we started using Closure compiler very recently in
Files app. Added @fukino who is an expert on closure compiling.

What I'm concerned it that you may pass testFileSystem to a function which
accepts {!TestFileSystem}. Closure compiler wouldn't complain because jsdoc
matches, but we will end up on passing undefined to the function. Correct me if
my understanding is wrong.

[fukino, 2014/10/28 04:05:59]

We use the style
  /** @type {!Foo} */
  var foo;
in externs files when the foo is already initialized in the global object.
But, in this case, I have the same concern with mtomasz.
If the foo is initialized later in this file, {!Foo|undefined} makes more sense
to me.

[Steve McKay, 2014/10/28 18:04:01]

Done. But respectfully under protest.

+var testFileSystem;
+
+/** @type {!MockFileEntry} */
+var testFileEntry;
+
+/** @type {!RecordStorage} */
+var storage;
+
+var historyLoader;

[mtomasz, 2014/10/27 00:40:11]

nit: jsdoc seems missing.

[Steve McKay, 2014/10/27 21:06:28]

Done.

+
+/** @type {!FileEntry} */
+// var entry;

[mtomasz, 2014/10/27 00:40:12]

nit: Shall we uncomment?

[Steve McKay, 2014/10/27 21:06:28]

Nuked.

+
+// Set up the test components.
+function setUp() {
+  testFileSystem = new TestFileSystem('abc-123');
+  testFileEntry = new MockFileEntry(
+      testFileSystem,
+      'hey/there', {
+        size: 1234,
+        modificationTime: 'Thursday, very late',
+      });
+
+  storage = new TestRecordStorage();
+  historyLoader = ImportHistory.load(storage);
+}
+
+
+var TestRecordStorage = function() {
+
+  // TODO(smckay): Existing records will be loaded
+  // when ImportHistory is constructed.
+  this.records_ = [

[mtomasz, 2014/10/27 00:40:11]

Please add jsdoc to members and methods.

[Steve McKay, 2014/10/27 21:06:29]

Let's call this done. Here's what I did and why. Lemme know if you're happy with
this appoach. If so, we'll stick to this pattern for future CLs.

THE WHY:

It sounds like the team is quickly moving towards using the closure compiler on
the source.

THE WHAT:

I introduced a RecordStorage interface, and renamed the existing implementation
to FileEntryRecordStorage. That'll let use easily substitute the test double in
the tests, even when they are compiled.

[mtomasz, 2014/10/28 01:36:32]

SGTM

[Steve McKay, 2014/10/28 18:04:01]

Acknowledged.

+    ['Thursday, very late_1234', GOOGLE_DRIVE],
+    ['99999_99999', DROPBOX]
+  ];
+
+  this.readAll = function() {
+    return Promise.resolve(this.records_);
+  };
+
+  this.write = function(record) {
+    this.records_.push(record);
+    return Promise.resolve();
+  };
+};
+
+
+/**
+ * @return {!Promise.<RecordStorage>}
+ */
+function createRealStorage() {
+  return new Promise(
+      function(resolve, reject) {
+        var onFsReady = function(fs) {

[mtomasz, 2014/10/27 00:40:11]

nit: Avoid abbreviations per our style guide. How about simply fileSystem?

[Steve McKay, 2014/10/27 21:06:29]

Done.

+          fs.root.getFile(
+              'test.data',
+              {
+                create: true,
+                exclusive: false
+              },
+              function(fileEntry) {
+                resolve(new RecordStorage(fileEntry));
+              },
+              reject);
+        };
+
+        window.webkitRequestFileSystem(
+            TEMPORARY,
+            1024 * 1024,
+            onFsReady,
+            reject);
+      });
+}
+
+
+function testRecordWriter(callback) {
+  createRealStorage().then(
+      function(storage) {
+        storage.write(['abc', '123']).then(
+            function() {
+              storage.readAll().then(
+                  function(records) {
+                    callback(/* error */ records.length != 1);
+                  },
+                  callback);
+            });
+        },
+        callback);
+}
+
+
+function testHistory_NotImported(callback) {

[mtomasz, 2014/10/27 00:40:11]

I'm not sure if _ in method names doesn't violate our style guide. Please check
other tests if we do that.

[Steve McKay, 2014/10/27 21:06:29]

It's permitted in Google style (internal) for basically qualifications of test
cases. I don't, however see it used in the immediate vicinity of this test.

I'd like to make the argument that this type of qualification can aid
readability of tests by allowing authors to pack details into a method name,
while retaining some reader-beneficial segmentation.

If you still don't like, I'll change. But at least consider the benefits.

[mtomasz, 2014/10/28 01:36:32]

It makes sense from the readability point of view, but our style guide doesn't
have this rule. By the way do we need the prefix, if it's the same for all test
cases in this file?

[Steve McKay, 2014/10/28 18:04:01]

Okay. Nuked the underscores, but under protest...respectfully.

testRecordWriter (now renamed testRecordStorage) doesn't share the same
testHistory prefix.

What's going on here is I'm cramming unit tests for two separate classes into
this one file. Usually it would be 1-to-1, and one class per file, but that
doesn't seem to be the norm in this codebase. Trying to stick to the norms right
now.

+  // TestRecordWriter is pre-configured with a Dropbox entry
+  // but not for this file.
+  historyLoader.then(
+    function(testHistory) {
+      testHistory.wasImported(testFileEntry, DROPBOX).then(

[mtomasz, 2014/10/27 00:40:11]

nit: Can we use some other fake name?

[Steve McKay, 2014/10/27 21:06:29]

Good point. Done..."Space Cloud" :)

+        function(result) {
+          callback(/* error */ result);
+        });
+    });
+}
+
+
+function testHistory_LoadsStoredRecords(callback) {
+  // TestRecordWriter is pre-configured with this entry.
+  historyLoader.then(
+    function(testHistory) {
+      testHistory.wasImported(testFileEntry, GOOGLE_DRIVE).then(
+        function(result) {
+          callback(/* error */ !result);
+        });
+    });
+}
+
+
+function testHistory_Updates(callback) {
+  historyLoader.then(
+    function(testHistory) {
+      testHistory.markImported(testFileEntry, 'Dropbox').then(

[mtomasz, 2014/10/27 00:40:11]

nit: Same here.

[Steve McKay, 2014/10/27 21:06:28]

Done.

+          function() {
+            testHistory.wasImported(testFileEntry, 'Dropbox').then(
+                function(result) {
+                  callback(/* error */ !result);
+                });
+          });
+    });
+}