Bug 186259 - [MotionMark] Strip out more characters when creating permalinks
Summary: [MotionMark] Strip out more characters when creating permalinks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jon Lee
URL:
Keywords: InRadar
Depends on: 186257
Blocks: 186260
  Show dependency treegraph
 
Reported: 2018-06-03 22:09 PDT by Jon Lee
Modified: 2018-06-06 10:52 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.16 KB, patch)
2018-06-03 22:12 PDT, Jon Lee
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.73 MB, application/zip)
2018-06-04 15:40 PDT, EWS Watchlist
no flags Details
Patch (4.21 KB, patch)
2018-06-06 09:50 PDT, Jon Lee
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jon Lee 2018-06-03 22:09:02 PDT
ssia
Comment 1 Jon Lee 2018-06-03 22:12:30 PDT
Created attachment 341892 [details]
Patch
Comment 2 Said Abou-Hallawa 2018-06-04 12:45:14 PDT
Comment on attachment 341892 [details]
Patch

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

> PerformanceTests/MotionMark/resources/extensions.js:153
>      stripNonASCIICharacters: function(inputString)
>      {
> -        return inputString.replace(/[ .,]/g, '');
> +        return inputString.replace(/[ .,\-\+\(\)]/g, '');
>      },

The name of this function is misleading. What is the actual purpose of this function?

The ASCII UTF-7 range is 0-127 and the ASCII UTF-8 range is 0-255. But this function just removes some special characters from the string. For example it removes the parentheses but it leaves the brackets and the curly braces. Also it is not clear why we want these characters only to be removed from the link now.
Comment 3 Jon Lee 2018-06-04 13:39:52 PDT
(In reply to Said Abou-Hallawa from comment #2)
> Comment on attachment 341892 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341892&action=review
> 
> > PerformanceTests/MotionMark/resources/extensions.js:153
> >      stripNonASCIICharacters: function(inputString)
> >      {
> > -        return inputString.replace(/[ .,]/g, '');
> > +        return inputString.replace(/[ .,\-\+\(\)]/g, '');
> >      },
> 
> The name of this function is misleading. What is the actual purpose of this
> function?

This could be renamed stripNonAlphanumericCharacters. It's just to avoid having test names being URI encoded to something less legible.

> 
> The ASCII UTF-7 range is 0-127 and the ASCII UTF-8 range is 0-255. But this
> function just removes some special characters from the string. For example
> it removes the parentheses but it leaves the brackets and the curly braces.
> Also it is not clear why we want these characters only to be removed from
> the link now.

Maybe I should redefine it to be [^A-Za-z0-9]
Comment 4 EWS Watchlist 2018-06-04 15:39:50 PDT
Comment on attachment 341892 [details]
Patch

Attachment 341892 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7982897

New failing tests:
http/tests/security/contentSecurityPolicy/video-with-https-url-allowed-by-csp-media-src-star.html
Comment 5 EWS Watchlist 2018-06-04 15:40:01 PDT
Created attachment 341925 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 6 Darin Adler 2018-06-05 14:51:21 PDT
Comment on attachment 341892 [details]
Patch

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

>>> PerformanceTests/MotionMark/resources/extensions.js:153
>>>      },
>> 
>> The name of this function is misleading. What is the actual purpose of this function?
>> 
>> The ASCII UTF-7 range is 0-127 and the ASCII UTF-8 range is 0-255. But this function just removes some special characters from the string. For example it removes the parentheses but it leaves the brackets and the curly braces. Also it is not clear why we want these characters only to be removed from the link now.
> 
> This could be renamed stripNonAlphanumericCharacters. It's just to avoid having test names being URI encoded to something less legible.

It could be renamed stripUnwantedCharactersForURL.

Or if you want it to be stripNonAlphanumericCharacters then it could be replace(/\W/g, '').
Comment 7 Jon Lee 2018-06-06 09:50:59 PDT
Created attachment 342054 [details]
Patch
Comment 8 WebKit Commit Bot 2018-06-06 10:40:47 PDT
Comment on attachment 342054 [details]
Patch

Clearing flags on attachment: 342054

Committed r232547: <https://trac.webkit.org/changeset/232547>
Comment 9 WebKit Commit Bot 2018-06-06 10:40:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Radar WebKit Bug Importer 2018-06-06 10:42:09 PDT
<rdar://problem/40857247>