On 2011-03-01, at 1:24 AM, Peter Hosey wrote: > On Feb 28, 2011, at 17:55:36, Colin Barrett^[c2bFrank Dowsett wrote: >> <xmppFB.diff> > > Return of the JSON framework! Different one, smaller in size, I believe.
>> +- (void)doIT > > Surely we can think of a better method name than that. As I wrote to Colin this "is my unfinished, nowhere-near-release-ready coding." > >> + token = >> @"176717249009197|2.SBY9kCilxdgHv6dRMAPZvQ__.86400.1297969200-899650296|AA0KSXp7kShNmKEC5PzAHdo6O50"; > > What is this? What is its origin/how was it made? > > This sort of thing should be documented in a variable name and/or comment. That was my FB token so that I didn't have to login. > >> + NSString *token = [adium.accountController passwordForAccount:account]; > >> + NSString *urlstring = [NSString >> stringWithFormat:@"https://graph.facebook.com/me?access_token=%@";, token]; > > So, if the user sets a password, it will be passed to Facebook in clear text > as a GET parameter? Is this necessary, or can we do better? The password is the access token. >> + NSDictionary *resp = [[[NSString alloc] initWithData:conn >> encoding:NSUTF8StringEncoding] JSONValue]; > > Leak. I know. > >> + [[NSNotificationCenter defaultCenter] >> postNotificationName:FACEBOOK_OAUTH_FINISHED > >> + object:[self parseURLParams:[[request URL] fragment]]]; > >> + NSString *token = [[note object] objectForKey:@"access_token"]; > > Are you sure you want fragment and not queryString? Yes. > I think an example of the sort of URL we're parsing here, stored in a comment > in the redirect method, is warranted. I agree. > [snip] See second comment. > Should this be a subclass of ESJabberService? I tried it at the start as a subclass, but there was a lot more going on than I thought needed to be before I could even login. >> +- (NSCharacterSet *)allowedCharacters{ >> + return [NSCharacterSet >> characterSetWithCharactersInString:@"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_ >> "]; >> +} >> + >> +- (NSCharacterSet *)allowedCharactersForUIDs{ >> + return [NSCharacterSet >> characterSetWithCharactersInString:@"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_"]; >> } > > Do we really only want the ASCII letters and numbers, or would it be better > to use the alphanumeric character set and add the ‘_’ and space to it? > > Also, is that space supposed to be there? Do we only want the space, or > should we allow all whitespace? What about line breaks? The space is for the formattedUID to show a pretty name. > If we make this a subclass of ESJabberService, should we inherit its > -allowedCharacters and -allowedCharactersForUIDs methods? Probably depends on how we want the account name displayed. >> +GCC_VERSION = com.apple.compilers.llvm.clang.1_0 > > Is this meant to be included in this patch? Eventually, but in a different patch. I didn't go through the patch at all. -- Frank
