Variable endLine is not correctly determined in styleDeclarationTextRange()

• Component: Web Inspector
• Status: NEW ---

[Tomas Popela]

Variable endLine is not correctly determined in styleDeclarationTextRange() in Source/WebInspectorUI/UserInterface/Models/CSSProperty.js as it's subtracting startLine from endLine (it should subtract endLine).

[Tomas Popela]

Created attachment 303063 [details]
Patch

[BJ Burg]

Comment on attachment 303063 [details]
Patch

Do we have any test coverage of this? How did you notice that this was wrong in the first place?

[Tomas Popela]

(In reply to comment #2)
> Do we have any test coverage of this? How did you notice that this was wrong
> in the first place?

It was detected by Coverity scan:

1. Defect type: COPY_PASTE_ERROR 
1. webkitgtk-2.14.5/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:237: original: "styleTextRange.startLine" looks like the original copy.
2. webkitgtk-2.14.5/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:238: copy_paste_error: "startLine" in "styleTextRange.startLine" looks like a copy-paste error.
3. webkitgtk-2.14.5/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:238: remediation: Should it say "endLine" instead?
#   236|   
#   237|           var startLine = this._styleSheetTextRange.startLine - styleTextRange.startLine;
#   238|->         var endLine = this._styleSheetTextRange.endLine - styleTextRange.startLine;
#   239|   
#   240|           var startColumn = this._styleSheetTextRange.startColumn;

[Joseph Pecoraro]

Comment on attachment 303063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=303063&action=review

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:212
>          var startLine = this._styleSheetTextRange.startLine - styleTextRange.startLine;
> -        var endLine = this._styleSheetTextRange.endLine - styleTextRange.startLine;
> +        var endLine = this._styleSheetTextRange.endLine - styleTextRange.endLine;

This doesn't feel correct to me. I think styleTextRange.startLine is essentially a starting offset, and we would be subtracting this starting offset from both the values to be consistent. But I could be wrong. The important thing is there should be a test for this to prove its correctness. LayoutTests/inspector/css/css-property.html could be expanded to test styleDeclarationTextRange.

[Joseph Pecoraro]

Comment on attachment 303063 [details]
Patch

r- until there is a test