Created attachment 121786 [details] Patch

Comment on attachment 121786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121786&action=review > Tools/Scripts/webkitpy/common/system/platforminfo.py:92 > + if self.os_name == 'mac': It's a little strange to my eyes that os_name is a member instead of a function. But it does make sense. It's not like the platform changes during execution. :) I also have concern with string comparisions like this as they're very error prone. I've found lots of errors in webkitpy where we check things like sys.platform == "mac" (when it's really supposed to be "darwin"), or sys.platform == "Darwin", etc. I think we should consider using platform_info.is_mac() (or just .is_mac) to get around this possible gotcha. In any case, this is a great improvement. centralizing our platform calls behind a mock is a good thing.

I'd like you and Adam to have a chance to see/respond to my musings. Then I'll happily r+ this tomorrow. I'm a bit too drained to give a full review atm.

Comment on attachment 121786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121786&action=review This is great. > Tools/Scripts/webkitpy/common/system/platforminfo.py:54 > + raise AssertionError('unrecognized platform string "%s"' % sys_platform) Why not just assert (the language construct)? > Tools/Scripts/webkitpy/common/system/platforminfo.py:63 > + # FIXME: Should this routine enforce minimum versions and handle 'future', or let callers do that? I would support future here. >> Tools/Scripts/webkitpy/common/system/platforminfo.py:92 >> + if self.os_name == 'mac': > > It's a little strange to my eyes that os_name is a member instead of a function. But it does make sense. It's not like the platform changes during execution. :) > > I also have concern with string comparisions like this as they're very error prone. I've found lots of errors in webkitpy where we check things like sys.platform == "mac" (when it's really supposed to be "darwin"), or sys.platform == "Darwin", etc. > > I think we should consider using platform_info.is_mac() (or just .is_mac) to get around this possible gotcha. > > In any case, this is a great improvement. centralizing our platform calls behind a mock is a good thing. Yeah, having an is_mac predicate is a good idea. > Tools/Scripts/webkitpy/common/system/platforminfo_unittest.py:48 > + def setUp(self): > + self.sys_platform = sys.platform > + self.getwindowsversion = None > + if hasattr(sys, 'getwindowsversion'): > + self.getwindowsversion = getattr(sys, 'getwindowsversion') > + del sys.getwindowsversion Hum... mutating globals is sad times. Maybe we should pass in a mock sys object to the platforminfo constructor?

(In reply to comment #2) > (From update of attachment 121786 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121786&action=review > > > Tools/Scripts/webkitpy/common/system/platforminfo.py:92 > > + if self.os_name == 'mac': > > It's a little strange to my eyes that os_name is a member instead of a function. But it does make sense. It's not like the platform changes during execution. :) > I don't feel strongly one way or another here. > I also have concern with string comparisions like this as they're very error prone. I've found lots of errors in webkitpy where we check things like sys.platform == "mac" (when it's really supposed to be "darwin"), or sys.platform == "Darwin", etc. > > I think we should consider using platform_info.is_mac() (or just .is_mac) to get around this possible gotcha. > Sure, I can add some convenience functions. I'll add is_mac, is_win, is_linux, and wrappers for mac versions, since I've seen those elsewhere, but I won't add win or linux version wrappers until someone needs them ... (In reply to comment #4) > (From update of attachment 121786 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121786&action=review > > This is great. > > > Tools/Scripts/webkitpy/common/system/platforminfo.py:54 > > + raise AssertionError('unrecognized platform string "%s"' % sys_platform) > > Why not just assert (the language construct)? > I'm not sure I follow you ... what language construct would that be in this case? > > Tools/Scripts/webkitpy/common/system/platforminfo.py:63 > > + # FIXME: Should this routine enforce minimum versions and handle 'future', or let callers do that? > > I would support future here. > Okay. > > Tools/Scripts/webkitpy/common/system/platforminfo_unittest.py:48 > > + def setUp(self): > > + self.sys_platform = sys.platform > > + self.getwindowsversion = None > > + if hasattr(sys, 'getwindowsversion'): > > + self.getwindowsversion = getattr(sys, 'getwindowsversion') > > + del sys.getwindowsversion > > Hum... mutating globals is sad times. Maybe we should pass in a mock sys object to the platforminfo constructor? I will try to rework it to make it more better :).

> I'm not sure I follow you ... what language construct would that be in this case? Reading <http://docs.python.org/reference/simple_stmts.html>, it seems like the construct doesn't exist. I must have dreamed it up.

Comment on attachment 121786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121786&action=review >>> Tools/Scripts/webkitpy/common/system/platforminfo.py:54 >>> + raise AssertionError('unrecognized platform string "%s"' % sys_platform) >> >> Why not just assert (the language construct)? > > I'm not sure I follow you ... what language construct would that be in this case? Presumably you could do something like: assert False, 'unrecognized platform string "%s"' % sys_platform

Created attachment 121912 [details] revamp to make it more mockable, add is_mac(), etc.

Comment on attachment 121912 [details] revamp to make it more mockable, add is_mac(), etc. View in context: https://bugs.webkit.org/attachment.cgi?id=121912&action=review Thanks for your work on this. > Tools/Scripts/webkitpy/common/system/platforminfo.py:39 > + Public (static) properties: > + -- os_name > + -- os_version I'm slightly wary about making the public, now that you have the is_mac() stuff. We wish to discourage folks from writing error-prone string comparisons. > Tools/Scripts/webkitpy/common/system/platforminfo.py:67 > + if self.os_name == 'mac': I think you want is_mac() here.

(In reply to comment #9) > (From update of attachment 121912 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121912&action=review > > Thanks for your work on this. > > > Tools/Scripts/webkitpy/common/system/platforminfo.py:39 > > + Public (static) properties: > > + -- os_name > > + -- os_version > > I'm slightly wary about making the public, now that you have the is_mac() stuff. We wish to discourage folks from writing error-prone string comparisons. > True. However, at least for now there are places where I need a string, e.g., in expanding parsing "chromium-mac" and figuring out what the os mapping is. Hopefully once I get further along in the refactoring I'll be able to get rid of most of these references. > > Tools/Scripts/webkitpy/common/system/platforminfo.py:67 > > + if self.os_name == 'mac': > > I think you want is_mac() here. Good catch, will fix.

Committed r104637: <http://trac.webkit.org/changeset/104637>