[webkit-reviews] review granted: [Bug 170461] webkitpy: Add apple_additions to webkitpy to insert Internal tools : [Attachment 308451] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 170461: webkitpy: Add apple_additions to webkitpy to insert Internal tools
https://bugs.webkit.org/show_bug.cgi?id=170461

Attachment 308451: Patch

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




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

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

> Tools/Scripts/webkitpy/port/config.py:61
> +	   return __import__('apple_additions', globals(), None, [], 0)

I suggest that we use named parameters for all arguments except the first. We
should explain the purpose of the last argument, 0, in a comment even if we use
a named parameter calling style as the name of the last parameter, level, is
not very meaningful.

> Tools/Scripts/webkitpy/port/ios_device.py:55
> +	   return apple_additions().ios_device_driver() if apple_additions()
else super(IOSDevicePort, self)._driver_class()

Does it make sense to call the base class implementation if we do not have
Apple Additions? I mean, would using the base class driver class ever work?

If we need to call the base class implementation then I would write this as:

if apple_additions():
    return apple_additions().ios_device_driver()
return super(IOSDevicePort, self)._driver_class()

> Tools/Scripts/webkitpy/port/ios_device.py:60
> +	       iphoneos_sdk_version =
host.platform.xcode_sdk_version(IOSDevicePort.SDK)

I suggest that we access the SDK class variable using the argument cls instead
of hardcoding the class name. The benefit of doing this is the same as the
benefit of using super() to call the base class implementation.

> Tools/Scripts/webkitpy/port/ios_device.py:67
> +    # FIXME: These need device implementations <rdar://problem/30497991>

Minor: Missing period at the end of this sentence.

> Tools/Scripts/webkitpy/port/ios_device.py:82
> +	   return ['--sdk', IOSDevicePort.SDK] + (['ARCHS=%s' %
self.architecture()] if self.architecture() else [])

Ditto.

> Tools/Scripts/webkitpy/port/ios_device.py:89
> +	       raise RuntimeError('On-device testing is not supported on this
machine')

We could move the runtime error string literal into a class variable and avoid
the need to duplicate it here and in _device_for_worker_number_map().

> Tools/Scripts/webkitpy/port/ios_device_unittest.py:40
> +    # FIXME: Update tests when <rdar://problem/30497991> is completed

Minor: Missing a period at the end of this sentence.