Summary: | Web Inspector: Find in source does not find all results with non-unix line endings | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Timothy Hatcher <timothy> | ||||||
Component: | Web Inspector | Assignee: | Timothy Hatcher <timothy> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, graouts, joepeck, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Timothy Hatcher
2014-02-27 15:54:20 PST
Created attachment 225424 [details]
Patch
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" Created attachment 225426 [details]
Patch
Comment on attachment 225426 [details] Patch Clearing flags on attachment: 225426 Committed r164845: <http://trac.webkit.org/changeset/164845> All reviewed patches have been landed. Closing bug. |