Bug 55685 - Web Inspector: [Text editor] Do bisect to find visible chunks
Summary: Web Inspector: [Text editor] Do bisect to find visible chunks
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: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-03 09:35 PST by Andrey Adaikin
Modified: 2011-03-04 06:41 PST (History)
11 users (show)

See Also:

Attachments
Patch (5.02 KB, patch)
2011-03-03 09:40 PST, Andrey Adaikin 		no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2011-03-04 05:36 PST, Andrey Adaikin 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) 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 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.