[webkit-reviews] review denied: [Bug 180829] [webkitpy] Use PlatformInfo in Executive class. : [Attachment 335624] PATCH

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Basuke Suzuki
<Basuke.Suzuki at sony.com>'s request for review:
Bug 180829: [webkitpy] Use PlatformInfo in Executive class.
https://bugs.webkit.org/show_bug.cgi?id=180829

Attachment 335624: PATCH

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




--- Comment #6 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 335624
  --> https://bugs.webkit.org/attachment.cgi?id=335624
PATCH

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

r-, this creates a cycle between the Executive class and PlatformInfo class. I
also see little value in replacing the platform checks in Executive with
PlatformInfo because these replacements are mostly cosmetic and do little more
than turn around and query sys.platform. PlatformInfo.is_native_win() is one
convenience function whose usage improves the readability of the code. However
I do not feel that it justifies creating a cycle where Executive depends on
PlatformInfo.

Currently Executive is one of the fundamental building blocks of webkitpy.
PlatformInfo sits on top of Executive and for the most part is a thin wrapper
around Python's sys.platform module and exposes functions to query OS
capabilities. The latter requires that is have an Executive. Maybe we can find
another place for the latter, but the port is not the appropriate place for
this logic because a single platform may support more than one WebKit port.
Windows is an example of a platform that supports both the Apple Windows and
Win Cairo ports.

> Tools/Scripts/webkitpy/common/system/platforminfo.py:79
>      def executive(self):
>	   if self._executive is None:
> -	       self._executive = Executive()
> +	       from webkitpy.common.system.executive import Executive
> +	       self._executive = Executive(self)
>  
>	   return self._executive
>  

Please remove this code. As I wrote in bug 180827, comment 12, PlatformInfo
should never be instantiated in isolation. It should be instantiated as part of
a Host object.