Bug 236818 – Add percent decoding to Scroll to Text Fragment parsing.

• Component: New Bugs
• Status: RESOLVED FIXED

[Megan Gardner]

Add percent decoding to Scroll to Text Fragment parsing.

[Megan Gardner]

Created attachment 452453 [details]
Patch

[Alex Christensen]

Comment on attachment 452453 [details]
Patch

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

Needs tests.

> Source/WTF/wtf/Vector.h:753
> +        removeFirst();

This is a very slow operation in a Vector.  takeLast is O(1) but this is O(n).  If you need this, use a Deque, but you probably don't.  It looks like you're already using a Deque.

> Source/WebCore/dom/FragmentDirectiveParser.cpp:106
> +            if (prefix)

nit: if (auto prefix = ...)
Then it's in the scope of the if statement.

[Megan Gardner]

Created attachment 452575 [details]
Patch

[Chris Dumez]

Comment on attachment 452575 [details]
Patch

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

> Source/WTF/wtf/Vector.h:750
> +    // Very slow, should not be used in common code paths or vectors of large N values.

Very slow, exactly so let's not do that. This is what Deque is for.

Also, where is this used in your patch?

> Source/WebCore/dom/FragmentDirectiveParser.cpp:106
> +            if (auto prefix = WTF::URLParser::formURLDecode(tokens.takeFirst()))

You are calling takeFirst() here but tokens seems to be a Deque, not a Vector?

[Chris Dumez]

Comment on attachment 452575 [details]
Patch

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

> Source/WebCore/dom/FragmentDirectiveParser.cpp:107
> +                parsedTextDirective.prefix = prefix.value();

= WTFMove(*prefix);

> Source/WebCore/dom/FragmentDirectiveParser.cpp:116
> +                parsedTextDirective.suffix = suffix.value();

= WTFMove(*suffix);

> Source/WebCore/dom/FragmentDirectiveParser.cpp:127
> +            parsedTextDirective.textStart = start.value();

ditto.

> Source/WebCore/dom/FragmentDirectiveParser.cpp:134
> +                parsedTextDirective.textEnd = end.value();

ditto.

[Chris Dumez]

Comment on attachment 452575 [details]
Patch

r- because of Vector::takeFirst() (which doesn't seem needed)

[Megan Gardner]

Created attachment 452579 [details]
Patch

[Chris Dumez]

Comment on attachment 452579 [details]
Patch

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

> Source/WTF/ChangeLog:12
> +        (WTF::Vector::takeFirst):

nit: Still here.

> Source/WebCore/dom/FragmentDirectiveParser.cpp:107
> +                parsedTextDirective.prefix = prefix.value();

Didn't WTFMove(*prefix) work? Seems this iteration didn't take this part of the feedback into account.

Same comment applies below.

[Megan Gardner]

Created attachment 452584 [details]
Patch

[Chris Dumez]

Comment on attachment 452584 [details]
Patch

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

r=me

> Source/WebCore/dom/FragmentDirectiveParser.cpp:132
> +            

nit: Weird extra line.

[Megan Gardner]

Created attachment 452595 [details]
Patch for landing

[EWS]

Found 1 new test failure: http/wpt/push-api/onpush-disabled.html

[Megan Gardner]

Created attachment 452883 [details]
Patch

[Megan Gardner]

Created attachment 453113 [details]
Patch

[Megan Gardner]

Created attachment 453122 [details]
Patch for landing

[EWS]

Committed r290455 (247756@main): <https://commits.webkit.org/247756@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 453122 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89435775>