Comment on revision r5667ab41ba9615bc71f67ac55fc875fd3b4c90ae in signon-plugin-oauth2.accounts-sso

[accoun...@googlecode.com]

Comment by martin.k...@gmail.com:

Score: Positive

General Comment:
Looks good, couple minor things pointed below, but nothing that would
prevent shipping this

Line-by-line comments:

File: /src/base-plugin.cpp (r5667ab41ba9615bc71f67ac55fc875fd3b4c90ae)
===============================================================================

Line 49: void disposeReply();
-------------------------------------------------------------------------------
This stopped me for a second to think what "dispose" actually mean (not
native english speaker). While technically correct, I think "discard" would
be better fit here for future (easier) readability. Just an idea :)
Line 152: if (handleNetworkError(reply, reply->error()))
-------------------------------------------------------------------------------
Seems like this could take just the reply and do reply->error() inside
handleNetworkError...?
File: /src/base-plugin.h (r5667ab41ba9615bc71f67ac55fc875fd3b4c90ae)
===============================================================================

Line 67: virtual bool handleNetworkError(QNetworkReply *reply,
-------------------------------------------------------------------------------
One thing this could use is a doc somewhere to say what the return value is
(or should be)

For more information:
https://code.google.com/p/accounts-sso/source/detail?r=5667ab41ba9615bc71f67ac55fc875fd3b4c90ae&repo=signon-plugin-oauth2