Bug 169032 - Variable endLine is not correctly determined in styleDeclarationTextRange()
Summary: Variable endLine is not correctly determined in styleDeclarationTextRange()
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tomas Popela
URL:
Keywords:
Depends on:
Blocks: 104114
  Show dependency tree / graph
 
Reported: 2017-03-01 04:57 PST by Tomas Popela
Modified: 2017-03-13 01:18 PDT (History)
2 users (show)

See Also:

Attachments
Patch (1.69 KB, patch)
2017-03-01 05:00 PST, Tomas Popela 		joepeck: review-
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Popela 2017-03-01 04:57:50 PST 
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).
Comment 1 Tomas Popela 2017-03-01 05:00:17 PST 
Created attachment 303063 [details]
Patch
Comment 2 BJ Burg 2017-03-01 08:18:35 PST 
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?
Comment 3 Tomas Popela 2017-03-02 01:03:56 PST 
(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;
Comment 4 Joseph Pecoraro 2017-03-02 11:46:27 PST 
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.
Comment 5 Joseph Pecoraro 2017-03-02 11:47:00 PST 
Comment on attachment 303063 [details]
Patch

r- until there is a test