[webkit-reviews] review granted: [Bug 41269] add python keyring support to webkit-patch : [Attachment 69114] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Tony Chang <tony at chromium.org>'s
request for review:
Bug 41269: add python keyring support to webkit-patch
https://bugs.webkit.org/show_bug.cgi?id=41269

Attachment 69114: Patch
https://bugs.webkit.org/attachment.cgi?id=69114&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69114&action=review

I think this is great, but there are some nits above.  I'm happy to look again.
 It would be OK to land as is, but probably best to go one more round.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:45
> +    import keyring

Should we just autoinstall this instead?

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:57
> +	   self.keyring = keyring

Seems odd to hang this off of self, especially since it's not passed in via the
constructor.

Also, despite our earlier lack of understanding of python design.  Adam and I
know realize that we should be naming private members to _foo, so this should
be _keyring and the other members will eventually get fixed.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:126
>	   if not username or not password:
>	       (username, password) = self._credentials_from_keychain(username)


Would we need this apple-specific code if the keyring module was autoinstalled?
 If you don't feel comfortable removing it, we could add a FIXME about removing
it.

> WebKitTools/Scripts/webkitpy/common/net/credentials.py:139
> +		       "Store password in system keyring?", User.DEFAULT_NO)

I wonder if keyring exposes some platform-specific naming here.  Since on OS X
its called the keychain, and I expect windows has a specific name, etc.

> WebKitTools/Scripts/webkitpy/common/net/credentials_unittest.py:124
> +		   self.keyring = MockKeyring()

Seems it would be better to pass it as an optional parameter to the
constructor, which default to None and then set it in __init__ as self._keyring
= keyring or keyring_module?