Replacing PHP with equivalent Python CGI scripts
<rdar://problem/74207513>
Created attachment 419932 [details] Patch
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.
Created attachment 420134 [details] Patch
(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
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.
Created attachment 420159 [details] Patch
(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.
Committed r272812: <https://commits.webkit.org/r272812> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420159 [details].