next patch coming up implementing all reccomendations.
http://codereview.appspot.com/149041/diff/3003/3004
File java/common/conf/shindig.properties (right):
http://codereview.appspot.com/149041/diff/3003/3004#newcode45
java/common/conf/shindig.properties:45:
shindig.signing.ownerPageSecure=false
okay, changes to
viewer-access-tokens-enabled
and isViewerAccessTokensEnabled
On 2009/11/09 00:58:11, beaton wrote:
naming convention in this file suggests
"shindig.signing.owner-page-secure".
But I think a better name might be
"shindig.signing.enable-viewer-access-tokens"
http://codereview.appspot.com/149041/diff/3003/3005
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java
(right):
http://codereview.appspot.com/149041/diff/3003/3005#newcode62
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:62:
public GadgetOAuthTokenStore(OAuthStore store, GadgetSpecFactory
specFactory, OAuthFetcherConfig fetcherConfig) {
sounds good, changed..
On 2009/11/09 00:58:11, beaton wrote:
hmm. my initial instinct was to say fetcherConfig should be a
parameter on
getOAuthAccessToken, so that fetcher configs can vary per request...
Does that
seem reasonable?
http://codereview.appspot.com/149041/diff/3003/3005#newcode116
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/GadgetOAuthTokenStore.java:116:
securityToken.getOwnerId().equals(securityToken.getViewerId())) {
not sure where this would live? fetcherConfig? Willing to add this
later..
On 2009/11/09 00:58:11, beaton wrote:
my goodness. maybe time to create a method for this instead? That
way we can
collapse these lines to "if (shouldUseToken(...)) {"
http://codereview.appspot.com/149041/diff/3003/3007
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java
(right):
http://codereview.appspot.com/149041/diff/3003/3007#newcode330
java/gadgets/src/main/java/org/apache/shindig/gadgets/oauth/OAuthRequest.java:330:
"(state owner=" + stateOwner + ", pageOwner=" + pageViewer + ')');
fixed
On 2009/11/09 00:58:11, beaton wrote:
text should be pageViewer here
http://codereview.appspot.com/149041
