OS Lite SHINDIG-923

[louiscryan, 2009-02-20 01:19:12]

http://codereview.appspot.com/14109/diff/1/14
File features/features.txt (right):

http://codereview.appspot.com/14109/diff/1/14#newcode23
Line 23: features/opensocial.data/feature.xml
opensocial.data and opensocial-data dont mix well. Time for spec cleanup fun!

http://codereview.appspot.com/14109/diff/1/27
File features/opensocial.data.activities/activities.js (right):

http://codereview.appspot.com/14109/diff/1/27#newcode37
Line 37: opensocial.data.base.ensureAppId(options);
why do we need to ensureAppId. It should be inferred from the security token by
the receiving server if absent.

http://codereview.appspot.com/14109/diff/1/36
File features/opensocial.data.appdata/appdata.js (right):

http://codereview.appspot.com/14109/diff/1/36#newcode56
Line 56: ensureFieldsFromKeys(options);
dont think we want to do this either. I know this is because of the weirdo REST
semantics around PUT but we want to expose the protocol not hide it. Im all for
fixing the handling on the backend

http://codereview.appspot.com/14109/diff/1/36#newcode70
Line 70: ensureData(options);
Do we need to do this. If the caller didnt pass any does the backend break? If
so thats a bug on the backend not here.

http://codereview.appspot.com/14109/diff/1/36#newcode82
Line 82: var deleteData = function(options) {
Naming is odd, if we have conflict with 'delete' because of the reserved keyword
then we should use a unified replacement across all services.

http://codereview.appspot.com/14109/diff/1/35
File features/opensocial.data.base/jsonrequest.js (right):

http://codereview.appspot.com/14109/diff/1/35#newcode56
Line 56: "CONTENT_TYPE" : "JSON",
JSON_CONTENT_TYPE instead of "JSON ?

http://codereview.appspot.com/14109/diff/1/35#newcode99
Line 99: var defaultUserId = '@viewer';
defaults should live in the protocols and I think they do.

http://codereview.appspot.com/14109/diff/1/35#newcode124
Line 124: var normalizeSortAndFilterOptionNames = function(params) {
no need to do this any more

http://codereview.appspot.com/14109/diff/1/35#newcode133
Line 133: if (options.userId) {
protocol accepts one or many I believe as documented

http://codereview.appspot.com/14109/diff/1/31
File features/opensocial.data.base/util.js (right):

http://codereview.appspot.com/14109/diff/1/31#newcode53
Line 53: opensocial.data.base.translateHttpError = function(httpError) {
these should probably be extracted as delimited strings from the error message

http://codereview.appspot.com/14109/diff/1/23
File features/opensocial.data.people/people.js (right):

http://codereview.appspot.com/14109/diff/1/23#newcode26
Line 26: var defaultFields = ['id', 'displayName'];
protocol should handle this

http://codereview.appspot.com/14109/diff/1/11
File features/opensocial.data.ui/ui.js (right):

http://codereview.appspot.com/14109/diff/1/11#newcode30
Line 30: opensocial.data.ui = function() {
Lets discuss turning this into a protocol

[awiner, 2009-02-24 21:06:43]

Some haphazard comments.

http://codereview.appspot.com/14109/diff/1/28
File features/opensocial.data.activities/activitiestest.js (right):

http://codereview.appspot.com/14109/diff/1/28#newcode27
Line 27: shindig.auth = shindig.auth || {};
i think shindig.auth should just be overwritten here with {}.  In part, that
verifies that nothing out of shindig.auth other than getSecurityToken is called.

http://codereview.appspot.com/14109/diff/1/28#newcode28
Line 28: shindig.auth.getSecurityToken = shindig.auth.getSecurityToken ||
function() {
think it should just be overridden unconditionally

http://codereview.appspot.com/14109/diff/1/28#newcode38
Line 38: shindig.auth = undefined;
i think the original shindig.auth should be restored

http://codereview.appspot.com/14109/diff/1/28#newcode42
Line 42: *
cool comment ;)

http://codereview.appspot.com/14109/diff/1/28#newcode138
Line 138: [{title:"yellow",userId:"john.doe",id:"1",body:"what a color!"}],
JSON formatting is widely divergent within this file.  pick a style?

http://codereview.appspot.com/14109/diff/1/33
File features/opensocial.data.base/batch.js (right):

http://codereview.appspot.com/14109/diff/1/33#newcode65
Line 65: opensocial.data.base.setGlobalError(responseMap, error.error);
should move to context API?

http://codereview.appspot.com/14109/diff/1/22
File features/test/internal/testutils.js (right):

http://codereview.appspot.com/14109/diff/1/22#newcode51
Line 51: var makeInspectableCallback = function() {
pass callback to this function, instead of setRealCallback() - it's always used,
right?

http://codereview.appspot.com/14109/diff/1/22#newcode62
Line 62: realCallback(result);
i'd use 
  realCallback.apply(this, args)
... so this will work for any set of arguments, with "this" preserved.

[rbe5000, 2009-02-27 07:43:37]

I fixed all the stuff that wasn't related to default handling of json params by
protocol handlers, because several of those suggestions were broke, and didn't
have a chance yet to figure out why they are broken on the server.

Still, wanted to get the first pass checked in, and I can figure out the
problems when I get back.

http://codereview.appspot.com/14109/diff/1/14
File features/features.txt (right):

http://codereview.appspot.com/14109/diff/1/14#newcode23
Line 23: features/opensocial.data/feature.xml
On 2009/02/20 01:19:12, louiscryan wrote:
> opensocial.data and opensocial-data dont mix well. Time for spec cleanup fun!

Done.

http://codereview.appspot.com/14109/diff/1/28
File features/opensocial.data.activities/activitiestest.js (right):

http://codereview.appspot.com/14109/diff/1/28#newcode27
Line 27: shindig.auth = shindig.auth || {};
On 2009/02/24 21:06:43, awiner wrote:
> i think shindig.auth should just be overwritten here with {}.  In part, that
> verifies that nothing out of shindig.auth other than getSecurityToken is
called.

There are some other weird things going on, but overwrote it in all tests, and
then undefined it, because any test using shindig.auth should ensure it's
existence.
Done.

http://codereview.appspot.com/14109/diff/1/28#newcode28
Line 28: shindig.auth.getSecurityToken = shindig.auth.getSecurityToken ||
function() {
On 2009/02/24 21:06:43, awiner wrote:
> think it should just be overridden unconditionally

Done.

http://codereview.appspot.com/14109/diff/1/28#newcode38
Line 38: shindig.auth = undefined;
On 2009/02/24 21:06:43, awiner wrote:
> i think the original shindig.auth should be restored

Done.

http://codereview.appspot.com/14109/diff/1/28#newcode42
Line 42: *
On 2009/02/24 21:06:43, awiner wrote:
> cool comment ;)

Done.

http://codereview.appspot.com/14109/diff/1/28#newcode138
Line 138: [{title:"yellow",userId:"john.doe",id:"1",body:"what a color!"}],
On 2009/02/24 21:06:43, awiner wrote:
> JSON formatting is widely divergent within this file.  pick a style?

Done.

http://codereview.appspot.com/14109/diff/1/35
File features/opensocial.data.base/jsonrequest.js (right):

http://codereview.appspot.com/14109/diff/1/35#newcode56
Line 56: "CONTENT_TYPE" : "JSON",
On 2009/02/20 01:19:12, louiscryan wrote:
> JSON_CONTENT_TYPE instead of "JSON ?

Done.

http://codereview.appspot.com/14109/diff/1/35#newcode124
Line 124: var normalizeSortAndFilterOptionNames = function(params) {
On 2009/02/20 01:19:12, louiscryan wrote:
> no need to do this any more

Done.

http://codereview.appspot.com/14109/diff/1/22
File features/test/internal/testutils.js (right):

http://codereview.appspot.com/14109/diff/1/22#newcode51
Line 51: var makeInspectableCallback = function() {
On 2009/02/24 21:06:43, awiner wrote:
> pass callback to this function, instead of setRealCallback() - it's always
used,
> right?

Done.

http://codereview.appspot.com/14109/diff/1/22#newcode62
Line 62: realCallback(result);
On 2009/02/24 21:06:43, awiner wrote:
> i'd use 
>   realCallback.apply(this, args)
> ... so this will work for any set of arguments, with "this" preserved.

Done.