Bug 180829

Created attachment 335624 [details] PATCH

Comment on attachment 335624 [details] PATCH View in context: https://bugs.webkit.org/attachment.cgi?id=335624&action=review > Tools/ChangeLog:8 > + Remove direct sys dependencies form Executive class and use PlatformInfo. Can you explain the motivation for this change? I mean, it creates a circular dependency and needs the workaround you did in PlatformInfo.

Comment on attachment 335624 [details] PATCH Attachment 335624 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/6921075 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

Created attachment 335642 [details] Archive of layout-test-results from ews205 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews205 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

(In reply to Daniel Bates from comment #2) > Comment on attachment 335624 [details] > PATCH > > View in context: > https://bugs.webkit.org/attachment.cgi?id=335624&action=review > > > Tools/ChangeLog:8 > > + Remove direct sys dependencies form Executive class and use PlatformInfo. > > Can you explain the motivation for this change? I mean, it creates a > circular dependency and needs the workaround you did in PlatformInfo. Motivation is simple. Current platform detection is so messy. Every module uses different way to detect running platform, especially detection of cygwin and native_win . Also direct detect makes mock testing difficult. My goal is to make PlatformInfo as the unique way of platform detection. The workaround causes because PlatformInfo do too much by itself. The executive is used to detect Xcode version or total memory size. Both are only for Mac. They can safely move to Mac port, I guess.

Comment on attachment 335624 [details] 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.

(In reply to Basuke Suzuki from comment #5) > My goal is to make PlatformInfo as the unique way of platform detection. > I disagree with this goal. See my remarks in bug 180827, comment 12 and comment #6 for more details.

(In reply to Daniel Bates from comment #7) > (In reply to Basuke Suzuki from comment #5) > > My goal is to make PlatformInfo as the unique way of platform detection. > > > > I disagree with this goal. See my remarks in bug 180827, comment 12 and > comment #6 for more details. > The preferred way to get a PlatformInfo object is to instantiate a Host object. Okay, we can switch to use Host().platform for casual usage, but still weird to use PlatformInfo inside Executive. There're many platform dependent code in Executive and PlatformInfo should be used to detect such platform. Forget about my patch, I believe you agree with this goal, don't you? The issue may platform depends on Executive and Executive does not have any on instantiation. How about changing this to both depends host object? If platform requires executive, it can be accessible via self.host.executive. And Executive can use platform via self.host.executive.

Cannot get agreement for the goal.