Bug 107433 – Web Inspector: fix highlight bug in DTE.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Lushnikov]

Sometimes a part of a line is not highlighted at all in DTE (see screenshot).

[Andrey Lushnikov]

Created attachment 183740 [details]
broken highlight screenshot

[Andrey Lushnikov]

Created attachment 183742 [details]
Patch

[Pavel Feldman]

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

[Andrey Lushnikov]

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 || []";

[Pavel Feldman]

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!

[Andrey Lushnikov]

Created attachment 183751 [details]
Patch

[WebKit Review Bot]

Comment on attachment 183751 [details]
Patch

Clearing flags on attachment: 183751

Committed r140320: <http://trac.webkit.org/changeset/140320>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.