Bug 78550 - Implement enough of ChromiumAndroidPort to make ChromiumAndroidPortTest pass
Summary: Implement enough of ChromiumAndroidPort to make ChromiumAndroidPortTest pass
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 78524
  Show dependency treegraph
 
Reported: 2012-02-13 16:40 PST by Adam Barth
Modified: 2012-02-13 17:35 PST (History)
6 users (show)

See Also:


Attachments
Patch (7.07 KB, patch)
2012-02-13 16:41 PST, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2012-02-13 16:40:28 PST
Implement enough of ChromiumAndroidPort to make ChromiumAndroidPortTest pass
Comment 1 Adam Barth 2012-02-13 16:41:59 PST
Created attachment 126865 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-02-13 16:47:16 PST
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.
Comment 3 Adam Barth 2012-02-13 16:47:58 PST
> I suspect we'll eventually want some sort of HostedPort.

Yes, especially if other folks want to cross-compile (e.g., iOS).
Comment 4 Adam Barth 2012-02-13 16:48:21 PST
Committed r107637: <http://trac.webkit.org/changeset/107637>
Comment 5 Eric Seidel (no email) 2012-02-13 16:50:17 PST
+ddkilzer so he can see the HostedPort comment. :)
Comment 6 Dirk Pranke 2012-02-13 16:54:01 PST
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 ...
Comment 7 Adam Barth 2012-02-13 17:35:10 PST
(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.