Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
19967005:
Simplifying using tasks (Closed)
|
Created:
7 years, 5 months ago by vadimt Modified:
7 years, 5 months ago CC:
chromium-reviews, arv+watch_chromium.org, stromme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSimplifying using tasks. Now:
1. debugSetStepName is not needed
2. No need to call a completion callback when a task ends.
To minimize conflicts with pending changes, debugSetStepName and task callback are still defined, but do nothing. They will be removed opportunistically. New code shouldn't use them.
BUG=164227
TEST=No; this is a code cleanup.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213844
Patch Set 1 #Patch Set 2 : Fixing upload glitch #
Total comments: 4
Patch Set 3 : skare's notes #Patch Set 4 : Fixing branch hierarchy glitch #
Total comments: 6
Patch Set 5 : More notes. #
Total comments: 6
Patch Set 6 : More comments. #
Total comments: 4
Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/19967005/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:105: if (taskPendingCallbackCount == 0) does this need to be kept separate from pendingCallbacks.length? The latter should be fast. As a side effect that would make pendingCallbacks non-debug info of course. https://codereview.chromium.org/19967005/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:297: debugSetStepName: function() {}, // TODO(vadimt): remove nit: 2 spaces between code and comments. Could this be removed now? or makes more sense with a change that just removes the function?
Answered. Note that I've done 2 uploads this time. https://codereview.chromium.org/19967005/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:105: if (taskPendingCallbackCount == 0) On 2013/07/22 23:09:54, Travis Skare wrote: > does this need to be kept separate from pendingCallbacks.length? The latter > should be fast. As a side effect that would make pendingCallbacks non-debug info > of course. pendingCallbacks also includes non-task callbacks. We may want to call finish() even when it's not empty, given that all task callbacks are complete. https://codereview.chromium.org/19967005/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:297: debugSetStepName: function() {}, // TODO(vadimt): remove On 2013/07/22 23:09:54, Travis Skare wrote: > nit: 2 spaces between code and comments. > Could this be removed now? or makes more sense with a change that just removes > the function? Removing this now would break code or would require removing the call from all places, which would create lost of conflicts with pending CLs. I'll choose a quiet moment and remove this method later.
https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:69: * Not yet called required callbacks. Includes both task and non-task read slightly weird on the first pass but I don't necessarily have better suggestions :( Required callbacks not yet called? Required callbacks still to be called? Callbacks remaining in the chain? https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:101: entry.task(function() {}); // TODO(vadimt): Don't pass parameter. tiny nit: 2 spaces between code and comments
Also rebasing. https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:69: * Not yet called required callbacks. Includes both task and non-task On 2013/07/23 20:02:30, Travis Skare wrote: > read slightly weird on the first pass but I don't necessarily have better > suggestions :( > Required callbacks not yet called? > Required callbacks still to be called? > Callbacks remaining in the chain? Done. https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:101: entry.task(function() {}); // TODO(vadimt): Don't pass parameter. On 2013/07/23 20:02:30, Travis Skare wrote: > tiny nit: 2 spaces between code and comments Done.
code looks good, couple of high level comments https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:82: var isInTask = false; maybe "Whether a task is currently running?" "currently executed code" might not be authoritative (e.g. a task makes an async call and execution goes to another event in the queue) https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:210: * @param {boolean=} opt_dontRequire True if the callback is not required to more comments or logic flip could help here, just IMO from my reading since this seems slightly confusing (true to not enforce some requirement). Mostly my first question on reading the comments was "what is 'not required to be invoked'?"
https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:82: var isInTask = false; On 2013/07/23 21:10:52, Travis Skare wrote: > maybe "Whether a task is currently running?" > > "currently executed code" might not be authoritative (e.g. a task makes an async > call and execution goes to another event in the queue) "Whether a task is currently running" may be confusing. It may imply that it's true starting from a moment when a task was started, finishing at the moment when the task is done. But it should NOT be true for non-task callback that get executed between task's start and end. It should be true only when executing task callbacks (and the task bootstrap passed to start() method). This is why I chose this name. https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:210: * @param {boolean=} opt_dontRequire True if the callback is not required to On 2013/07/23 21:10:52, Travis Skare wrote: > more comments or logic flip could help here, just IMO from my reading since this > seems slightly confusing (true to not enforce some requirement). Mostly my first > question on reading the comments was "what is 'not required to be invoked'?" You may call this method without passing this param. I wanted 'undefined' to mean 'required'. Hence the "don't" part.
https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:216: if (isInTask) Why not isTaskCallback here too? https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:382: tasks.debugSetStepName('planForNext-get-storage'); Can we remove from here right now? Or is this also being edited right now?
https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/9001/chrome/browser/resources/g... chrome/browser/resources/google_now/utility.js:216: if (isInTask) On 2013/07/24 19:53:01, rgustafson wrote: > Why not isTaskCallback here too? Done. https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/14001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:382: tasks.debugSetStepName('planForNext-get-storage'); On 2013/07/24 19:53:02, rgustafson wrote: > Can we remove from here right now? Or is this also being edited right now? Done.
lgtm
lgtm
arv@, please provide OWNER's approval
LGTM https://codereview.chromium.org/19967005/diff/22001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:106: if (taskPendingCallbackCount == 0) if (!taskPendingCallbackCount) https://codereview.chromium.org/19967005/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:291: queue.length == 0 && stringifiedPendingCallbacks == '{}', !queue.length
https://codereview.chromium.org/19967005/diff/22001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/19967005/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:106: if (taskPendingCallbackCount == 0) On 2013/07/24 23:10:19, arv wrote: > if (!taskPendingCallbackCount) Respectfully disagree. It's shorter, but makes code less clear. Since this is a numeric value, better not to work with it like with a boolean. https://codereview.chromium.org/19967005/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:291: queue.length == 0 && stringifiedPendingCallbacks == '{}', On 2013/07/24 23:10:19, arv wrote: > !queue.length Same.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/19967005/22001
OK, arv@ is OOF. I'm landing this without making requested changes. I'll follow up with arv@ and other JS style guide authors regarding these comments.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/19967005/22001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/19967005/22001
Message was sent while issue was closed.
Change committed as 213844 |
