Bug 180827 – [webkitpy] PlatformInfo should have default argument for casual use

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Basuke Suzuki]

Add singleton class method to PlatformInfo to be used anywhere which need platform information with consistent manner.

[Basuke Suzuki]

Created attachment 329384 [details]
patch

[Jonathan Bedard]

Comment on attachment 329384 [details]
patch

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

Generally, I like this change, I think it makes PlatformInfo easier to use outside of the host object.

> Tools/Scripts/webkitpy/common/system/systemhost.py:41
> -        self.executive = executive.Executive()
>          self.filesystem = filesystem.FileSystem()
>          self.user = user.User()
> -        self.platform = platforminfo.PlatformInfo(sys, platform, self.executive)
> +        self.platform = platforminfo.PlatformInfo.singleton()
> +        self.executive = self.platform.executive

I think instead of using the singleton instance of the PlatformInfo object (and by extension, a singleton Executive object) each host should continue to own it's own instance of these objects. This may break your plans for <https://bugs.webkit.org/show_bug.cgi?id=180829>, though.  Was the idea behind that bug just to use the singleton PlatformInfo object in place of sys.platform?

Something you should also be aware of regarding the host object is that you can have meaningfully different host objects instantiated on the same machine. This is how on-device testing works. The proposed code here doesn't actually break that functionality, because the device testing host isn't actually SystemHost object, it's a Device object (which uses Python's duck typing to work like a SystemHost). In short, the singleton pattern is the wrong paradigm for the SystemHost object.

As a side note, I think we should probably be passing self.platform into the the User constructor.

[Basuke Suzuki]

Got it. So forget about this patch, all I want to achieve is to reconstruct the dependency tree among host, platform info and executive. Currently host depends on platform and executive, platform depends on executive, and executive is there on its foot. In the actual code, though, executive uses many platform dependent code in it.

Singleton is not the objective of me. I just want to make the PlatformInfo more casual. Another possibility is to add default handling of sys_module and platform_module. If they are None, use system's sys and platform module. Then we can easily create own instance of PlatformInfo in many places. What do you think about this idea?

> As a side note, I think we should probably be passing self.platform into the the User constructor.

Agreed.

[Basuke Suzuki]

Created attachment 329493 [details]
patch

Stop using singleton pattern. Add default arguments for PlatformInfo instead.

[Basuke Suzuki]

Comment on attachment 329493 [details]
patch

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

> Tools/Scripts/webkitpy/common/system/user.py:46
> -    if not sys.platform.startswith('win32'):
> +    if not PlatformInfo().is_native_win():

This is what this patch really achieved. Now PlatformInfo is handy to use.

[Jonathan Bedard]

Comment on attachment 329493 [details]
patch

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

I tested this patch with apple_additions(), everything works fine there.

Unofficial R+ from me.

> Tools/Scripts/webkitpy/common/system/systemhost.py:39
> -        self.executive = executive.Executive()
>          self.filesystem = filesystem.FileSystem()
> -        self.user = user.User()
> -        self.platform = platforminfo.PlatformInfo(sys, platform, self.executive)
> +        self.platform = platforminfo.PlatformInfo()
> +        self.executive = self.platform.executive
> +        self.user = user.User(self.platform)

A bit of nit-pick, I think this is more clear:

self.executive = executive.Executive()
self.filesystem = filesystem.FileSystem()
self.platform = platforminfo.PlatformInfo(executive=self.executive)
self.user = user.User(self.platform)

The code you have makes it seem like it is the PlatformInfo object which owns the executive.

[Basuke Suzuki]

Make sense.

[Basuke Suzuki]

Created attachment 329499 [details]
patch

Revert to original by passing its own executive to PlatformInfo

[David Kilzer (:ddkilzer)]

Comment on attachment 329499 [details]
patch

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

r=me

> Tools/ChangeLog:14
> +        Currently to instanciate PlatformInfo, it requires arguments, which is usually
> +        sys, platform and Executive(). They are straight forward and should be handled
> +        by default arguments. Then we can instanciate PlatformInfo more casual.

Typo × 2:  instanciate => instantiate

[David Kilzer (:ddkilzer)]

Committed r226652: <https://trac.webkit.org/changeset/226652>

[Radar WebKit Bug Importer]

<rdar://problem/36380742>

[Daniel Bates]

Comment on attachment 329499 [details]
patch

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

I know that this patch was reviewed and landed a long time ago. I disagree with the direction of this patch.

>> Tools/ChangeLog:14
>> +        Currently to instanciate PlatformInfo, it requires arguments, which is usually
>> +        sys, platform and Executive(). They are straight forward and should be handled
>> +        by default arguments. Then we can instanciate PlatformInfo more casual.
> 
> Typo × 2:  instanciate => instantiate

PlatformInfo should never be instantiated in isolation. So, PlatformInfo should not have default argument values. The preferred way to get a PlatformInfo object is to instantiate a Host object. An example of this can be seen in check-webkit-style: <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/style/main.py?rev=229641#L126>.

> Tools/Scripts/webkitpy/common/system/platforminfo.py:85
> +    @property
> +    def executive(self):
> +        if self._executive is None:
> +            self._executive = Executive()
> +
> +        return self._executive

This function is unnecessary. Notice that SystemHost(), which is responsible for instantiating a PlatformInfo, creates and exposes an Executive object. PlatformInfo should not be responsible for creating an Executive. Instead to construct a PlatformInfo a developer should be required to pass an Executive object.

[Basuke Suzuki]

> PlatformInfo should never be instantiated in isolation. So, PlatformInfo should not have default argument values. The preferred way to get a PlatformInfo object is to instantiate a Host object. An example of this can be seen in check-webkit-style: <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/style/main.py?rev=229641#L126>.

I've restarted this patch.

[Basuke Suzuki]

Created attachment 343851 [details]
Rolling out

[Basuke Suzuki]

To rolling this out

[Basuke Suzuki]

Created attachment 343853 [details]
Rolling out

[WebKit Commit Bot]

Comment on attachment 343853 [details]
Rolling out

Clearing flags on attachment: 343853

Committed r233332: <https://trac.webkit.org/changeset/233332>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.