Bug 71494 – Remove deprecated free functions in port.factory

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Eric Seidel (no email)]

Remove deprecated free functions in port.factory

[Eric Seidel (no email)]

Created attachment 113540 [details]
Patch

[Adam Barth]

Comment on attachment 113540 [details]
Patch

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.

[Eric Seidel (no email)]

(In reply to comment #2)
> (From update of attachment 113540 [details])
> 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.

Well, we still execute a command, which is what was showing up in the unittests which instantiate a CrWinPort and have a logging executive.

> 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.

Yes, we should.  Probably a separate change.

> > 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.

It is.  Not sure where to put it.

> > 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.

Yeah, it is kinda lame.  Again, just trying to avoid running a command we don't need when we have a logging executive.

[Eric Seidel (no email)]

Created attachment 113542 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 113542 [details]
Patch for landing

Clearing flags on attachment: 113542

Committed r99233: <http://trac.webkit.org/changeset/99233>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

This patch regressed not needing python-irclib to run new-run-webkit-tests.  Previously, you could run new-run-webkit-tests without having the autoinstall need to download python-irclib.  Is it possible to get that behavior back?

[Adam Barth]

Very likely.  Is there a way we can test that property to make sure we don't regress it again?

[Eric Seidel (no email)]

Yes, it's very easy to add that behavior back.

[Tony Chang]

(In reply to comment #8)
> Very likely.  Is there a way we can test that property to make sure we don't regress it again?

Probably.  Feel free to file a bug against me to write a test for it.

[Csaba Osztrogonác]

It broke a webkitpy test on all port:

ERROR: test_get_host_port_object (webkitpy.to_be_moved.rebaseline_chromium_webkit_tests_unittest.TestGetHostPortObject)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests_unittest.py", line 181, in test_get_host_port_object
    self.assert_result(False, False, False)
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests_unittest.py", line 171, in assert_result
    port_obj = rebaseline_chromium_webkit_tests.get_host_port_object(port_factory, options)
  File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests.py", line 815, in get_host_port_object
    port_obj = port_factory.get(None, options)
TypeError: get() takes exactly 2 arguments (3 given)

Could you check it, please?

[Eric Seidel (no email)]

Certainly, one sec.

[Eric Seidel (no email)]

(In reply to comment #11)
> It broke a webkitpy test on all port:
> 
> ERROR: test_get_host_port_object (webkitpy.to_be_moved.rebaseline_chromium_webkit_tests_unittest.TestGetHostPortObject)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
>   File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests_unittest.py", line 181, in test_get_host_port_object
>     self.assert_result(False, False, False)
>   File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests_unittest.py", line 171, in assert_result
>     port_obj = rebaseline_chromium_webkit_tests.get_host_port_object(port_factory, options)
>   File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests.py", line 815, in get_host_port_object
>     port_obj = port_factory.get(None, options)
> TypeError: get() takes exactly 2 arguments (3 given)
> 
> Could you check it, please?

Sorry.  Fixed.

[Eric Seidel (no email)]

Committed r99271: <http://trac.webkit.org/changeset/99271>

[Eric Seidel (no email)]

(In reply to comment #9)
> Yes, it's very easy to add that behavior back.

Filed bug 71549.

[Eric Seidel (no email)]

Committed r99335: <http://trac.webkit.org/changeset/99335>