Summary: | Web Inspector: Selector's raw start position in its line is considered to be 0 when computing UILocation | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | apavlov, graouts, joepeck, keishi, loislo, m.goleb+bugzilla, pfeldman, pmuellr, timothy, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2013-02-25 00:53:58 PST
Created attachment 190046 [details]
Patch
Comment on attachment 190046 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190046&action=review > Source/WebCore/inspector/InspectorStyleSheet.cpp:149 > + ASSERT(m_hasText); Could you reuse the code in getRegularExpressionMatchesByLines? > Source/WebCore/inspector/InspectorStyleSheet.cpp:176 > +struct LineColumn { wtf/text/TextPosition.h > Source/WebCore/inspector/InspectorStyleSheet.cpp:191 > + const size_t vectorSize = lineEndings.size(); Move to ContentSearchUtil Created attachment 190212 [details]
Patch
Comment on attachment 190212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190212&action=review > Source/WebCore/inspector/ContentSearchUtils.cpp:74 > + if (!vectorSize) This check should not be needed, see my comment to lineEndings below. > Source/WebCore/inspector/ContentSearchUtils.cpp:79 > + size_t previousLineEndingOffset; const size_t* foundLineEnding = approximateBinarySearch<size_t, size_t>(lineEndings, vectorSize, offset, sizetExtractor); size_t lineIndex = foundLineEnding - &lineEndings.at(0); if (offset > *foundLineEnding) ++lineIndex; size_t previousLineEndingOffset = lineIndex >= 0 ? lineEndings.at(lineIndex-1) : 0; size_t column = offset - previousLineEndingOffset; return TextPosition(OrdinalNumber::fromZeroBasedInt(lineIndex), OrdinalNumber::fromZeroBasedInt(column)); Seems to be a shorter way to do the same. > Source/WebCore/inspector/ContentSearchUtils.cpp:128 > + while (start < text.length()) { I would do as follows instead to make sure we lways have at least one line (even for an empty file). That would be consistent with what we do in front-end (see utilities.js) while (start < text.length()) { size_t lineEnd = text.find('\n', start); if (lineEnd == notFound) break; result->append(lineEnd); start = lineEnd + 1; } result->append(text.length()); Comment on attachment 190212 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190212&action=review > Source/WebCore/inspector/InspectorStyleSheet.cpp:527 > + OwnPtr<Vector<size_t> > lineEndings(m_parentStyleSheet ? m_parentStyleSheet->lineEndings() : PassOwnPtr<Vector<size_t> >()); Could you please explain what is the expected value of SourceRange in case m_parentStyleSheet is null and hence lineEndings are not available? > Source/WebCore/inspector/front-end/CSSStyleModel.js:882 > + if (resource && resource.type === WebInspector.resourceTypes.Stylesheet) { Why is the resource type important here? Comment on attachment 190212 [details]
Patch
r- per my comment about complexity of lineColumnFromOffset implementation.
Created attachment 190690 [details]
Patch
Comment on attachment 190690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190690&action=review > Source/WebCore/inspector/ContentSearchUtils.cpp:71 > +TextPosition lineColumnFromOffset(size_t offset, const Vector<size_t>& lineEndings) textPositionFromOffset? Comment on attachment 190690 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190690&action=review >> Source/WebCore/inspector/ContentSearchUtils.cpp:71 >> +TextPosition lineColumnFromOffset(size_t offset, const Vector<size_t>& lineEndings) > > textPositionFromOffset? Makes sense, as long as it represents the line:column pair. Thanks. Created attachment 190729 [details]
Patch
Comment on attachment 190729 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190729&action=review > Source/WebCore/inspector/InspectorStyleSheet.cpp:64 > +#include <wtf/StdLibExtras.h> This include should go to ContentSearchUtils Committed r144434: <http://trac.webkit.org/changeset/144434> |