Bug 221720 – [LayoutTests] Convert http/tests/storageAccess convert PHP to Python

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Chris Gambrell]

Replacing PHP with equivalent Python CGI scripts

[Radar WebKit Bug Importer]

<rdar://problem/74207513>

[Chris Gambrell]

Created attachment 419932 [details]
Patch

[Jonathan Bedard]

Comment on attachment 419932 [details]
Patch

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

> LayoutTests/http/tests/storageAccess/resources/echo-incoming-cookies-as-json.py:8
> +if 'HTTP_COOKIE' in os.environ:

I would do this instead:

header_cookies = os.environ.get('HTTP_COOKIE')
header_cookies = header_cookies.split('; ') if header_cookies else []

then you can take all of this out of the if statement

> LayoutTests/http/tests/storageAccess/resources/get-cookies.py:31
> +if not cname1:

Readability suggestion here: If you made the default for name1 the empty string (like it is now), but the defaults of name2 and name3 None, you could unify this code into a single loop:

for name in [
    query.get('name1', [''])[0],
    query.get('name2', [None])[0],
    query.get('name3', [None])[0],
]:
    if name is not None:
        continue
    cookie = cookies.get(name)
    if cookie:
        print(...)
    else:
        print(...)

> LayoutTests/http/tests/storageAccess/resources/get-cookies.py:48
> +print('<p id="output"></p>')

I think this is a good use-case for a multi-line string (ie, ''' <content> '''

> LayoutTests/http/tests/storageAccess/resources/set-cookie.py:20
> +print('<script>')

Another code case for a multi-line string.

[Chris Gambrell]

Created attachment 420134 [details]
Patch

[Chris Gambrell]

(In reply to Jonathan Bedard from comment #3)
> Comment on attachment 419932 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419932&action=review
> 
> > LayoutTests/http/tests/storageAccess/resources/echo-incoming-cookies-as-json.py:8
> > +if 'HTTP_COOKIE' in os.environ:
> 
> I would do this instead:
> 
> header_cookies = os.environ.get('HTTP_COOKIE')
> header_cookies = header_cookies.split('; ') if header_cookies else []
> 
> then you can take all of this out of the if statement
> 

Like this change, very concise compared to my block


> > LayoutTests/http/tests/storageAccess/resources/get-cookies.py:31
> > +if not cname1:
> 
> Readability suggestion here: If you made the default for name1 the empty
> string (like it is now), but the defaults of name2 and name3 None, you could
> unify this code into a single loop:
> 
> for name in [
>     query.get('name1', [''])[0],
>     query.get('name2', [None])[0],
>     query.get('name3', [None])[0],
> ]:
>     if name is not None:
>         continue
>     cookie = cookies.get(name)
>     if cookie:
>         print(...)
>     else:
>         print(...)
> 

Implemented this change


> > LayoutTests/http/tests/storageAccess/resources/get-cookies.py:48
> > +print('<p id="output"></p>')
> 
> I think this is a good use-case for a multi-line string (ie, ''' <content>
> '''
> 
> > LayoutTests/http/tests/storageAccess/resources/set-cookie.py:20
> > +print('<script>')
> 
> Another code case for a multi-line string.

I like these stylistic changes quite a bit. It makes the large blocks of content much cleaner

[Jonathan Bedard]

Comment on attachment 420134 [details]
Patch

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

> LayoutTests/http/tests/storageAccess/resources/get-cookies.py:29
> +        print('Received cookie named \'{}\'.<br>'.format(name), end='')

Looks like the code I have you wasn't 100% correct, this seems to be what's causing the test failure EWS is flagging.

[Chris Gambrell]

Created attachment 420159 [details]
Patch

[Chris Gambrell]

(In reply to Jonathan Bedard from comment #6)
> Comment on attachment 420134 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420134&action=review
> 
> > LayoutTests/http/tests/storageAccess/resources/get-cookies.py:29
> > +        print('Received cookie named \'{}\'.<br>'.format(name), end='')
> 
> Looks like the code I have you wasn't 100% correct, this seems to be what's
> causing the test failure EWS is flagging.

The for loop in the cookie header separation is still required...

for cookie in header_cookies:
        cookie = cookie.split('=')
        cookies[cookie[0]] = cookie[1]

...due to needing to separate...

['foo=bar', 'hello=world']

...into...

{
    'foo': 'bar',
    'hello: 'world'
}

Fixed that in latest patch.

[EWS]

Committed r272812: <https://commits.webkit.org/r272812>

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