Rework update model to eliminate a "flicker" resulting from the brief update to zero results when a…

ui/file_manager/file_manager/background/js/media_scanner.js

 // 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.
 
 /**
  * Class representing the results of a scan operation.
  *
  * @interface
  */
 importer.MediaScanner = function() {};
@@ skipping 76 matching lines @@
  * @constructor
  * @struct
  * @implements {importer.MediaScanner}
  *
  * @param {function(!FileEntry): !Promise.<string>} hashGenerator
  * @param {!importer.HistoryLoader} historyLoader
  * @param {!importer.DirectoryWatcherFactory} watcherFactory
  */
 importer.DefaultMediaScanner = function(
     hashGenerator, historyLoader, watcherFactory) {
+
+  /** @private {!importer.HistoryLoader} */
+  this.historyLoader_ = historyLoader;
+
+  /** @private {function(!FileEntry): !Promise.<string>} */
+  this.createHashcode_ = hashGenerator;
+
   /**
    * A little factory for DefaultScanResults which allows us to forgo
    * the saving it's dependencies in our fields.
    * @return {!importer.DefaultScanResult}
    */
   this.createScanResult_ = function() {
-    return new importer.DefaultScanResult(hashGenerator, historyLoader);
+    return new importer.DefaultScanResult();
   };
 
   /** @private {!Array.<!importer.ScanObserver>} */
   this.observers_ = [];
 
   /**
    * @private {!importer.DirectoryWatcherFactory}
    * @const
    */
   this.watcherFactory_ = watcherFactory;
@@ skipping 18 matching lines @@
 importer.DefaultMediaScanner.prototype.scan = function(entries) {
   if (entries.length == 0) {
     throw new Error('Cannot scan empty list of entries.');
   }
 
   var scanResult = this.createScanResult_();
   var watcher = this.watcherFactory_(
       /** @this {importer.DefaultMediaScanner} */
       function() {
         scanResult.invalidateScan();
-        this.observers_.forEach(
-            /** @param {!importer.ScanObserver} observer */
-            function(observer) {
-              observer(importer.ScanEvent.INVALIDATED, scanResult);
-            });
+        this.notify_(importer.ScanEvent.INVALIDATED, scanResult);
       }.bind(this));
   var scanPromises = entries.map(
       this.scanEntry_.bind(this, scanResult, watcher));
 
   Promise.all(scanPromises)
       .then(scanResult.resolve)
       .catch(scanResult.reject);
 
   scanResult.whenFinal()
       .then(
+          /** @this {importer.DefaultMediaScanner} */
           function() {
-            this.onScanFinished_(scanResult);
+            this.notify_(importer.ScanEvent.FINALIZED, scanResult);
           }.bind(this));
 
   return scanResult;
 };
 
 /**
- * Called when a scan is finished.
+ * Notifies all listeners at some point in the near future.
  *
+ * @param {!importer.ScanEvent} event
  * @param {!importer.DefaultScanResult} result
  * @private
  */
-importer.DefaultMediaScanner.prototype.onScanFinished_ = function(result) {
+importer.DefaultMediaScanner.prototype.notify_ = function(event, result) {
   this.observers_.forEach(
       /** @param {!importer.ScanObserver} observer */
       function(observer) {
-        observer(importer.ScanEvent.FINALIZED, result);
+        observer(event, result);
       });
 };
 
 /**
- * Resolves the entry to a list of {@code FileEntry}.
+ * Resolves the entry by either:
+ * a) recursing on it (when a directory)
+ * b) adding it to the results (when a media type file)
+ * c) ignoring it, if neither a or b
  *
- * @param {!importer.DefaultScanResult} result
+ * @param {!importer.DefaultScanResult} scan
  * @param {!importer.DirectoryWatcher} watcher
  * @param {!Entry} entry
+ *
  * @return {!Promise}
  * @private
  */
 importer.DefaultMediaScanner.prototype.scanEntry_ =
-    function(result, watcher, entry) {
-  return entry.isFile ?
-      result.onFileEntryFound(/** @type {!FileEntry} */ (entry)) :
-      this.scanDirectory_(
-          result, watcher, /** @type {!DirectoryEntry} */ (entry));
+    function(scan, watcher, entry) {
+  if (entry.isDirectory) {
+    return this.scanDirectory_(
+      scan,
+      watcher,
+      /** @type {!DirectoryEntry} */ (entry));
+  }
+
+  console.assert(entry.isFile);

[Ben Kwa, 2015/02/05 14:54:26]

Suggestion: include an explanatory string in the assert.

[Steve McKay, 2015/02/05 15:46:46]

Removed.

+  return this.onFileEntryFound_(scan, /** @type {!FileEntry} */ (entry));
 };
 
 /**
  * Finds all files beneath directory.
  *
- * @param {!importer.DefaultScanResult} result
+ * @param {!importer.DefaultScanResult} scan
  * @param {!importer.DirectoryWatcher} watcher
  * @param {!DirectoryEntry} entry
  * @return {!Promise}
  * @private
  */
 importer.DefaultMediaScanner.prototype.scanDirectory_ =
-    function(result, watcher, entry) {
+    function(scan, watcher, entry) {
   // Collect promises for all files being added to results.
   // The directory scan promise can't resolve until all
   // file entries are completely promised.
   var promises = [];
 
   return fileOperationUtil.findEntriesRecursively(
       entry,
-      /** @param  {!Entry} entry */
+      /**
+       * @param  {!Entry} entry
+       * @this {importer.DefaultMediaScanner}
+       */
       function(entry) {
         if (watcher.triggered) {
+          console.log('Skipping file entry...watched directory was modified.');

[hirono, 2015/02/05 10:51:43]

Is it intentionally left here?

[Steve McKay, 2015/02/05 15:46:46]

It was, but on second thought...removed it.

           return;
         }
+
         if (entry.isDirectory) {
+          // Note, there is no need for us to recurse, the utility
+          // funciton findEntriesRecursively does that. So we

[Ben Kwa, 2015/02/05 14:54:26]

function

[Steve McKay, 2015/02/05 15:46:46]

Done.

+          // just watch the directory for modifications, and that's it.
           watcher.addDirectory(/** @type {!DirectoryEntry} */(entry));
+          return;
+        }
+
+        promises.push(
+            this.onFileEntryFound_(scan, /** @type {!FileEntry} */(entry)));
+
+      }.bind(this))
+      .then(Promise.all.bind(Promise, promises));
+};
+
+/**
+ * Finds all files beneath directory.
+ *
+ * @param {!importer.DefaultScanResult} scan
+ * @param {!FileEntry} entry
+ * @return {!Promise}
+ * @private
+ */
+importer.DefaultMediaScanner.prototype.onFileEntryFound_ =
+    function(scan, entry) {
+
+  if (!FileType.isImageOrVideo(entry)) {
+    return Promise.resolve(false);

[Ben Kwa, 2015/02/05 14:54:26]

I think this should just be Promise.resolve().  The boolean return means nothing
here.

[Steve McKay, 2015/02/05 15:46:46]

Done.

+  }
+  return this.hasHistoryDuplicate_(entry).then(

[Ben Kwa, 2015/02/05 14:54:26]

nit: line break before .then

[Steve McKay, 2015/02/05 15:46:46]

Done.

+      /**
+       * @param {boolean} duplicate
+       * @return {!Promise}
+       * @this {importer.DefaultMediaScanner}
+       */
+      function(duplicate) {
+        if (duplicate) {
+          return false;
         } else {
-          promises.push(
-              result.onFileEntryFound(/** @type {!FileEntry} */(entry)));
+          return this.createHashcode_(entry)
+              .then(
+                  function(hashcode) {
+                    return scan.addFileEntry(entry, hashcode);
+                  });
         }
-      })
-      .then(Promise.all.bind(Promise, promises));
+      }.bind(this)).then(

[Ben Kwa, 2015/02/05 14:54:26]

nit: line break before .then

[Steve McKay, 2015/02/05 15:46:46]

Done.

+          /**
+           * @param {boolean} added
+           * @this {importer.DefaultMediaScanner}
+           */
+          function(added) {
+            if (added) {
+              this.notify_(importer.ScanEvent.UPDATED, scan);
+            }
+          }.bind(this));
+};
+
+/**
+ * @param {!FileEntry} entry
+ * @return {!Promise.<boolean>} True if there is a history-entry-duplicate
+ *     for the file.
+ * @private
+ */
+importer.DefaultMediaScanner.prototype.hasHistoryDuplicate_ = function(entry) {
+  return this.historyLoader_.getHistory()
+      .then(
+          /**
+           * @param {!importer.ImportHistory} history
+           * @return {!Promise}
+           * @this {importer.DefaultMediaScanner}
+           */
+          function(history) {
+            return Promise.all([
+              history.wasCopied(entry, importer.Destination.GOOGLE_DRIVE),
+              history.wasImported(entry, importer.Destination.GOOGLE_DRIVE)
+            ]).then(
+                /**
+                 * @param {!Array.<boolean>} results
+                 * @return {!Promise}
+                 * @this {importer.DefaultMediaScanner}
+                 */
+                function(results) {
+                  return results[0] || results[1];
+                }.bind(this));
+          }.bind(this));
 };
 
 /**
  * Results of a scan operation. The object is "live" in that data can and
  * will change as the scan operation discovers files.
  *
  * <p>The scan is complete, and the object will become static once the
  * {@code whenFinal} promise resolves.
  *
  * @constructor
  * @struct
  * @implements {importer.ScanResult}
- *
- * @param {function(!FileEntry): !Promise.<string>} hashGenerator
- * @param {!importer.HistoryLoader} historyLoader
  */
-importer.DefaultScanResult = function(hashGenerator, historyLoader) {
-
-  /** @private {function(!FileEntry): !Promise.<string>} */
-  this.createHashcode_ = hashGenerator;
-
-  /** @private {!importer.HistoryLoader} */
-  this.historyLoader_ = historyLoader;
+importer.DefaultScanResult = function() {
 
   /**
    * List of file entries found while scanning.
    * @private {!Array.<!FileEntry>}
    */
   this.fileEntries_ = [];
 
   /**
-   * @private {boolean}
-   */
-  this.invalidated_ = false;
-
-  /**
    * Hashcodes of all files included captured by this result object so-far.
    * Used to dedupe newly discovered files against other files withing
    * the ScanResult.
    * @private {!Object.<string, !FileEntry>}
    */
   this.fileHashcodes_ = {};
 
   /** @private {number} */
   this.totalBytes_ = 0;
 
   /**
    * The point in time when the scan was started.
    * @type {Date}
    */
   this.scanStarted_ = new Date();
 
   /**
    * The point in time when the last scan activity occured.
    * @type {Date}
    */
   this.lastScanActivity_ = this.scanStarted_;
 
+  /**
+   * @private {boolean}
+   */
+  this.invalidated_ = false;
+
   /** @private {!importer.Resolver.<!importer.ScanResult>} */
   this.resolver_ = new importer.Resolver();
 };
 
 /** @struct */
 importer.DefaultScanResult.prototype = {
 
   /** @return {function()} */
   get resolve() { return this.resolver_.resolve.bind(null, this); },
 
@@ skipping 31 matching lines @@
 };
 
 /**
  * Invalidates this scan.
  */
 importer.DefaultScanResult.prototype.invalidateScan = function() {
   this.invalidated_ = true;
 };
 
 /**
- * Handles files discovered during scanning.
- *
- * @param {!FileEntry} entry
- * @return {!Promise} Resolves once file entry has been processed
- *     and is represented in results.
- */
-importer.DefaultScanResult.prototype.onFileEntryFound = function(entry) {
-  this.lastScanActivity_ = new Date();
-
-  if (!FileType.isImageOrVideo(entry)) {
-    return Promise.resolve();
-  }
-
-  return this.historyLoader_.getHistory()
-      .then(
-          /**
-           * @param {!importer.ImportHistory} history
-           * @return {!Promise}
-           * @this {importer.DefaultScanResult}
-           */
-          function(history) {
-            return Promise.all([
-              history.wasCopied(entry, importer.Destination.GOOGLE_DRIVE),
-              history.wasImported(entry, importer.Destination.GOOGLE_DRIVE)
-            ]).then(
-                /**
-                 * @param {!Array.<boolean>} results
-                 * @return {!Promise}
-                 * @this {importer.DefaultScanResult}
-                 */
-                function(results) {
-                  return results[0] || results[1] ?
-                      Promise.resolve() :
-                      this.addFileEntry_(entry);
-                }.bind(this));
-          }.bind(this));
-};
-
-/**
  * Adds a file to results.
  *
  * @param {!FileEntry} entry
- * @return {!Promise} Resolves once file entry has been processed
- *     and is represented in results.
- * @private
+ * @param {string} hashcode
+ * @return {!Promise.<boolean>} True if the file as added, false if it was
+ *     rejected as a dupe.
  */
-importer.DefaultScanResult.prototype.addFileEntry_ = function(entry) {
-  return new Promise(
-      function(resolve, reject) {
-        this.createHashcode_(entry).then(
-            /**
-             * @param {string} hashcode
-             * @this {importer.DefaultScanResult}
-             */
-            function(hashcode) {
-              // Ignore the entry if it is a duplicate.
-              if (hashcode in this.fileHashcodes_) {
-                resolve();
-                return;
-              }
+importer.DefaultScanResult.prototype.addFileEntry = function(entry, hashcode) {
+  return new Promise(entry.getMetadata.bind(entry)).then(
+      /**
+       * @param {!Metadata} metadata
+       * @this {importer.DefaultScanResult}
+       */
+      function(metadata) {
+        console.assert(
+            'size' in metadata,
+            'size attribute missing from metadata.');
+        this.lastScanActivity_ = new Date();
 
-              entry.getMetadata(
-                  /**
-                   * @param {!Metadata} metadata
-                   * @this {importer.DefaultScanResult}
-                   */
-                  function(metadata) {
-                    console.assert(
-                        'size' in metadata,
-                        'size attribute missing from metadata.');
-                    this.lastScanActivity_ = new Date();
+        // We wait to check the hashcode until after all

[Ben Kwa, 2015/02/05 14:54:26]

I find this comment confusing.  Which async data are we waiting for?

[Steve McKay, 2015/02/05 15:46:46]

Rewrote to make the thought clearer.

+        // async data is loaded. This avoids a possible race.
+        if (hashcode in this.fileHashcodes_) {
+          return false;
+        }
 
-                    // Double check that a dupe entry wasn't added while we were
-                    // busy looking up metadata.
-                    if (hashcode in this.fileHashcodes_) {
-                      resolve();
-                      return;
-                    }
-                    entry.size = metadata.size;
-                    this.totalBytes_ += metadata['size'];
-                    this.fileHashcodes_[hashcode] = entry;
-                    this.fileEntries_.push(entry);
-                    resolve();
-                }.bind(this));
-            }.bind(this));
-      }.bind(this));
+        entry.size = metadata.size;
+        this.totalBytes_ += metadata['size'];
+        this.fileHashcodes_[hashcode] = entry;
+        this.fileEntries_.push(entry);
+        return true;
+
+    }.bind(this));
 };
 
 /**
  * Watcher for directories.
  * @interface
  */
 importer.DirectoryWatcher = function() {};
 
 /**
  * Registers new directory to be watched.
@@ skipping 59 matching lines @@
   if (!this.watchedDirectories_[event.entry.toURL()])
     return;
   this.triggered = true;
   for (var url in this.watchedDirectories_) {
     chrome.fileManagerPrivate.removeFileWatch(url, function() {});
   }
   chrome.fileManagerPrivate.onDirectoryChanged.removeListener(
       assert(this.listener_));
   this.callback_();
 };