GitHub PullRequestReviewCommentEvent at 2014-10-09 16:34:18
Repository: ds-mdata-responder
Actor: tongxiang
Payload: {"action":"created","comment":{"url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/comments/18656993","id":18656993,"diff_hunk":"@@ -1367,20 +1380,22 @@ SGCompetitiveStoryController.prototype._endGameFromPlayerExit = function(playerD\n  * @param delay\n  *   Time in milliseconds to delay the message. Defaults to 0 if not set.\n  */\n-\n-SGCompetitiveStoryController.prototype.scheduleMobileCommonsOptIn = function(args, delay) {\n-  if (!delay) {\n-    delay = 0;\n-  }\n-\n-  setTimeout(function() {\n+function scheduleMobileCommonsOptIn(args, delay) {","path":"app/controllers/SGCompetitiveStoryController.js","position":202,"original_position":202,"commit_id":"06aae07b41f47edb7cea01700861009f7a1ef091","original_commit_id":"06aae07b41f47edb7cea01700861009f7a1ef091","user":{"login":"tongxiang","id":5678066,"avatar_url":"https://avatars.githubusercontent.com/u/5678066?v=2","gravatar_id":"","url":"https://api.github.com/users/tongxiang","html_url":"https://github.com/tongxiang","followers_url":"https://api.github.com/users/tongxiang/followers","following_url":"https://api.github.com/users/tongxiang/following{/other_user}","gists_url":"https://api.github.com/users/tongxiang/gists{/gist_id}","starred_url":"https://api.github.com/users/tongxiang/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tongxiang/subscriptions","organizations_url":"https://api.github.com/users/tongxiang/orgs","repos_url":"https://api.github.com/users/tongxiang/repos","events_url":"https://api.github.com/users/tongxiang/events{/privacy}","received_events_url":"https://api.github.com/users/tongxiang/received_events","type":"User","site_admin":false},"body":"Would it be a good idea to pull this function into the `mobilecommons.js` module? Or not, since it's referencing a `mobilecommons.js` function? ","created_at":"2014-10-09T16:34:18Z","updated_at":"2014-10-09T16:34:18Z","html_url":"https://github.com/DoSomething/ds-mdata-responder/pull/208#discussion_r18656993","pull_request_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/208","_links":{"self":{"href":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/comments/18656993"},"html":{"href":"https://github.com/DoSomething/ds-mdata-responder/pull/208#discussion_r18656993"},"pull_request":{"href":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/208"}}},"pull_request":{"url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/208","id":22503132,"html_url":"https://github.com/DoSomething/ds-mdata-responder/pull/208","diff_url":"https://github.com/DoSomething/ds-mdata-responder/pull/208.diff","patch_url":"https://github.com/DoSomething/ds-mdata-responder/pull/208.patch","issue_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/issues/208","number":208,"state":"open","locked":false,"title":"Decrease # of setTimeout calls and simplify closure data in each","user":{"login":"jonuy","id":696595,"avatar_url":"https://avatars.githubusercontent.com/u/696595?v=2","gravatar_id":"","url":"https://api.github.com/users/jonuy","html_url":"https://github.com/jonuy","followers_url":"https://api.github.com/users/jonuy/followers","following_url":"https://api.github.com/users/jonuy/following{/other_user}","gists_url":"https://api.github.com/users/jonuy/gists{/gist_id}","starred_url":"https://api.github.com/users/jonuy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jonuy/subscriptions","organizations_url":"https://api.github.com/users/jonuy/orgs","repos_url":"https://api.github.com/users/jonuy/repos","events_url":"https://api.github.com/users/jonuy/events{/privacy}","received_events_url":"https://api.github.com/users/jonuy/received_events","type":"User","site_admin":false},"body":"#### What's this PR do?\r\nIn an effort to resolve the gradual memory increase we're seeing in the app, we're starting by trying to cleanup our use of `setTimeout` calls.\r\n\r\n#### Where should the reviewer start?\r\n* With `scheduleMobileCommonsOptIn()`, we're decreasing the number of setTimeout calls by just calling the `optin` method if `delay` is either undefined or 0.\r\n\r\n* The logic to send a solo play message has been moved into `scheduleSoloMessage()`. Instead of passing in the entire context of the SGCompetitveStoryController object to be tracked in the closure, the callback only needs the game id, a reference to the game model, and the opt-in path. I'm not entirely sure if this is gonna show marked improvement, but it does at least simplify the data needed within the closure.\r\n\r\n#### How should this be manually tested?\r\nSuccessful test cases I manually went through:\r\n* Creating game and receiving the solo-play message\r\n* Creating game, having a beta join, and not receiving the solo-play message\r\n* Creating game, having a beta join, and playing through the first level\r\n\r\nFeel free to try those or any others you might think are worth trying\r\n\r\n#### Any background context?\r\nThis is in response to some of the sporadic 502 errors the app is throwing. The hunch is that memory isn't getting properly cleaned up somewhere, and our first attempt is with these setTimeouts.","created_at":"2014-10-09T15:51:30Z","updated_at":"2014-10-09T16:34:18Z","closed_at":null,"merged_at":null,"merge_commit_sha":"a9e17f80dacd58b6c4125a843a4de1f31393cf87","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/208/commits","review_comments_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/208/comments","review_comment_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/comments/{number}","comments_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/issues/208/comments","statuses_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/statuses/06aae07b41f47edb7cea01700861009f7a1ef091","head":{"label":"jonuy:settimeout","ref":"settimeout","sha":"06aae07b41f47edb7cea01700861009f7a1ef091","user":{"login":"jonuy","id":696595,"avatar_url":"https://avatars.githubusercontent.com/u/696595?v=2","gravatar_id":"","url":"https://api.github.com/users/jonuy","html_url":"https://github.com/jonuy","followers_url":"https://api.github.com/users/jonuy/followers","following_url":"https://api.github.com/users/jonuy/following{/other_user}","gists_url":"https://api.github.com/users/jonuy/gists{/gist_id}","starred_url":"https://api.github.com/users/jonuy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jonuy/subscriptions","organizations_url":"https://api.github.com/users/jonuy/orgs","repos_url":"https://api.github.com/users/jonuy/repos","events_url":"https://api.github.com/users/jonuy/events{/privacy}","received_events_url":"https://api.github.com/users/jonuy/received_events","type":"User","site_admin":false},"repo":{"id":21406365,"name":"ds-mdata-responder","full_name":"jonuy/ds-mdata-responder","owner":{"login":"jonuy","id":696595,"avatar_url":"https://avatars.githubusercontent.com/u/696595?v=2","gravatar_id":"","url":"https://api.github.com/users/jonuy","html_url":"https://github.com/jonuy","followers_url":"https://api.github.com/users/jonuy/followers","following_url":"https://api.github.com/users/jonuy/following{/other_user}","gists_url":"https://api.github.com/users/jonuy/gists{/gist_id}","starred_url":"https://api.github.com/users/jonuy/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/jonuy/subscriptions","organizations_url":"https://api.github.com/users/jonuy/orgs","repos_url":"https://api.github.com/users/jonuy/repos","events_url":"https://api.github.com/users/jonuy/events{/privacy}","received_events_url":"https://api.github.com/users/jonuy/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/jonuy/ds-mdata-responder","description":" Handle mData requests from Mobile Commons.","fork":true,"url":"https://api.github.com/repos/jonuy/ds-mdata-responder","forks_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/forks","keys_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/keys{/key_id}","collaborators_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/teams","hooks_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/hooks","issue_events_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/issues/events{/number}","events_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/events","assignees_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/assignees{/user}","branches_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/branches{/branch}","tags_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/tags","blobs_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/git/refs{/sha}","trees_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/git/trees{/sha}","statuses_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/statuses/{sha}","languages_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/languages","stargazers_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/stargazers","contributors_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/contributors","subscribers_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/subscribers","subscription_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/subscription","commits_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/commits{/sha}","git_commits_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/git/commits{/sha}","comments_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/comments{/number}","issue_comment_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/issues/comments/{number}","contents_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/contents/{+path}","compare_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/compare/{base}...{head}","merges_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/merges","archive_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/downloads","issues_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/issues{/number}","pulls_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/pulls{/number}","milestones_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/milestones{/number}","notifications_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/labels{/name}","releases_url":"https://api.github.com/repos/jonuy/ds-mdata-responder/releases{/id}","created_at":"2014-07-02T00:26:42Z","updated_at":"2014-07-24T19:36:50Z","pushed_at":"2014-10-09T15:08:42Z","git_url":"git://github.com/jonuy/ds-mdata-responder.git","ssh_url":"git@github.com:jonuy/ds-mdata-responder.git","clone_url":"https://github.com/jonuy/ds-mdata-responder.git","svn_url":"https://github.com/jonuy/ds-mdata-responder","homepage":null,"size":1426,"stargazers_count":0,"watchers_count":0,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":1,"forks":0,"open_issues":1,"watchers":0,"default_branch":"master"}},"base":{"label":"DoSomething:dev","ref":"dev","sha":"f22458760fe913a3cf2b520c3eada934e35ae92b","user":{"login":"DoSomething","id":731685,"avatar_url":"https://avatars.githubusercontent.com/u/731685?v=2","gravatar_id":"","url":"https://api.github.com/users/DoSomething","html_url":"https://github.com/DoSomething","followers_url":"https://api.github.com/users/DoSomething/followers","following_url":"https://api.github.com/users/DoSomething/following{/other_user}","gists_url":"https://api.github.com/users/DoSomething/gists{/gist_id}","starred_url":"https://api.github.com/users/DoSomething/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DoSomething/subscriptions","organizations_url":"https://api.github.com/users/DoSomething/orgs","repos_url":"https://api.github.com/users/DoSomething/repos","events_url":"https://api.github.com/users/DoSomething/events{/privacy}","received_events_url":"https://api.github.com/users/DoSomething/received_events","type":"Organization","site_admin":false},"repo":{"id":17884814,"name":"ds-mdata-responder","full_name":"DoSomething/ds-mdata-responder","owner":{"login":"DoSomething","id":731685,"avatar_url":"https://avatars.githubusercontent.com/u/731685?v=2","gravatar_id":"","url":"https://api.github.com/users/DoSomething","html_url":"https://github.com/DoSomething","followers_url":"https://api.github.com/users/DoSomething/followers","following_url":"https://api.github.com/users/DoSomething/following{/other_user}","gists_url":"https://api.github.com/users/DoSomething/gists{/gist_id}","starred_url":"https://api.github.com/users/DoSomething/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DoSomething/subscriptions","organizations_url":"https://api.github.com/users/DoSomething/orgs","repos_url":"https://api.github.com/users/DoSomething/repos","events_url":"https://api.github.com/users/DoSomething/events{/privacy}","received_events_url":"https://api.github.com/users/DoSomething/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/DoSomething/ds-mdata-responder","description":" Handle mData requests from Mobile Commons.","fork":false,"url":"https://api.github.com/repos/DoSomething/ds-mdata-responder","forks_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/forks","keys_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/keys{/key_id}","collaborators_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/teams","hooks_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/hooks","issue_events_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/issues/events{/number}","events_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/events","assignees_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/assignees{/user}","branches_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/branches{/branch}","tags_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/tags","blobs_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/git/refs{/sha}","trees_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/git/trees{/sha}","statuses_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/statuses/{sha}","languages_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/languages","stargazers_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/stargazers","contributors_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/contributors","subscribers_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/subscribers","subscription_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/subscription","commits_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/commits{/sha}","git_commits_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/git/commits{/sha}","comments_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/comments{/number}","issue_comment_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/issues/comments/{number}","contents_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/contents/{+path}","compare_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/compare/{base}...{head}","merges_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/merges","archive_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/downloads","issues_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/issues{/number}","pulls_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls{/number}","milestones_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/milestones{/number}","notifications_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/labels{/name}","releases_url":"https://api.github.com/repos/DoSomething/ds-mdata-responder/releases{/id}","created_at":"2014-03-18T22:51:45Z","updated_at":"2014-09-02T16:02:41Z","pushed_at":"2014-10-09T13:54:00Z","git_url":"git://github.com/DoSomething/ds-mdata-responder.git","ssh_url":"git@github.com:DoSomething/ds-mdata-responder.git","clone_url":"https://github.com/DoSomething/ds-mdata-responder.git","svn_url":"https://github.com/DoSomething/ds-mdata-responder","homepage":null,"size":3417,"stargazers_count":1,"watchers_count":1,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":5,"mirror_url":null,"open_issues_count":12,"forks":5,"open_issues":12,"watchers":1,"default_branch":"dev"}},"_links":{"self":{"href":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/208"},"html":{"href":"https://github.com/DoSomething/ds-mdata-responder/pull/208"},"issue":{"href":"https://api.github.com/repos/DoSomething/ds-mdata-responder/issues/208"},"comments":{"href":"https://api.github.com/repos/DoSomething/ds-mdata-responder/issues/208/comments"},"review_comments":{"href":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/208/comments"},"review_comment":{"href":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/comments/{number}"},"commits":{"href":"https://api.github.com/repos/DoSomething/ds-mdata-responder/pulls/208/commits"},"statuses":{"href":"https://api.github.com/repos/DoSomething/ds-mdata-responder/statuses/06aae07b41f47edb7cea01700861009f7a1ef091"}}}}