Bug 55685

Summary: Web Inspector: [Text editor] Do bisect to find visible chunks
Product: WebKit Reporter: Andrey Adaikin <aandrey>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, podivilov, rik, timothy, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch none

Description Andrey Adaikin 2011-03-03 09:35:57 PST
We should do a bisect to find chunks in the visible area instead of a linear search.
Comment 1 Andrey Adaikin 2011-03-03 09:40:17 PST
Created attachment 84576 [details]
Patch
Comment 2 Pavel Feldman 2011-03-04 02:01:17 PST
Comment on attachment 84576 [details]
Patch

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

> Source/WebCore/inspector/front-end/TextViewer.js:408
> +    _findVisibleChunks: function(visibleFrom, visibleTo)

You should use insertionIndexForObjectInListSortedByFunction from utilities.js (and you can rename it to binarySearch).

> Source/WebCore/inspector/front-end/TextViewer.js:661
> +        return this._expandedLineRows ? this._expandedLineRows[0].offsetTop : this.element.offsetTop;

What does this logic do?

> Source/WebCore/inspector/front-end/TextViewer.js:1580
> +        return this._expandedLineRows ? this._expandedLineRows[0].offsetTop : this.element.offsetTop;

ditto. Also, I see that gutter chunk and main chunk have a lot in common. This does not sound right.
Comment 3 Andrey Adaikin 2011-03-04 05:31:45 PST
Comment on attachment 84576 [details]
Patch

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

>> Source/WebCore/inspector/front-end/TextViewer.js:408
>> +    _findVisibleChunks: function(visibleFrom, visibleTo)
> 
> You should use insertionIndexForObjectInListSortedByFunction from utilities.js (and you can rename it to binarySearch).

done

>> Source/WebCore/inspector/front-end/TextViewer.js:661
>> +        return this._expandedLineRows ? this._expandedLineRows[0].offsetTop : this.element.offsetTop;
> 
> What does this logic do?

this calculates offsetTop of the chunk, that is offsetTop of the first DOM node controlled by the chunk

>> Source/WebCore/inspector/front-end/TextViewer.js:1580
>> +        return this._expandedLineRows ? this._expandedLineRows[0].offsetTop : this.element.offsetTop;
> 
> ditto. Also, I see that gutter chunk and main chunk have a lot in common. This does not sound right.

there is even more code duplication now. I am planning to address this when doing the redesign with a ChunkedModel (as we discussed earlier)
Comment 4 Andrey Adaikin 2011-03-04 05:36:07 PST
Created attachment 84731 [details]
Patch
Comment 5 Pavel Podivilov 2011-03-04 06:41:09 PST
Comment on attachment 84731 [details]
Patch

Clearing flags on attachment: 84731

Committed r80347: <http://trac.webkit.org/changeset/80347>
Comment 6 Pavel Podivilov 2011-03-04 06:41:18 PST
All reviewed patches have been landed.  Closing bug.