[webkit-reviews] review granted: [Bug 71494] Remove deprecated free functions in port.factory : [Attachment 113540] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Eric Seidel <eric at webkit.org>'s
request for review:
Bug 71494: Remove deprecated free functions in port.factory
https://bugs.webkit.org/show_bug.cgi?id=71494

Attachment 113540: Patch
https://bugs.webkit.org/attachment.cgi?id=113540&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=113540&action=review


Seems fine.  I'm slightly worried this is going to cause tests to fail on
Windows because of the branch in engage_windows_hacks.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_win.py:104
> +	   if sys.platform != "win32":
> +	       return

This isn't actually as bad as it seems.  We feature-detect whether the hacks
are needed.  If they're not needed, we won't do anything.

should we add a static boolean for whether we've tried to engage the hacks
before?  We shouldn't need to try to engage them twice.

> Tools/Scripts/webkitpy/layout_tests/port/test.py:278
> +    def _set_default_overriding_none(self, dictionary, key, default):
> +	   # dict.setdefault almost works, but won't actually override None
values, which we want.
> +	   if not dictionary.get(key):
> +	       dictionary[key] = default
> +	   return dictionary[key]

This code looks duplicated.

> Tools/Scripts/webkitpy/layout_tests/port/win.py:62
> +	   # No sense in trying to detect our windows version on non-windows
platforms, unless we're unittesting.
> +	   if sys.platform != 'cygwin' and not run_on_non_windows_platforms:

Lame.