[webkit-reviews] review denied: [Bug 171881] webkitpy: Run sample/spindump on iOS devices : [Attachment 312245] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 171881: webkitpy: Run sample/spindump on iOS devices
https://bugs.webkit.org/show_bug.cgi?id=171881

Attachment 312245: Patch

https://bugs.webkit.org/attachment.cgi?id=312245&action=review




--- Comment #10 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 312245
  --> https://bugs.webkit.org/attachment.cgi?id=312245
Patch

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

> Tools/Scripts/webkitpy/port/darwin.py:159
> +	       str(pid),

Is it necessary to stringify this? I thought Executive will do this for us. If
so, then please remove the str().

> Tools/Scripts/webkitpy/port/darwin.py:160
> +	       '10',

Similarly, do we need to stringify this? See my above remark.

> Tools/Scripts/webkitpy/port/darwin.py:162
> +	       '-file',

Ditto.

> Tools/Scripts/webkitpy/port/darwin.py:165
> +	   command = ['/usr/bin/sudo', '-n'] + command if
self.host.platform.is_mac() else command

I suggest writing this as a single if statement without an else clause.

> Tools/Scripts/webkitpy/port/darwin.py:171
> +		       str(pid),

Do we need to stringify this? See my remark for line 159.

> Tools/Scripts/webkitpy/port/darwin.py:172
> +		       '10',

Ditto.

> Tools/Scripts/webkitpy/port/darwin.py:173
> +		       '10',

Ditto.

> Tools/Scripts/webkitpy/port/darwin_testcase.py:107
>	   port.host.filesystem.files['/__im_tmp/tmp_0_/test-42-spindump.txt']
= 'Spindump file'

We should move this test and the test you modify below as well as any other
applicable ones to the test file for the Mac port. We should not be asserting
Mac in this test because these tests should be applicable to all Darwin-based
platforms.

> Tools/Scripts/webkitpy/port/ios_device_unittest.py:32
> +    os_name = 'ios'

What do we use this class variable for?

> Tools/Scripts/webkitpy/port/ios_device_unittest.py:82
> +	       raise ScriptError("MOCK script error")

" => '

> Tools/Scripts/webkitpy/port/ios_simulator_unittest.py:33
> +    os_name = 'mac'

What do we use this class variable for?