Bug 186259 – [MotionMark] Strip out more characters when creating permalinks

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Jon Lee]

ssia

[Jon Lee]

Created attachment 341892 [details]
Patch

[Said Abou-Hallawa]

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.

[Jon Lee]

(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]

[EWS Watchlist]

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

[EWS Watchlist]

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

[Darin Adler]

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, '').

[Jon Lee]

Created attachment 342054 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 342054 [details]
Patch

Clearing flags on attachment: 342054

Committed r232547: <https://trac.webkit.org/changeset/232547>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/40857247>