Bug 236818 - Add percent decoding to Scroll to Text Fragment parsing.
Summary: Add percent decoding to Scroll to Text Fragment parsing.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-02-17 17:32 PST by Megan Gardner
Modified: 2022-09-12 13:41 PDT (History)
10 users (show)

See Also:


Attachments
Patch (5.69 KB, patch)
2022-02-17 17:34 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (12.47 KB, patch)
2022-02-18 13:34 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (11.86 KB, patch)
2022-02-18 14:07 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (11.81 KB, patch)
2022-02-18 14:36 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (11.79 KB, patch)
2022-02-18 16:04 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (11.79 KB, patch)
2022-02-22 10:19 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (14.56 KB, patch)
2022-02-24 09:58 PST, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch for landing (15.14 KB, patch)
2022-02-24 12:01 PST, Megan Gardner
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2022-02-17 17:32:06 PST
Add percent decoding to Scroll to Text Fragment parsing.
Comment 1 Megan Gardner 2022-02-17 17:34:57 PST
Created attachment 452453 [details]
Patch
Comment 2 Alex Christensen 2022-02-17 17:45:02 PST
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.
Comment 3 Megan Gardner 2022-02-18 13:34:41 PST
Created attachment 452575 [details]
Patch
Comment 4 Chris Dumez 2022-02-18 13:38:39 PST
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?
Comment 5 Chris Dumez 2022-02-18 13:41:36 PST
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.
Comment 6 Chris Dumez 2022-02-18 13:42:35 PST
Comment on attachment 452575 [details]
Patch

r- because of Vector::takeFirst() (which doesn't seem needed)
Comment 7 Megan Gardner 2022-02-18 14:07:29 PST
Created attachment 452579 [details]
Patch
Comment 8 Chris Dumez 2022-02-18 14:10:41 PST
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.
Comment 9 Megan Gardner 2022-02-18 14:36:44 PST
Created attachment 452584 [details]
Patch
Comment 10 Chris Dumez 2022-02-18 16:02:28 PST
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.
Comment 11 Megan Gardner 2022-02-18 16:04:25 PST
Created attachment 452595 [details]
Patch for landing
Comment 12 EWS 2022-02-18 19:28:49 PST
Found 1 new test failure: http/wpt/push-api/onpush-disabled.html
Comment 13 Megan Gardner 2022-02-22 10:19:49 PST
Created attachment 452883 [details]
Patch
Comment 14 Megan Gardner 2022-02-24 09:58:02 PST
Created attachment 453113 [details]
Patch
Comment 15 Megan Gardner 2022-02-24 12:01:38 PST
Created attachment 453122 [details]
Patch for landing
Comment 16 EWS 2022-02-24 13:15:46 PST
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].
Comment 17 Radar WebKit Bug Importer 2022-02-24 13:16:22 PST
<rdar://problem/89435775>