[webkit-reviews] review granted: [Bug 129458] Web Inspector: Find in source does not find all results with non-unix line endings : [Attachment 225424] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Timothy Hatcher
<timothy at apple.com>'s request for review:
Bug 129458: Web Inspector: Find in source does not find all results with
non-unix line endings
https://bugs.webkit.org/show_bug.cgi?id=129458

Attachment 225424: Patch
https://bugs.webkit.org/attachment.cgi?id=225424&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
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"