Addresses a gr-diff-highlight edge case
It was possible to cause a JS error when creating a ranged comment that started at the very end of the first line (selecting no content on that line). The relevant null-guard needed an additional set of parens to avoid evaluating the second OR operand with a bad argument in this case. Addresses the null-guard boolean expressions in `_normalizeStart` and `_normalizeEnd` and reduces the number of calls to `_getLength` from thrice to once per iteration. Adds a relevant unit test. Change-Id: I98848f9f6089fd3240bda175765770c9f9c5ba30
This commit is contained in:
		| @@ -494,12 +494,13 @@ | ||||
|       } | ||||
|  | ||||
|       // Skip nodes before startOffset. | ||||
|       while (startNode && | ||||
|           this._getLength(startNode) <= startOffset || | ||||
|           this._getLength(startNode) === 0) { | ||||
|         startOffset -= this._getLength(startNode); | ||||
|       var nodeLength = this._getLength(startNode); | ||||
|       while (startNode && (nodeLength <= startOffset || nodeLength === 0)) { | ||||
|         startOffset -= nodeLength; | ||||
|         startNode = startNode.nextSibling; | ||||
|         nodeLength = startNode && this._getLength(startNode); | ||||
|       } | ||||
|       if (!startNode) { return null; } | ||||
|  | ||||
|       // Split Text node. | ||||
|       if (startNode instanceof Text) { | ||||
| @@ -546,12 +547,13 @@ | ||||
|       } | ||||
|  | ||||
|       // Find the node where endOffset points at. | ||||
|       while (endNode && | ||||
|           this._getLength(endNode) < endOffset || | ||||
|           this._getLength(endNode) === 0) { | ||||
|         endOffset -= this._getLength(endNode); | ||||
|       var nodeLength = this._getLength(endNode); | ||||
|       while (endNode && (nodeLength < endOffset || nodeLength === 0)) { | ||||
|         endOffset -= nodeLength; | ||||
|         endNode = endNode.nextSibling; | ||||
|         nodeLength = endNode && this._getLength(endNode); | ||||
|       } | ||||
|       if (!endNode) { return null; } | ||||
|  | ||||
|       if (endNode instanceof Text) { | ||||
|         endNode = | ||||
|   | ||||
| @@ -371,6 +371,40 @@ limitations under the License. | ||||
|       assert.equal(tabs[1].nextSibling.textContent, 'verbis, '); | ||||
|     }); | ||||
|  | ||||
|     test('multiline highlight w/ start at end of 1st line', function() { | ||||
|       var diff = element.querySelector('#diffTable'); | ||||
|       var startContent = | ||||
|           diff.querySelector('.left.lineNum[data-value="138"] ~ .content'); | ||||
|       var betweenContent = | ||||
|           diff.querySelector('.left.lineNum[data-value="140"] ~ .content'); | ||||
|       var endContent = | ||||
|           diff.querySelector('.left.lineNum[data-value="141"] ~ .content'); | ||||
|       var commentThread = | ||||
|           diff.querySelector('gr-diff-comment-thread'); | ||||
|       var builder = { | ||||
|         getCommentThreadByContentEl: sandbox.stub().returns(commentThread), | ||||
|         getContentByLine: sandbox.stub().returns({}), | ||||
|         getContentsByLineRange: sandbox.stub().returns([betweenContent]), | ||||
|         getLineElByChild: sandbox.stub().returns( | ||||
|             {getAttribute: sandbox.stub()}), | ||||
|       }; | ||||
|       element._cachedDiffBuilder = builder; | ||||
|       element.enabled = true; | ||||
|       builder.getContentByLine.withArgs(138, 'left').returns( | ||||
|           startContent); | ||||
|       builder.getContentByLine.withArgs(141, 'left').returns( | ||||
|           endContent); | ||||
|  | ||||
|       var expectedStartContentNodes = startContent.childNodes.length; | ||||
|  | ||||
|       // The following should not cause an error. | ||||
|       element._applyRangedHighlight( | ||||
|           'some', 138, startContent.textContent.length, 141, 28, 'left'); | ||||
|  | ||||
|       assert.equal(startContent.childNodes.length, expectedStartContentNodes, | ||||
|           'Should not add a highlight to the start content'); | ||||
|     }); | ||||
|  | ||||
|     suite('single line ranges', function() { | ||||
|       var diff; | ||||
|       var content; | ||||
|   | ||||
| @@ -42,7 +42,7 @@ limitations under the License. | ||||
|         getAccount: function() { | ||||
|           return Promise.resolve({name: 'Judy Hopps'}); | ||||
|         }, | ||||
|       }) | ||||
|       }); | ||||
|       element = fixture('basic'); | ||||
|       errorStub = sinon.stub(console, 'error'); | ||||
|       Gerrit.install(function(p) { plugin = p; }, '0.1', | ||||
|   | ||||
		Reference in New Issue
	
	Block a user
	 Wyatt Allen
					Wyatt Allen