[webkit-reviews] review denied: [Bug 46128] webkitpy - display web pages using a common routine : [Attachment 68168] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 46128: webkitpy - display web pages using a common routine
https://bugs.webkit.org/show_bug.cgi?id=46128

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

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

> WebKitTools/Scripts/webkitpy/common/system/user.py:53
> +def open_url(url):

We should give this some sort of pejorative name to discourage folks from using
this function.	They shouldn't be using a global static.  Instead, they should
be using a user instance hung off a context object.

> WebKitTools/Scripts/webkitpy/layout_tests/run_webkit_tests_unittest.py:100
> +    def setUp(self):
> +	   def open_url(url):
> +	       self._url_opened = url
> +	   self._user_open_url = user.open_url
> +	   user.open_url = open_url
> +	   self._url_opened = None

Mutating global statics in unit tests is error-prone.  It's much better to pass
in the dependencies explicitly so you can inject them explicitly too.