Lots of small code review cleanups from #7809:

* Got rid of unnecessary partial in search-summary-paging.html
* Refactored change notifications in Document to _notifyDocumentChange() method, and fix up similar logic in SpecRunnerUtils.createMockActiveDocument()
* Moved makeDialogFileList from StringUtils to FileUtils
* Renamed "file/item/index" to "fileIndex/itemIndex/matchIndex" for clarity in SearchResultsView
* Return empty array from _getSearchMatches instead of null
* Properly debounce document changes when updating search results view
* Fix up file rename handling in search results view to use indexOf() instead of match()
* Rename "doReplaceAll" event on FindBar to "replaceAll"
* Return a promise from FindInFilesUI.searchAndShowResults()
* Replace calls to _.filter() and _.map() with native JS calls
* Lots of comment/JSDoc improvements

src/document/Document.js

@@ -275,6 +275,17 @@ define(function (require, exports, module) {
         // _handleEditorChange() triggers "change" event
     };
 
+    /**
+     * @private
+     * Triggers the appropriate events when a change occurs: "change" on the Document instance
+     * and "documentChange" on the Document module.
+     * @param {Object} changeList Changelist in CodeMirror format
+     */
+    Document.prototype._notifyDocumentChange = function (changeList) {
+        $(this).triggerHandler("change", [this, changeList]);
+        $(exports).triggerHandler("documentChange", [this, changeList]);
+    };
+
     /**
      * Sets the contents of the document. Treated as reloading the document from disk: the document
      * will be marked clean with a new timestamp, the undo/redo history is cleared, and we re-check
@@ -303,9 +314,7 @@ define(function (require, exports, module) {
                 // TODO: Dumb to split it here just to join it again in the change handler, but this is
                 // the CodeMirror change format. Should we document our change format to allow this to
                 // either be an array of lines or a single string?
-                var fakeChangeList = [{text: text.split(/\r?\n/)}];
-                $(this).triggerHandler("change", [this, fakeChangeList]);
-                $(exports).triggerHandler("documentChange", [this, fakeChangeList]);
+                this._notifyDocumentChange([{text: text.split(/\r?\n/)}]);
             }
         }
         this._updateTimestamp(newTimestamp);
@@ -410,11 +419,10 @@ define(function (require, exports, module) {
         }
 
         // Notify that Document's text has changed
-        // TODO: This needs to be kept in sync with SpecRunnerUtils.createMockDocument(). In the
+        // TODO: This needs to be kept in sync with SpecRunnerUtils.createMockActiveDocument(). In the
         // future, we should fix things so that we either don't need mock documents or that this
         // is factored so it will just run in both.
-        $(this).triggerHandler("change", [this, changeList]);
-        $(exports).triggerHandler("documentChange", [this, changeList]);
+        this._notifyDocumentChange(changeList);
     };
 
     /**

src/document/DocumentCommandHandlers.js

@@ -1174,7 +1174,7 @@ define(function (require, exports, module) {
 
         } else {
             // Multiple unsaved files: show a single bulk prompt listing all files
-            var message = Strings.SAVE_CLOSE_MULTI_MESSAGE + StringUtils.makeDialogFileList(_.map(unsavedDocs, _shortTitleForDocument));
+            var message = Strings.SAVE_CLOSE_MULTI_MESSAGE + FileUtils.makeDialogFileList(_.map(unsavedDocs, _shortTitleForDocument));
 
             Dialogs.showModalDialog(
                 DefaultDialogs.DIALOG_ID_SAVE_CLOSE,
@@ -1232,9 +1232,12 @@ define(function (require, exports, module) {
     /**
      * Closes all open documents; equivalent to calling handleFileClose() for each document, except
      * that unsaved changes are confirmed once, in bulk.
-     * @param {?{promptOnly: boolean}}  If true, only displays the relevant confirmation UI and does NOT
+     * @param {?{promptOnly: boolean, _forceClose: boolean}}
+     *          If promptOnly is true, only displays the relevant confirmation UI and does NOT
      *          actually close any documents. This is useful when chaining close-all together with
      *          other user prompts that may be cancelable.
+     *          If _forceClose is true, forces the files to close with no confirmation even if dirty. 
+     *          Should only be used for unit test cleanup.
      * @return {$.Promise} a promise that is resolved when all files are closed
      */
     function handleFileCloseAll(commandData) {

src/file/FileUtils.js

@@ -183,6 +183,21 @@ define(function (require, exports, module) {
         );
     }
 
+    /**
+     * Creates an HTML string for a list of files to be reported on, suitable for use in a dialog.
+     * @param {Array.<string>} Array of filenames or paths to display.
+     */
+    function makeDialogFileList(paths) {
+        var result = "<ul class='dialog-list'>";
+        paths.forEach(function (path) {
+            result += "<li><span class='dialog-filename'>";
+            result += StringUtils.breakableUrl(path);
+            result += "</span></li>";
+        });
+        result += "</ul>";
+        return result;
+    }
+
     /**
      * Convert a URI path to a native path.
      * On both platforms, this unescapes the URI
@@ -446,11 +461,12 @@ define(function (require, exports, module) {
     }
 
     /**
-     * Compares two paths. Useful for sorting.
-     * @param {string} filename1
-     * @param {string} filename2
-     * @param {boolean} extFirst If true it compares the extensions first and then the file names.
-     * @return {number} The result of the local compare function
+     * Compares two paths segment-by-segment, used for sorting. Sorts folders before files,
+     * and sorts files based on `compareFilenames()`.
+     * @param {string} path1
+     * @param {string} path2
+     * @return {number} -1, 0, or 1 depending on whether path1 is less than, equal to, or greater than
+     *     path2 according to this ordering.
      */
     function comparePaths(path1, path2) {
         var entryName1, entryName2,
@@ -486,6 +502,7 @@ define(function (require, exports, module) {
     exports.translateLineEndings           = translateLineEndings;
     exports.showFileOpenError              = showFileOpenError;
     exports.getFileErrorString             = getFileErrorString;
+    exports.makeDialogFileList             = makeDialogFileList;
     exports.readAsText                     = readAsText;
     exports.writeText                      = writeText;
     exports.convertToNativePath            = convertToNativePath;

src/htmlContent/search-results.html

@@ -1,14 +1,14 @@
 <table class="bottom-panel-table table table-striped table-condensed row-highlight">
     <tbody>
         {{#searchList}}
-        <tr class="file-section" data-file="{{file}}">
+        <tr class="file-section" data-file-index="{{fileIndex}}">
             <td colspan="{{#replace}}3{{/replace}}{{^replace}}2{{/replace}}">
                 <span class="disclosure-triangle expanded" title="{{Strings.FIND_IN_FILES_EXPAND_COLLAPSE}}"></span>
                 {{{filename}}}
             </td>
         </tr>
         {{#items}}
-        <tr data-file="{{file}}" data-item="{{item}}" data-index="{{index}}">
+        <tr data-file-index="{{fileIndex}}" data-item-index="{{itemIndex}}" data-match-index="{{matchIndex}}">
             {{#replace}}<td class="checkbox-column"><input type="checkbox" class="check-one" {{#isChecked}}checked="true"{{/isChecked}} /></td>{{/replace}}
             <td class="line-number">{{line}}</td>
             <td class="line-text">{{pre}}<span class="highlight">{{highlight}}</span>{{post}}</td>

src/htmlContent/search-summary.html

@@ -13,7 +13,15 @@
 <div class="contracting-col">{{{scope}}}</div>
 {{/scope}}
 <div class="fixed-col">{{{summary}}}</div>
-{{>paging}}
+{{#hasPages}}
+<div class="pagination-col">
+    <span class="first-page {{^hasPrev}}disabled{{/hasPrev}}"></span>
+    <span class="prev-page {{^hasPrev}}disabled{{/hasPrev}}"></span>
+    {{{results}}}
+    <span class="next-page {{^hasNext}}disabled{{/hasNext}}"></span>
+    <span class="last-page {{^hasNext}}disabled{{/hasNext}}"></span>
+</div>
+{{/hasPages}}
 {{#replace}}
 <div class="replace-col">
     <button class="btn small replace-checked">{{Strings.BUTTON_REPLACE}}</button>

src/search/FindInFiles.js

@@ -79,15 +79,15 @@ define(function (require, exports, module) {
 
     /**
      * @private
-     * Searches through the contents an returns an array of matches
+     * Searches through the contents and returns an array of matches
      * @param {string} contents
      * @param {RegExp} queryExpr
-     * @return {Array.<{start: {line:number,ch:number}, end: {line:number,ch:number}, line: string}>}
+     * @return {!Array.<{start: {line:number,ch:number}, end: {line:number,ch:number}, line: string}>}
      */
     function _getSearchMatches(contents, queryExpr) {
         // Quick exit if not found or if we hit the limit
         if (searchModel.foundMaximum || contents.search(queryExpr) === -1) {
-            return null;
+            return [];
         }
 
         var match, lineNum, line, ch, matchLength,
@@ -129,13 +129,14 @@ define(function (require, exports, module) {
      * Searches and stores the match results for the given file, if there are matches
      * @param {string} fullPath
      * @param {string} contents
-     * @param {RegExp} queryExpr
+     * @param {!RegExp} queryExpr
+     * @param {!Date} timestamp
      * @return {boolean} True iff the matches were added to the search results
      */
     function _addSearchMatches(fullPath, contents, queryExpr, timestamp) {
         var matches = _getSearchMatches(contents, queryExpr);
 
-        if (matches && matches.length) {
+        if (matches.length) {
             searchModel.addResultMatches(fullPath, matches, timestamp);
             return true;
         }
@@ -201,7 +202,7 @@ define(function (require, exports, module) {
 
                 // Searches only over the lines that changed
                 matches = _getSearchMatches(lines.join("\r\n"), searchModel.queryExpr);
-                if (matches && matches.length) {
+                if (matches.length) {
                     // Updates the line numbers, since we only searched part of the file
                     matches.forEach(function (value, key) {
                         matches[key].start.line += change.from.line;
@@ -232,7 +233,8 @@ define(function (require, exports, module) {
         });
 
         if (resultsChanged) {
-            searchModel.fireChanged();
+            // Debounce document changes since the user might be typing quickly.
+            searchModel.fireChanged(true);
         }
     }
 
@@ -270,6 +272,8 @@ define(function (require, exports, module) {
     /**
      * Finds all candidate files to search in the given scope's subtree that are not binary content. Does NOT apply
      * the current filter yet.
+     * @param {?FileSystemEntry} scope Search scope, or null if whole project
+     * @return {$.Promise} A promise that will be resolved with the list of files in the scope. Never rejected.
      */
     function getCandidateFiles(scope) {
         function filter(file) {
@@ -278,8 +282,7 @@ define(function (require, exports, module) {
 
         // If the scope is a single file, just check if the file passes the filter directly rather than
         // trying to use ProjectManager.getAllFiles(), both for performance and because an individual
-        // in-memory file might be an untitled document or external file that doesn't show up in
-        // getAllFiles().
+        // in-memory file might be an untitled document that doesn't show up in getAllFiles().
         if (scope && scope.isFile) {
             return new $.Deferred().resolve(filter(scope) ? [scope] : []).promise();
         } else {
@@ -437,7 +440,8 @@ define(function (require, exports, module) {
      * @param {{query: string, caseSensitive: boolean, isRegexp: boolean}} queryInfo Query info object
      * @param {?Entry} scope Project file/subfolder to search within; else searches whole project.
      * @param {?string} filter A "compiled" filter as returned by FileFilters.compile(), or null for no filter
-     * @param {?string} replaceText If this is a replacement, the text to replace matches with.
+     * @param {?string} replaceText If this is a replacement, the text to replace matches with. This is just
+     *      stored in the model for later use - the replacement is not actually performed right now.
      * @param {?$.Promise} candidateFilesPromise If specified, a promise that should resolve with the same set of files that
      *      getCandidateFiles(scope) would return.
      * @return {$.Promise} A promise that's resolved with the search results or rejected when the find competes.
@@ -487,7 +491,7 @@ define(function (require, exports, module) {
 
         // Update the search results
         _.forEach(searchModel.results, function (item, fullPath) {
-            if (fullPath.match(oldName)) {
+            if (fullPath.indexOf(oldName) === 0) {
                 searchModel.results[fullPath.replace(oldName, newName)] = item;
                 delete searchModel.results[fullPath];
                 resultsChanged = true;

src/search/FindInFilesUI.js

@@ -25,13 +25,9 @@
 /*global define, $, window, Mustache */
 
 /*
- * Adds a "find in files" command to allow the user to find all occurrences of a string in all files in
- * the project.
- * 
- * The keyboard shortcut is Cmd(Ctrl)-Shift-F.
+ * UI and controller logic for find/replace across multiple files within the project.
  *
  * FUTURE:
- *  - Search files in working set that are *not* in the project
  *  - Handle matches that span multiple lines
  */
 define(function (require, exports, module) {
@@ -44,6 +40,7 @@ define(function (require, exports, module) {
         DefaultDialogs    = require("widgets/DefaultDialogs"),
         EditorManager     = require("editor/EditorManager"),
         FileFilters       = require("search/FileFilters"),
+        FileUtils         = require("file/FileUtils"),
         FindBar           = require("search/FindBar").FindBar,
         FindInFiles       = require("search/FindInFiles"),
         FindUtils         = require("search/FindUtils"),
@@ -76,7 +73,7 @@ define(function (require, exports, module) {
      * @return {$.Promise} A promise that's resolved with the search results or rejected when the find competes.
      */
     function searchAndShowResults(queryInfo, scope, filter, replaceText, candidateFilesPromise) {
-        FindInFiles.doSearchInScope(queryInfo, scope, filter, replaceText, candidateFilesPromise)
+        return FindInFiles.doSearchInScope(queryInfo, scope, filter, replaceText, candidateFilesPromise)
             .done(function (zeroFilesToken) {
                 // Done searching all files: show results
                 if (FindInFiles.searchModel.hasResults()) {
@@ -263,7 +260,7 @@ define(function (require, exports, module) {
 
         // Clone the search results so that they don't get updated in the middle of the replacement.
         var resultsClone = _.cloneDeep(model.results),
-            replacedFiles = _.filter(Object.keys(resultsClone), function (path) {
+            replacedFiles = Object.keys(resultsClone).filter(function (path) {
                 return FindUtils.hasCheckedMatches(resultsClone[path]);
             }),
             isRegexp = model.queryInfo.isRegexp,
@@ -273,8 +270,8 @@ define(function (require, exports, module) {
             StatusBar.showBusyIndicator(true);
             FindInFiles.doReplace(resultsClone, replaceText, { forceFilesOpen: forceFilesOpen, isRegexp: isRegexp })
                 .fail(function (errors) {
-                    var message = Strings.REPLACE_IN_FILES_ERRORS + StringUtils.makeDialogFileList(
-                            _.map(errors, function (errorInfo) {
+                    var message = Strings.REPLACE_IN_FILES_ERRORS + FileUtils.makeDialogFileList(
+                            errors.map(function (errorInfo) {
                                 return ProjectManager.makeProjectRelativeIfPossible(errorInfo.item);
                             })
                         );
@@ -370,7 +367,7 @@ define(function (require, exports, module) {
         var model = FindInFiles.searchModel;
         _resultsView = new SearchResultsView(model, "find-in-files-results", "find-in-files.results");
         $(_resultsView)
-            .on("doReplaceAll", function () {
+            .on("replaceAll", function () {
                 _finishReplaceAll(model);
             })
             .on("close", function () {

src/search/FindReplace.js

@@ -66,12 +66,6 @@ define(function (require, exports, module) {
      */
     var FIND_HIGHLIGHT_MAX  = 2000;
 
-    /**
-     * Maximum number of matches to collect for Replace All; any additional matches are not listed in the panel & are not replaced
-     * @const {number}
-     */
-    var REPLACE_ALL_MAX     = 10000;
-
     /**
      * Instance of the currently opened document when replaceAllPanel is visible
      * @type {?Document}

src/search/FindUtils.js

@@ -73,8 +73,11 @@ define(function (require, exports, module) {
     function _doReplaceInDocument(doc, matchInfo, replaceText, isRegexp) {
         // Double-check that the open document's timestamp matches the one we recorded. This
         // should normally never go out of sync, because if it did we wouldn't start the
-        // replace in the first place, but we want to double-check. This will *not* handle
-        // cases where the document has been edited in memory since the matchInfo was generated.
+        // replace in the first place (due to the fact that we immediately close the search
+        // results panel whenever we detect a filesystem change that affects the results),
+        // but we want to double-check in case we don't happen to get the change in time.
+        // This will *not* handle cases where the document has been edited in memory since 
+        // the matchInfo was generated.
         if (doc.diskTimestamp.getTime() !== matchInfo.timestamp.getTime()) {
             return new $.Deferred().reject(exports.ERROR_FILE_CHANGED).promise();
         }
@@ -171,6 +174,8 @@ define(function (require, exports, module) {
      * Checks timestamps to ensure replacements are not performed in files that have changed on disk since
      * the original search results were generated. However, does *not* check whether edits have been performed
      * in in-memory documents since the search; it's up to the caller to guarantee this hasn't happened.
+     * (When called from the standard Find in Files UI, SearchResultsView guarantees this. If called headlessly,
+     * the caller needs to track changes.)
      * 
      * Replacements in documents that are already open in memory at the start of the replacement are guaranteed to
      * happen synchronously; replacements in files on disk will return an error if the on-disk file changes between
@@ -211,6 +216,7 @@ define(function (require, exports, module) {
 
                     if (firstPath) {
                         var newDoc = DocumentManager.getOpenDocumentForPath(firstPath);
+                        // newDoc might be null if the replacement failed.
                         if (newDoc) {
                             DocumentManager.setCurrentDocument(newDoc);
                         }