Bug 78550 – Implement enough of ChromiumAndroidPort to make ChromiumAndroidPortTest pass

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

[Adam Barth]

Implement enough of ChromiumAndroidPort to make ChromiumAndroidPortTest pass

[Adam Barth]

Created attachment 126865 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 126865 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:154
> +    def _build_path(self, *comps):
> +        return self._host_port._build_path(*comps)
> +
> +    def _path_to_apache(self):
> +        return self._host_port._path_to_apache()
> +
> +    def _path_to_apache_config_file(self):
> +        return self._host_port._path_to_apache_config_file()

I suspect we'll eventually want some sort of HostedPort.

[Adam Barth]

> I suspect we'll eventually want some sort of HostedPort.

Yes, especially if other folks want to cross-compile (e.g., iOS).

[Adam Barth]

Committed r107637: <http://trac.webkit.org/changeset/107637>

[Eric Seidel (no email)]

+ddkilzer so he can see the HostedPort comment. :)

[Dirk Pranke]

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:94
> +        self._adb_command = ['adb']

what's this used for? Did this creep in accidentally?

> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:55
> +    default_worker_model = 'processes'

Nit: maybe rename this to expected_worker_model to reduce confusion with the port method of the same name and make the purpose clearer?

> Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:61
> +            host.port_factory = factory.PortFactory(host)

Nit: I don't feel real warm and fuzzy about the polyfilling of the host object here ... in addition to the general shadiness of it, it might make it easier to write code in the actual port implementations that make it look like we have a full Host and not a SystemHost; I prefer the approach I used in mock_drt.py where we create a PortFactory(host) instead where it is needed.

Then again, I've never felt great about the fact that the Host has the port_factory method ...

[Adam Barth]

(In reply to comment #6)
> View in context: https://bugs.webkit.org/attachment.cgi?id=126865&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_android.py:94
> > +        self._adb_command = ['adb']
> 
> what's this used for? Did this creep in accidentally?

It will be used in the next patch.  It's used to communicate with the device.

> > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:55
> > +    default_worker_model = 'processes'
> 
> Nit: maybe rename this to expected_worker_model to reduce confusion with the port method of the same name and make the purpose clearer?

Will do.

> > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:61
> > +            host.port_factory = factory.PortFactory(host)
> 
> Nit: I don't feel real warm and fuzzy about the polyfilling of the host object here ... in addition to the general shadiness of it, it might make it easier to write code in the actual port implementations that make it look like we have a full Host and not a SystemHost; I prefer the approach I used in mock_drt.py where we create a PortFactory(host) instead where it is needed.

Will do.

Thanks.