[webkit-reviews] review denied: [Bug 73226] make chromium layout_tests observe use_skia_on_mac.gypi from chromium/build : [Attachment 116775] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied epoger at google.com's request for
review:
Bug 73226: make chromium layout_tests observe use_skia_on_mac.gypi from
chromium/build
https://bugs.webkit.org/show_bug.cgi?id=73226

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116775&action=review


> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:50
> +    gypi_path = os.path.join(
> +	   os.path.dirname(__file__),
> +	   os.path.pardir, os.path.pardir, os.path.pardir, os.path.pardir,
os.path.pardir,
> +	   'Source', 'WebKit', 'chromium', 'build', 'use_skia_on_mac.gypi')

Filesystem, please.  Also Checkout can point you to the chromium directory I
believe.  Certainly Port can.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:54
> +    with open(gypi_path) as gypi_file:

Please use FileSystem.	Please move this to a class so it can have a pointer to
a Host/FileSystem so it can be mocked and tested.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:55
> +	   use_skia_dict = eval(gypi_file.read())

Is this the expected way to parse gypi's?

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:58
> +	   if use_skia_dict['use_skia_on_mac%'] == 1:
> +	       return True
> +	   else:

This could just be return use_skia_dict['use_skia_on_mac%'] == 1 instead of
this 4-line if.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac.py:59
>> +		return False
> 
> Woah, crazy.	This seems like something of a hack.

This is a huge hack. :(  Doubly so that it's a free function. :(

> Tools/Scripts/webkitpy/layout_tests/port/chromium_mac_unittest.py:54
> +	   if chromium_mac.is_using_skia_by_default():
> +	       self.assertTrue(self.make_port().name() in (
> +		   'chromium-mac-leopard', 'chromium-mac-snowleopard',
'chromium-mac-lion', 'chromium-mac-future'))
> +	   else:
> +	       self.assertTrue(self.make_port().name() in (
> +		   'chromium-cg-mac-leopard', 'chromium-cg-mac-snowleopard',
'chromium-cg-mac-lion', 'chromium-cg-mac-future'))

No, no.  Please do not make the unittests depend on the environment anymore
than they already do.