Bug 107433 - Web Inspector: fix highlight bug in DTE.
Summary: Web Inspector: fix highlight bug in DTE.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Lushnikov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-21 02:16 PST by Andrey Lushnikov
Modified: 2013-01-21 03:55 PST (History)
9 users (show)

See Also:


Attachments
broken highlight screenshot (24.24 KB, image/png)
2013-01-21 02:17 PST, Andrey Lushnikov
no flags Details
Patch (7.97 KB, patch)
2013-01-21 02:35 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (7.60 KB, patch)
2013-01-21 03:37 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Lushnikov 2013-01-21 02:16:05 PST
Sometimes a part of a line is not highlighted at all in DTE (see screenshot).
Comment 1 Andrey Lushnikov 2013-01-21 02:17:25 PST
Created attachment 183740 [details]
broken highlight screenshot
Comment 2 Andrey Lushnikov 2013-01-21 02:35:17 PST
Created attachment 183742 [details]
Patch
Comment 3 Pavel Feldman 2013-01-21 03:04:35 PST
Comment on attachment 183742 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Fix highlight bug related to highlightChunkLimit in DefaultTextEditor.

You should explain what has changed and why

> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:188
> +                } else {

What if there is no midConditionStringified? Will you hit NPE at state.ranges.push?

no {} around single line blocks. Why creating ranges early
Comment 4 Andrey Lushnikov 2013-01-21 03:26:41 PST
Comment on attachment 183742 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:188
>> +                } else {
> 
> What if there is no midConditionStringified? Will you hit NPE at state.ranges.push?
> 
> no {} around single line blocks. Why creating ranges early

Let's go through the following cases:
1. midConditionStringified and postConditionStringified both do not exist
In this case, we're going to highlight the line from the scratch, and in this case we will create an empty state.ranges 
array.

2. postConditionStringified does not exist, but midConditionStringified exists. 
In this case, we don't want to re-create that array, because it contains highlight information.

What is important is that the first case will always be hit before the second case (due to code logic). 

Addressing the NPE threat you've mentioned, it might be more robust to write smth like "state.ranges = state.ranges || []";
Comment 5 Pavel Feldman 2013-01-21 03:31:12 PST
Comment on attachment 183742 [details]
Patch

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

>>> Source/WebCore/inspector/front-end/TextEditorHighlighter.js:188
>>> +                } else {
>> 
>> What if there is no midConditionStringified? Will you hit NPE at state.ranges.push?
>> 
>> no {} around single line blocks. Why creating ranges early
> 
> Let's go through the following cases:
> 1. midConditionStringified and postConditionStringified both do not exist
> In this case, we're going to highlight the line from the scratch, and in this case we will create an empty state.ranges 
> array.
> 
> 2. postConditionStringified does not exist, but midConditionStringified exists. 
> In this case, we don't want to re-create that array, because it contains highlight information.
> 
> What is important is that the first case will always be hit before the second case (due to code logic). 
> 
> Addressing the NPE threat you've mentioned, it might be more robust to write smth like "state.ranges = state.ranges || []";

>> state.ranges = state.ranges || []

Yep, I would have no problems understanding this one!
Comment 6 Andrey Lushnikov 2013-01-21 03:37:33 PST
Created attachment 183751 [details]
Patch
Comment 7 WebKit Review Bot 2013-01-21 03:55:28 PST
Comment on attachment 183751 [details]
Patch

Clearing flags on attachment: 183751

Committed r140320: <http://trac.webkit.org/changeset/140320>
Comment 8 WebKit Review Bot 2013-01-21 03:55:32 PST
All reviewed patches have been landed.  Closing bug.