Add singleton class method to PlatformInfo to be used anywhere which need platform information with consistent manner.
Created attachment 329384 [details] patch
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.
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.
Created attachment 329493 [details] patch Stop using singleton pattern. Add default arguments for PlatformInfo instead.
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.
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.
Make sense.
Created attachment 329499 [details] patch Revert to original by passing its own executive to PlatformInfo
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
Committed r226652: <https://trac.webkit.org/changeset/226652>
<rdar://problem/36380742>
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.
> 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.
Created attachment 343851 [details] Rolling out
To rolling this out
Created attachment 343853 [details] Rolling out
Comment on attachment 343853 [details] Rolling out Clearing flags on attachment: 343853 Committed r233332: <https://trac.webkit.org/changeset/233332>
All reviewed patches have been landed. Closing bug.