Bug 202465

Created attachment 380783 [details] Patch

Comment on attachment 380783 [details] Patch Attachment 380783 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13121916 New failing tests: http/tests/is-logged-in/available-in-secure-contexts.https.html http/tests/websocket/tests/hybi/non-document-mixed-content-blocked-https-with-embedded-http-with-embedded-https.https.html http/tests/security/navigate-when-restoring-cached-page.html

Created attachment 380808 [details] Archive of layout-test-results from ews214 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Comment on attachment 380783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380783&action=review Looks good other than a few small suggestions. Unofficial r+. > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:423 > + attachment['is_obsolete'] = (element.has_attr('isobsolete') and element['isobsolete'] == "1") This is probably cleaner (if it works in Python3): attachment['is_obsolete'] = getattr(element, 'isobsolete', "0") == "1" > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:424 > + attachment['is_patch'] = (element.has_attr('ispatch') and element['ispatch'] == "1") Ditto, but with `ispatch`. > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:426 > + attachment['is_obsolete'] = (element.has_key('isobsolete') and element['isobsolete'] == "1") Ditto. > Tools/Scripts/webkitpy/common/net/bugzilla/bugzilla.py:427 > + attachment['is_patch'] = (element.has_key('ispatch') and element['ispatch'] == "1") Ditto. > Tools/Scripts/webkitpy/thirdparty/__init__.py:211 > + if self._install("https://files.pythonhosted.org/packages/86/cd/495c68f0536dcd25f016e006731ba7be72e072280305ec52590012c1e6f2/beautifulsoup4-4.8.1.tar.gz", Can you assign the value of this to a variable, then check the value for the if check? I think it would read better.

Comment on attachment 380783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380783&action=review > Tools/Scripts/webkitpy/common/net/file_uploader.py:35 > +if sys.version_info > (3, 0): Python has 3.0 release, do we want to use '>=' here instead? Also, since we are repeating this a lot, maybe it's easier to make a helper function? > Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:43 > +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup Is there any specific reason to swap the import order? Or just make it follow the case insensitive alphabetical order? > Tools/Scripts/webkitpy/thirdparty/BeautifulSoup.py:30 > + # This hack isn't strictly accurate, but getting lxml to work with the autoinstaller is a non-trivial task Is this a FIXME then? > Tools/Scripts/webkitpy/thirdparty/__init__.py:210 > + "soupsieve-1.9.4/soupsieve") Nit: this indentation doesn't match one style used in this file. > Tools/Scripts/webkitpy/thirdparty/__init__.py:213 > + self._executive.run_command(['2to3', '-w', self._fs.join(_AUTOINSTALLED_DIR, 'bs4')]) Ditto

r=me according to the fact that no test failure occurrs because of this change for both python 2&3.

Comment on attachment 380783 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380783&action=review >> Tools/Scripts/webkitpy/common/net/file_uploader.py:35 >> +if sys.version_info > (3, 0): > > Python has 3.0 release, do we want to use '>=' here instead? > Also, since we are repeating this a lot, maybe it's easier to make a helper function? We discussed this in the last patch, basically, our helper function would not be any shorter, so we're better off using the standard library. >> Tools/Scripts/webkitpy/common/net/buildbot/buildbot.py:43 >> +from webkitpy.thirdparty.BeautifulSoup import BeautifulSoup > > Is there any specific reason to swap the import order? Or just make it follow the case insensitive alphabetical order? Just making it follow the case-insensitive alphabetical order. >> Tools/Scripts/webkitpy/thirdparty/BeautifulSoup.py:30 >> + # This hack isn't strictly accurate, but getting lxml to work with the autoinstaller is a non-trivial task > > Is this a FIXME then? Yeah, I can make it a FIXME

Created attachment 380938 [details] Patch for landing

Comment on attachment 380938 [details] Patch for landing Clearing flags on attachment: 380938 Committed r251112: <https://trac.webkit.org/changeset/251112>

All reviewed patches have been landed. Closing bug.

<rdar://problem/56270611>