Bug 110732 – Web Inspector: Selector's raw start position in its line is considered to be 0 when computing UILocation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Alexander Pavlov (apavlov)]

Upstreaming https://code.google.com/p/chromium/issues/detail?id=177976

[Alexander Pavlov (apavlov)]

Created attachment 190046 [details]
Patch

[Pavel Feldman]

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

[Alexander Pavlov (apavlov)]

Created attachment 190212 [details]
Patch

[Vsevolod Vlasov]

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());

[Vsevolod Vlasov]

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?

[Vsevolod Vlasov]

Comment on attachment 190212 [details]
Patch

r- per my comment about complexity of lineColumnFromOffset implementation.

[Alexander Pavlov (apavlov)]

Created attachment 190690 [details]
Patch

[Timothy Hatcher]

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?

[Alexander Pavlov (apavlov)]

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.

[Alexander Pavlov (apavlov)]

Created attachment 190729 [details]
Patch

[Vsevolod Vlasov]

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

[Alexander Pavlov (apavlov)]

Committed r144434: <http://trac.webkit.org/changeset/144434>