Bug 129458 – Web Inspector: Find in source does not find all results with non-unix line endings

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Timothy Hatcher]

ContentSearchUtilities lineEndings only supports \n. That causes issues with non-unix line endings since the front-end supports \n, \r or \r\n

<rdar://problem/15829887>

[Radar WebKit Bug Importer]

<rdar://problem/16190305>

[Radar WebKit Bug Importer]

<rdar://problem/16190323>

[Timothy Hatcher]

Created attachment 225424 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 225424 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:3
> +        Imporve how ContentSearchUtilities::lineEndings works by supporting the three common line endings.

Typo: Imporve

> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:91
>          size_t lineEnd = endings->at(lineNumber);
>          String line = text.substring(start, lineEnd - start);

These variable names are misleading. endings->at(lineNumber) is actually giving you the index of the first character on lineNumber+1. Maybe that the way I think about it. I would expect "lineEnd" to be a character on this line, but here it is not.

> Source/JavaScriptCore/inspector/ContentSearchUtilities.cpp:116
>  {
>      auto result = std::make_unique<Vector<size_t>>();
>  
> -    unsigned start = 0;
> +    size_t start = 0;
>      while (start < text.length()) {
> -        size_t lineEnd = text.find('\n', start);
> -        if (lineEnd == notFound)
> +        size_t nextStart = text.findNextLineStart(start);
> +        if (nextStart == notFound) {
> +            result->append(text.length());
>              break;
> +        }
>  
> -        result->append(lineEnd);
> -        start = lineEnd + 1;
> +        result->append(nextStart);
> +        start = nextStart;

Alongside the earlier comment, the function is named "lineEndings" but it returns the indexes that start the next line / EOF.

I'm fine with the patch as is, but it might be clearer with better names.

> LayoutTests/inspector-protocol/debugger/searchInContent-linebreaks.html:65
> +Verify that Debugger.searchInContent works with scripts that have varying  line endings.

Nit: Double space "varying  line endings"

[Timothy Hatcher]

Created attachment 225426 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 225426 [details]
Patch

Clearing flags on attachment: 225426

Committed r164845: <http://trac.webkit.org/changeset/164845>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.