Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12316075:
Preventing race conditions in Google Now extension (Closed)
|
Created:
7 years, 10 months ago by vadimt Modified:
7 years, 9 months ago CC:
chromium-reviews, arv+watch_chromium.org, govind1, stromme Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPreventing race conditions in Google Now extension.
Introducing a task manager. A task is an event chain that does certain work that we want to be atomic. Execution of multiple tasks is serialized.
Also, adding checks for anomalous situations, such as a forever-running task keeps the event page from unloading or a task still running when the event page unloads.
BUG=164227
TEST= From the JS console, execute:
updateNotificationsCards(); updateNotificationsCards();
Make sure that the server request gets sent only once.
Now execute:
updateNotificationsCards();
Make sure that the request is sent.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190023
Patch Set 1 #
Total comments: 26
Patch Set 2 : Rebase #Patch Set 3 : skare@ initial comments #Patch Set 4 : Rebase/switch to notifications.clear #
Total comments: 62
Patch Set 5 : More comments #Patch Set 6 : More notes #Patch Set 7 : #Patch Set 8 : #
Total comments: 8
Patch Set 9 : #Patch Set 10 : Removing last reference to chrome.experimental #Patch Set 11 : Fixing incorrect merge #Patch Set 12 : Separating utilities into a separate file. #
Total comments: 32
Patch Set 13 : Getting rid of remaining instances of TaskName. #
Total comments: 8
Patch Set 14 : CR comments after splitting the file. #
Total comments: 14
Patch Set 15 : 3/19 comments #
Total comments: 11
Patch Set 16 : arv@ comments. #
Messages
Total messages: 37 (0 generated)
some early stylistic stuff. Will need a bit more time to dig into the main behavior but this could change several lines so sending now https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:58: var UPDATE_CARDS_TASK_NAME = 'update-cards'; enum? /** @enum {string} */ var Task = { UPDATE_CARDS = 'update-cards', CHECK_AWESOMENESS = 'make-sure-things-are-awesome' }; then you can use Task.UPDATE_CARDS, and pass these around as types. if we ever run through jscompiler, even just for type-checking, we get some benefits too. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:63: * Name of the alarm that triggers updating the cards. probably don't need this comment https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:65: var UPDATE_ALARM_NAME = 'UPDATE'; any advantage to having the two alarm names in the same scope? then you could enum those too. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:70: * Check for internal errors. split into another file? Do we have access to a module loader or is the extension just copied into chrome binaries without modification? https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:88: function getTaskManager() { you might want to just call this function TaskManager since a first read (e.g. mine) is that this is a singleton getter. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:133: task. this line lost an asterisk https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:137: queuedTaskName == UPDATE_CARDS_TASK_NAME) { one more space https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:287: tasks.setStep('parseAndShowNotificationCards-get-active-notifications'); so setStep is purely diagnostic information to know how far along a given task is? https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:438: function(wasDeleted) {}); unneeded param? https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (alarm.name == UPDATE_ALARM_NAME) though I don't think it's in the style guide, you might hear some people request braces around single-line if() statements in JS.
https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:58: var UPDATE_CARDS_TASK_NAME = 'update-cards'; On 2013/02/22 20:53:39, Travis Skare wrote: > enum? > > /** @enum {string} */ > var Task = { > UPDATE_CARDS = 'update-cards', > CHECK_AWESOMENESS = 'make-sure-things-are-awesome' > }; > > then you can use Task.UPDATE_CARDS, and pass these around as types. > > if we ever run through jscompiler, even just for type-checking, we get some > benefits too. Done. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:63: * Name of the alarm that triggers updating the cards. On 2013/02/22 20:53:39, Travis Skare wrote: > probably don't need this comment Done. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:65: var UPDATE_ALARM_NAME = 'UPDATE'; On 2013/02/22 20:53:39, Travis Skare wrote: > any advantage to having the two alarm names in the same scope? then you could > enum those too. We are speaking about UPDATE_NOTIFICATIONS_ALARM_NAME and CANNOT_UNLOAD_ALARM_NAME. They serve different purposes, therefore, cannot be merged to one. UPDATE_NOTIFICATIONS_ALARM_NAME is global, while CANNOT_UNLOAD_ALARM_NAME is local to TaskManager, therefore, they cannot be gathered under one enum. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:70: * Check for internal errors. On 2013/02/22 20:53:39, Travis Skare wrote: > split into another file? Do we have access to a module loader or is the > extension just copied into chrome binaries without modification? This won't be an issue; I've added a comment to do this after we settle with other notes. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:88: function getTaskManager() { On 2013/02/22 20:53:39, Travis Skare wrote: > you might want to just call this function TaskManager since a first read (e.g. > mine) is that this is a singleton getter. Done. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:133: task. On 2013/02/22 20:53:39, Travis Skare wrote: > this line lost an asterisk Done. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:137: queuedTaskName == UPDATE_CARDS_TASK_NAME) { On 2013/02/22 20:53:39, Travis Skare wrote: > one more space Done. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:287: tasks.setStep('parseAndShowNotificationCards-get-active-notifications'); On 2013/02/22 20:53:39, Travis Skare wrote: > so setStep is purely diagnostic information to know how far along a given task > is? Yep. If a task quietly ends without calling finish(), we'll know which step to blame. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:438: function(wasDeleted) {}); On 2013/02/22 20:53:39, Travis Skare wrote: > unneeded param? Done. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (alarm.name == UPDATE_ALARM_NAME) On 2013/02/22 20:53:39, Travis Skare wrote: > though I don't think it's in the style guide, you might hear some people request > braces around single-line if() statements in JS. As far as I recall, if the condition or the statement takes more than 1 line, we need braces, otherwise, we don't need them.
https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); console.assert()? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); I didn't think alert() from a background page works. Might be wrong though.. does it? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:90: * Build the closure that manages tasks (mutually exclusive chains of events). you could omit "build the closure" and just say e.g. "object to manage..." https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:126: // Start the oldest queued task, but not remove it from the queue. nit: s/not/don't https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; maybe bail out if queue is empty here? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:137: * @return {boolean} Whether the new task doesn't conflict with the existing maybe invert logic -- isCompatible -> tasksConflict() or similar to avoid the valid-but-slightly-hard-to-parse "whether X doesn't conflict" language? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:152: * Check if a new task can be added to the task queue. nit (throughout comments in the file): this is in the imperative mood. JS Style (via C++ style) is generally to describe what the method does. here, that'd be "Checks if a new task" so maybe add some s's through the comments :) https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:178: startFirst(); it's ok as used now now, but you're ok with the fact that this function may synchronously start the work (via startFirst, after setting alarm). https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:185: function finish() { high-level comment: a lot of JS patterns end up having a success (and/or error) callback that can be passed around and called by a chain of async functions. Did you consider having a function like that rather than having everything call tasks.finish? Won't go into more detail on potential advantages there in case you considered that.
https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:65: var UPDATE_ALARM_NAME = 'UPDATE'; On 2013/02/28 02:23:22, vadimt wrote: > On 2013/02/22 20:53:39, Travis Skare wrote: > > any advantage to having the two alarm names in the same scope? then you could > > enum those too. > > We are speaking about UPDATE_NOTIFICATIONS_ALARM_NAME and > CANNOT_UNLOAD_ALARM_NAME. They serve different purposes, therefore, cannot be > merged to one. > > UPDATE_NOTIFICATIONS_ALARM_NAME is global, while CANNOT_UNLOAD_ALARM_NAME is > local to TaskManager, therefore, they cannot be gathered under one enum. [ok] yeah, figured this was the case; phrased as "any plans for moving them" on the off chance they could be combined. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (alarm.name == UPDATE_ALARM_NAME) yes that's correct. And I was a member of Team Omit-Braces-For-One-Line. But always including braces seems to be a de facto style rule in some code... not sure within Chrome; I guess we'll see. On 2013/02/28 02:23:22, vadimt wrote: > On 2013/02/22 20:53:39, Travis Skare wrote: > > though I don't think it's in the style guide, you might hear some people > request > > braces around single-line if() statements in JS. > > As far as I recall, if the condition or the statement takes more than 1 line, we > need braces, otherwise, we don't need them.
Sorry about the delay, but you were forewarned. I'll take another close look at it after your revisions to make sure I didn't miss anything. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (alarm.name == UPDATE_ALARM_NAME) "Do not use braces for single-line logic blocks." http://www.chromium.org/developers/web-development-style-guide On 2013/03/02 00:11:39, Travis Skare wrote: > yes that's correct. And I was a member of Team Omit-Braces-For-One-Line. But > always including braces seems to be a de facto style rule in some code... not > sure within Chrome; I guess we'll see. > > On 2013/02/28 02:23:22, vadimt wrote: > > On 2013/02/22 20:53:39, Travis Skare wrote: > > > though I don't think it's in the style guide, you might hear some people > > request > > > braces around single-line if() statements in JS. > > > > As far as I recall, if the condition or the statement takes more than 1 line, > we > > need braces, otherwise, we don't need them. > https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:121: function startFirst() { startFirstTask() makes this more self explanatory imo https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; This seems to be called only when there is a first task. Either check for empty here for complete confidence or at least add to the description that at least one task is a pre-req. On 2013/03/02 00:07:05, Travis Skare wrote: > maybe bail out if queue is empty here? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { Is there a reason you don't use this inside the TaskManager also and opt for 'stepName = '? It makes it a bit harder to find all the places you are setting the step. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:331: request.onerror = function() { Are there any other callbacks we need to handle like ontimeout? perhaps use onloadend again but only finish conditionally on status? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:435: chrome.experimental.notification.clear( This is going to have a notification pop up for a small period of time and then disappear? Seems sloppy. Couldn't we never reshow dismissed notifications? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:483: updateNotificationsCards(); Couldn't retryDelaySeconds not be set when update tries to fetch it now?
https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:65: var UPDATE_ALARM_NAME = 'UPDATE'; On 2013/03/02 00:11:39, Travis Skare wrote: > On 2013/02/28 02:23:22, vadimt wrote: > > On 2013/02/22 20:53:39, Travis Skare wrote: > > > any advantage to having the two alarm names in the same scope? then you > could > > > enum those too. > > > > We are speaking about UPDATE_NOTIFICATIONS_ALARM_NAME and > > CANNOT_UNLOAD_ALARM_NAME. They serve different purposes, therefore, cannot be > > merged to one. > > > > UPDATE_NOTIFICATIONS_ALARM_NAME is global, while CANNOT_UNLOAD_ALARM_NAME is > > local to TaskManager, therefore, they cannot be gathered under one enum. > > [ok] > yeah, figured this was the case; phrased as "any plans for moving them" on the > off chance they could be combined. Done. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (alarm.name == UPDATE_ALARM_NAME) On 2013/03/02 00:11:39, Travis Skare wrote: > yes that's correct. And I was a member of Team Omit-Braces-For-One-Line. But > always including braces seems to be a de facto style rule in some code... not > sure within Chrome; I guess we'll see. > > On 2013/02/28 02:23:22, vadimt wrote: > > On 2013/02/22 20:53:39, Travis Skare wrote: > > > though I don't think it's in the style guide, you might hear some people > > request > > > braces around single-line if() statements in JS. > > > > As far as I recall, if the condition or the statement takes more than 1 line, > we > > need braces, otherwise, we don't need them. > See rgustafson's note. https://codereview.chromium.org/12316075/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:497: if (alarm.name == UPDATE_ALARM_NAME) On 2013/03/04 22:31:27, rgustafson wrote: > "Do not use braces for single-line logic blocks." > http://www.chromium.org/developers/web-development-style-guide > > On 2013/03/02 00:11:39, Travis Skare wrote: > > yes that's correct. And I was a member of Team Omit-Braces-For-One-Line. But > > always including braces seems to be a de facto style rule in some code... not > > sure within Chrome; I guess we'll see. > > > > On 2013/02/28 02:23:22, vadimt wrote: > > > On 2013/02/22 20:53:39, Travis Skare wrote: > > > > though I don't think it's in the style guide, you might hear some people > > > request > > > > braces around single-line if() statements in JS. > > > > > > As far as I recall, if the condition or the statement takes more than 1 > line, > > we > > > need braces, otherwise, we don't need them. > > > Thanks rgustafson@! https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); On 2013/03/02 00:07:05, Travis Skare wrote: > console.assert()? Then no one will notice it. I'd prefer these important internal errors to be more prominent. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); On 2013/03/02 00:07:05, Travis Skare wrote: > I didn't think alert() from a background page works. Might be wrong though.. > does it? We checked that it works, remember :) ? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:90: * Build the closure that manages tasks (mutually exclusive chains of events). On 2013/03/02 00:07:05, Travis Skare wrote: > you could omit "build the closure" and just say e.g. "object to manage..." Done. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:121: function startFirst() { On 2013/03/04 22:31:27, rgustafson wrote: > startFirstTask() makes this more self explanatory imo The idea is that these are members of TaskManager, so basically, this is TaskManager::startFirst(). So, I didn't include 'task' word in method names. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:126: // Start the oldest queued task, but not remove it from the queue. On 2013/03/02 00:07:05, Travis Skare wrote: > nit: s/not/don't Done. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; On 2013/03/02 00:07:05, Travis Skare wrote: > maybe bail out if queue is empty here? The method will be called only when the queue is not empty. I've added a comment the this is a pre-req. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; On 2013/03/04 22:31:27, rgustafson wrote: > This seems to be called only when there is a first task. Either check for empty > here for complete confidence or at least add to the description that at least > one task is a pre-req. > On 2013/03/02 00:07:05, Travis Skare wrote: > > maybe bail out if queue is empty here? > Done. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; On 2013/03/04 22:31:27, rgustafson wrote: > This seems to be called only when there is a first task. Either check for empty > here for complete confidence or at least add to the description that at least > one task is a pre-req. > On 2013/03/02 00:07:05, Travis Skare wrote: > > maybe bail out if queue is empty here? > Done. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:137: * @return {boolean} Whether the new task doesn't conflict with the existing On 2013/03/02 00:07:05, Travis Skare wrote: > maybe invert logic -- isCompatible -> tasksConflict() or similar to avoid the > valid-but-slightly-hard-to-parse "whether X doesn't conflict" language? Done. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:152: * Check if a new task can be added to the task queue. On 2013/03/02 00:07:05, Travis Skare wrote: > nit (throughout comments in the file): this is in the imperative mood. JS Style > (via C++ style) is generally to describe what the method does. > > here, that'd be "Checks if a new task" > > so maybe add some s's through the comments :) Done. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:178: startFirst(); On 2013/03/02 00:07:05, Travis Skare wrote: > it's ok as used now now, but you're ok with the fact that this function may > synchronously start the work (via startFirst, after setting alarm). Yes, I think that it's better than starting asynchronously, which itself is an async operation and requires care. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:185: function finish() { On 2013/03/02 00:07:05, Travis Skare wrote: > high-level comment: a lot of JS patterns end up having a success (and/or error) > callback that can be passed around and called by a chain of async functions. > > Did you consider having a function like that rather than having everything call > tasks.finish? Won't go into more detail on potential advantages there in case > you considered that. I've considered this. I thought that this would be more beautiful from the design standpoint, but would add callback params to everything. With the current state of the code, it seems more reasonable for me to have explicit finish() calls. And, also, we have to call tasks.setStep explicitly... https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { On 2013/03/04 22:31:27, rgustafson wrote: > Is there a reason you don't use this inside the TaskManager also and opt for > 'stepName = '? It makes it a bit harder to find all the places you are setting > the step. One day, for example, I may want to add a "stepName != null" assert. This won't work for the initial assignment. In general, I believe that setting fields directly from class' methods is OK. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:331: request.onerror = function() { On 2013/03/04 22:31:27, rgustafson wrote: > Are there any other callbacks we need to handle like ontimeout? perhaps use > onloadend again but only finish conditionally on status? Done. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:435: chrome.experimental.notification.clear( On 2013/03/04 22:31:27, rgustafson wrote: > This is going to have a notification pop up for a small period of time and then > disappear? Seems sloppy. Couldn't we never reshow dismissed notifications? The reasoning for not doing this is that I expect this to happen extremely rarely. I could add a logic that would try to reduce likelihood of this, but I feel that complicating the code is not worth the outcome in this case. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:483: updateNotificationsCards(); On 2013/03/04 22:31:27, rgustafson wrote: > Couldn't retryDelaySeconds not be set when update tries to fetch it now? I had a long conversation with extensions guys who promised me that this won't happen. They'll document this, and add a test.
cool, hope the comment changes weren't too painful. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); [ok] yes, but that was after this set of comments went out. On 2013/03/06 21:04:38, vadimt wrote: > On 2013/03/02 00:07:05, Travis Skare wrote: > > I didn't think alert() from a background page works. Might be wrong though.. > > does it? > > We checked that it works, remember :) ? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; I still like bailing out on an empty queue (or like rgustafson says, enforce the constraint via a code path) in case someone didn't read the comments for every function he/she calls; otherwise the function will end halfway through with an exception after changing state (i.e. setting an alarm) On 2013/03/06 21:04:38, vadimt wrote: > On 2013/03/02 00:07:05, Travis Skare wrote: > > maybe bail out if queue is empty here? > > The method will be called only when the queue is not empty. I've added a comment > the this is a pre-req. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:185: function finish() { feel free to get other people to comment here but I'll read the use of start/finish again after responding to the more immediately useful patch reviews On 2013/03/06 21:04:38, vadimt wrote: > On 2013/03/02 00:07:05, Travis Skare wrote: > > high-level comment: a lot of JS patterns end up having a success (and/or > error) > > callback that can be passed around and called by a chain of async functions. > > > > Did you consider having a function like that rather than having everything > call > > tasks.finish? Won't go into more detail on potential advantages there in case > > you considered that. > > I've considered this. I thought that this would be more beautiful from the > design standpoint, but would add callback params to everything. With the current > state of the code, it seems more reasonable for me to have explicit finish() > calls. > And, also, we have to call tasks.setStep explicitly... https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { more general question -- could state name be instrumented into private debugging builds as needed, getting rid of these calls in the production code? Does having access to call chains at various points help? Are we expecting to need to read these strings frequently? also, setStep -> setStepName or some such? "setStep" seems like it might do more than set a debug string On 2013/03/06 21:04:38, vadimt wrote: > On 2013/03/04 22:31:27, rgustafson wrote: > > Is there a reason you don't use this inside the TaskManager also and opt for > > 'stepName = '? It makes it a bit harder to find all the places you are setting > > the step. > > One day, for example, I may want to add a "stepName != null" assert. This won't > work for the initial assignment. > In general, I believe that setting fields directly from class' methods is OK.
https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); On 2013/03/06 22:35:57, Travis Skare wrote: > [ok] > yes, but that was after this set of comments went out. > > > On 2013/03/06 21:04:38, vadimt wrote: > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > I didn't think alert() from a background page works. Might be wrong though.. > > > does it? > > > > We checked that it works, remember :) ? > No doubt about that. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:185: function finish() { On 2013/03/06 22:35:57, Travis Skare wrote: > feel free to get other people to comment here but I'll read the use of > start/finish again after responding to the more immediately useful patch reviews > > On 2013/03/06 21:04:38, vadimt wrote: > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > high-level comment: a lot of JS patterns end up having a success (and/or > > error) > > > callback that can be passed around and called by a chain of async functions. > > > > > > Did you consider having a function like that rather than having everything > > call > > > tasks.finish? Won't go into more detail on potential advantages there in > case > > > you considered that. > > > > I've considered this. I thought that this would be more beautiful from the > > design standpoint, but would add callback params to everything. With the > current > > state of the code, it seems more reasonable for me to have explicit finish() > > calls. > > And, also, we have to call tasks.setStep explicitly... > Done. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { On 2013/03/06 22:35:57, Travis Skare wrote: > more general question -- could state name be instrumented into private debugging > builds as needed, getting rid of these calls in the production code? Does having > access to call chains at various points help? Are we expecting to need to read > these strings frequently? > > also, setStep -> setStepName or some such? "setStep" seems like it might do more > than set a debug string > > On 2013/03/06 21:04:38, vadimt wrote: > > On 2013/03/04 22:31:27, rgustafson wrote: > > > Is there a reason you don't use this inside the TaskManager also and opt for > > > 'stepName = '? It makes it a bit harder to find all the places you are > setting > > > the step. > > > > One day, for example, I may want to add a "stepName != null" assert. This > won't > > work for the initial assignment. > > In general, I believe that setting fields directly from class' methods is OK. > The plan about this is: in the release, we'll probably have a UMA counter per each step name. We'll increment that counter if something went wrong during that step. The code will evolve to that state. In addition to this, having these calls only in debugging builds would only increase the integration tax (== hell), I'd prefer to avoid this.
'done' stuff looks good in general, just bringing a couple of points from previous patchsets forward: https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); On 2013/03/02 00:07:05, Travis Skare wrote: > console.assert()? bringing this comment back from the dead. Would console.assert() work instead of console.error? Even supports a text argument like the above. Or even use console.assert() instead of this function altogether--do we really want alerts bugging people, especially since the flag that would let you stop the extension (enable debugging of component extensions) is different from the one that lets you turn it on (enable chrome now)? Also I know this is isolated but consider not having this be something called assert() that does something slightly different from a standard function like console.assert() https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:107: * currently running task. @type this -- {Array.<Task>} // where you'd typedef, or {Array.<Object.<TaskName, function()>>} I think? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; On 2013/03/06 22:35:57, Travis Skare wrote: > I still like bailing out on an empty queue (or like rgustafson says, enforce the > constraint via a code path) in case someone didn't read the comments for every > function he/she calls; otherwise the function will end halfway through with an > exception after changing state (i.e. setting an alarm) > > On 2013/03/06 21:04:38, vadimt wrote: > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > maybe bail out if queue is empty here? > > > > The method will be called only when the queue is not empty. I've added a > comment > > the this is a pre-req. > [bringing this into latest round of comments too] https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { On 2013/03/06 22:56:35, vadimt wrote: > On 2013/03/06 22:35:57, Travis Skare wrote: > > more general question -- could state name be instrumented into private > debugging > > builds as needed, getting rid of these calls in the production code? Does > having > > access to call chains at various points help? Are we expecting to need to read > > these strings frequently? > > > > also, setStep -> setStepName or some such? "setStep" seems like it might do > more > > than set a debug string > > > > On 2013/03/06 21:04:38, vadimt wrote: > > > On 2013/03/04 22:31:27, rgustafson wrote: > > > > Is there a reason you don't use this inside the TaskManager also and opt > for > > > > 'stepName = '? It makes it a bit harder to find all the places you are > > setting > > > > the step. > > > > > > One day, for example, I may want to add a "stepName != null" assert. This > > won't > > > work for the initial assignment. > > > In general, I believe that setting fields directly from class' methods is > OK. > > > > The plan about this is: > in the release, we'll probably have a UMA counter per each step name. We'll > increment that counter if something went wrong during that step. The code will > evolve to that state. > > In addition to this, having these calls only in debugging builds would only > increase the integration tax (== hell), I'd prefer to avoid this. would UMA error counters be slightly more granular than step name? overall -- if you expect the traces to be useful in the future, ok, but we instrumented/added breakpoints after the fact to debug the G+ extensions and it worked out pretty well. You might have enough info between queue[0].name and the call stack to debug this on local machines. setStep() makes things very slightly heavier to read, possibly because task status seems more tied to a task than its parent collection class.
https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); On 2013/03/07 23:25:13, Travis Skare wrote: > On 2013/03/02 00:07:05, Travis Skare wrote: > > console.assert()? > > bringing this comment back from the dead. Would console.assert() work instead of > console.error? Even supports a text argument like the above. > > Or even use console.assert() instead of this function altogether--do we really > want alerts bugging people, especially since the flag that would let you stop > the extension (enable debugging of component extensions) is different from the > one that lets you turn it on (enable chrome now)? > > Also I know this is isolated but consider not having this be something called > assert() that does something slightly different from a standard function like > console.assert() Renamed assert to verify. This assert is for things that should absolutely not happen, and if this happens, we are in a big trouble (note that this will be removed before the release). So I want our testers to know about such events immediately. console.assert doesn't show a popup. So, I cannot use it here instead of alert. Now that we have to check the condition anyways, console.error seems adequate. I don't expect these assert to bug people a lot because (see above) this absolutely shouldn't happen. People will see and report to us the additional information included in the assert, which will help to identify issues. So, showing asserts makes sense even when debugging component extensions is off (note that early users often won't use the component extension, but use the CRX extension). https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:107: * currently running task. On 2013/03/07 23:25:13, Travis Skare wrote: > @type this -- > {Array.<Task>} // where you'd typedef, or > {Array.<Object.<TaskName, function()>>} > I think? Coool https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; On 2013/03/07 23:25:13, Travis Skare wrote: > On 2013/03/06 22:35:57, Travis Skare wrote: > > I still like bailing out on an empty queue (or like rgustafson says, enforce > the > > constraint via a code path) in case someone didn't read the comments for every > > function he/she calls; otherwise the function will end halfway through with an > > exception after changing state (i.e. setting an alarm) > > > > On 2013/03/06 21:04:38, vadimt wrote: > > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > > maybe bail out if queue is empty here? > > > > > > The method will be called only when the queue is not empty. I've added a > > comment > > > the this is a pre-req. > > > > [bringing this into latest round of comments too] Per the offline talk, seems we are proceeding as is? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { > would UMA error counters be slightly more granular than step name? May be, yes, may be, no. We'll have to decide then. > > overall -- if you expect the traces to be useful in the future, ok, but we > instrumented/added breakpoints after the fact to debug the G+ extensions and it > worked out pretty well. You might have enough info between queue[0].name and the > call stack to debug this on local machines. One situation is when the even page unloads, and there are incomplete tasks. This may mean that the event chain was broken. We want (1) a popup appear (see 'verify()' implementation); (2) this popup should contain the step name to indicate where the event chain was broken. If we don't set step names, we won't know what step to blame. > > setStep() makes things very slightly heavier to read, possibly because task > status seems more tied to a task than its parent collection class. Not sure I've understood this completely (about the parent collection class), but I agree that setStepName makes the code heavier. However, I believe that this is an absolutely necessary thing to catch hard-to-repro sutuations.
https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); If it's stuff that should never happen and this will be removed before release, why isn't this just contained in tests? Those tests will continue to exist past release, so you can continue to ensure that problems aren't coming up. On 2013/03/08 21:11:58, vadimt wrote: > On 2013/03/07 23:25:13, Travis Skare wrote: > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > console.assert()? > > > > bringing this comment back from the dead. Would console.assert() work instead > of > > console.error? Even supports a text argument like the above. > > > > Or even use console.assert() instead of this function altogether--do we really > > want alerts bugging people, especially since the flag that would let you stop > > the extension (enable debugging of component extensions) is different from the > > one that lets you turn it on (enable chrome now)? > > > > Also I know this is isolated but consider not having this be something called > > assert() that does something slightly different from a standard function like > > console.assert() > > Renamed assert to verify. > > This assert is for things that should absolutely not happen, and if this > happens, we are in a big trouble (note that this will be removed before the > release). So I want our testers to know about such events immediately. > console.assert doesn't show a popup. So, I cannot use it here instead of alert. > > Now that we have to check the condition anyways, console.error seems adequate. > > I don't expect these assert to bug people a lot because (see above) this > absolutely shouldn't happen. > > People will see and report to us the additional information included in the > assert, which will help to identify issues. So, showing asserts makes sense even > when debugging component extensions is off (note that early users often won't > use the component extension, but use the CRX extension). https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { Are we really expecting our event page being improperly evicted being a problem? I know that we had a problem with the location api, but that was also not some corner case that was difficult to catch. Are there some other examples of when this would be useful that can't be caught in tests? On 2013/03/08 21:11:58, vadimt wrote: > > would UMA error counters be slightly more granular than step name? > May be, yes, may be, no. We'll have to decide then. > > > > > overall -- if you expect the traces to be useful in the future, ok, but we > > instrumented/added breakpoints after the fact to debug the G+ extensions and > it > > worked out pretty well. You might have enough info between queue[0].name and > the > > call stack to debug this on local machines. > One situation is when the even page unloads, and there are incomplete tasks. > This may mean that the event chain was broken. We want (1) a popup appear (see > 'verify()' implementation); (2) this popup should contain the step name to > indicate where the event chain was broken. If we don't set step names, we won't > know what step to blame. > > > > > setStep() makes things very slightly heavier to read, possibly because task > > status seems more tied to a task than its parent collection class. > Not sure I've understood this completely (about the parent collection class), > but I agree that setStepName makes the code heavier. However, I believe that > this is an absolutely necessary thing to catch hard-to-repro sutuations. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:435: chrome.experimental.notification.clear( I'm okay with this for now. Eventually, I think the complexity may be worth ridding our users of weird behavior. On 2013/03/06 21:04:38, vadimt wrote: > On 2013/03/04 22:31:27, rgustafson wrote: > > This is going to have a notification pop up for a small period of time and > then > > disappear? Seems sloppy. Couldn't we never reshow dismissed notifications? > > The reasoning for not doing this is that I expect this to happen extremely > rarely. I could add a logic that would try to reduce likelihood of this, but I > feel that complicating the code is not worth the outcome in this case. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:483: updateNotificationsCards(); What exactly was their promise? A set followed by a get will always include the new set? Or something more/less specific than that? On 2013/03/06 21:04:38, vadimt wrote: > On 2013/03/04 22:31:27, rgustafson wrote: > > Couldn't retryDelaySeconds not be set when update tries to fetch it now? > > I had a long conversation with extensions guys who promised me that this won't > happen. They'll document this, and add a test. https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:143: function doConflict(newTaskName, queuedTaskName) { Things starting with do generally mean taking action rather than coming up with a boolean value. isConflicting hasConflict or just forget the verb with tasksConflict
some more high-level stuff https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); re: the alert() line, would it be ok to not show alerts to users from the ether without some opt-in (special extension, localStorage flag)? I think we talked about this in person briefly and there's some mention upthread too, just asking explicitly here. On 2013/03/11 18:11:04, rgustafson wrote: > If it's stuff that should never happen and this will be removed before release, > why isn't this just contained in tests? Those tests will continue to exist past > release, so you can continue to ensure that problems aren't coming up. > > On 2013/03/08 21:11:58, vadimt wrote: > > On 2013/03/07 23:25:13, Travis Skare wrote: > > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > > console.assert()? > > > > > > bringing this comment back from the dead. Would console.assert() work > instead > > of > > > console.error? Even supports a text argument like the above. > > > > > > Or even use console.assert() instead of this function altogether--do we > really > > > want alerts bugging people, especially since the flag that would let you > stop > > > the extension (enable debugging of component extensions) is different from > the > > > one that lets you turn it on (enable chrome now)? > > > > > > Also I know this is isolated but consider not having this be something > called > > > assert() that does something slightly different from a standard function > like > > > console.assert() > > > > Renamed assert to verify. > > > > This assert is for things that should absolutely not happen, and if this > > happens, we are in a big trouble (note that this will be removed before the > > release). So I want our testers to know about such events immediately. > > console.assert doesn't show a popup. So, I cannot use it here instead of > alert. > > > > Now that we have to check the condition anyways, console.error seems adequate. > > > > I don't expect these assert to bug people a lot because (see above) this > > absolutely shouldn't happen. > > > > People will see and report to us the additional information included in the > > assert, which will help to identify issues. So, showing asserts makes sense > even > > when debugging component extensions is off (note that early users often won't > > use the component extension, but use the CRX extension). > https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; so long as whatever the assert() is replaced with handles the failure gracefully -- ideally by early-return and possibly adding to an error stat... prefer not to depend on exceptions logged to the console for control flow. On 2013/03/08 21:11:58, vadimt wrote: > On 2013/03/07 23:25:13, Travis Skare wrote: > > On 2013/03/06 22:35:57, Travis Skare wrote: > > > I still like bailing out on an empty queue (or like rgustafson says, enforce > > the > > > constraint via a code path) in case someone didn't read the comments for > every > > > function he/she calls; otherwise the function will end halfway through with > an > > > exception after changing state (i.e. setting an alarm) > > > > > > On 2013/03/06 21:04:38, vadimt wrote: > > > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > > > maybe bail out if queue is empty here? > > > > > > > > The method will be called only when the queue is not empty. I've added a > > > comment > > > > the this is a pre-req. > > > > > > > [bringing this into latest round of comments too] > > Per the offline talk, seems we are proceeding as is? https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { re: 'parent name' I meant hypothetically myTask.setInfo(debugString) might be clearer than taskManager.setStep('') but we don't have properties of task objects like that. overall setStep() seems slightly obtuse to read (for me at least) just because it appears at somewhat random places in the code (legitimately), and the name seems like it should have side effects rather than setting a debug string. I'd still suggest instrumenting later or using private builds vs. shipping this, but again you're the expert on what can go wrong. On 2013/03/08 21:11:58, vadimt wrote: > > would UMA error counters be slightly more granular than step name? > May be, yes, may be, no. We'll have to decide then. > > > > > overall -- if you expect the traces to be useful in the future, ok, but we > > instrumented/added breakpoints after the fact to debug the G+ extensions and > it > > worked out pretty well. You might have enough info between queue[0].name and > the > > call stack to debug this on local machines. > One situation is when the even page unloads, and there are incomplete tasks. > This may mean that the event chain was broken. We want (1) a popup appear (see > 'verify()' implementation); (2) this popup should contain the step name to > indicate where the event chain was broken. If we don't set step names, we won't > know what step to blame. > > > > > setStep() makes things very slightly heavier to read, possibly because task > > status seems more tied to a task than its parent collection class. > Not sure I've understood this completely (about the parent collection class), > but I agree that setStepName makes the code heavier. However, I believe that > this is an absolutely necessary thing to catch hard-to-repro sutuations. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:435: chrome.experimental.notification.clear( especially if there's ever the ability to have the notifications center trigger sound, or for priority 1/2 notifications. On 2013/03/11 18:11:05, rgustafson wrote: > I'm okay with this for now. Eventually, I think the complexity may be worth > ridding our users of weird behavior. > On 2013/03/06 21:04:38, vadimt wrote: > > On 2013/03/04 22:31:27, rgustafson wrote: > > > This is going to have a notification pop up for a small period of time and > > then > > > disappear? Seems sloppy. Couldn't we never reshow dismissed notifications? > > > > The reasoning for not doing this is that I expect this to happen extremely > > rarely. I could add a logic that would try to reduce likelihood of this, but I > > feel that complicating the code is not worth the outcome in this case. >
https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { ignore setStep() naming comment; resolved in next patch. On 2013/03/11 18:37:12, Travis Skare wrote: > re: 'parent name' I meant hypothetically > myTask.setInfo(debugString) > might be clearer than > taskManager.setStep('') > but we don't have properties of task objects like that. > > overall setStep() seems slightly obtuse to read (for me at least) just because > it appears at somewhat random places in the code (legitimately), and the name > seems like it should have side effects rather than setting a debug string. I'd > still suggest instrumenting later or using private builds vs. shipping this, but > again you're the expert on what can go wrong. > > > On 2013/03/08 21:11:58, vadimt wrote: > > > would UMA error counters be slightly more granular than step name? > > May be, yes, may be, no. We'll have to decide then. > > > > > > > > overall -- if you expect the traces to be useful in the future, ok, but we > > > instrumented/added breakpoints after the fact to debug the G+ extensions and > > it > > > worked out pretty well. You might have enough info between queue[0].name and > > the > > > call stack to debug this on local machines. > > One situation is when the even page unloads, and there are incomplete tasks. > > This may mean that the event chain was broken. We want (1) a popup appear (see > > 'verify()' implementation); (2) this popup should contain the step name to > > indicate where the event chain was broken. If we don't set step names, we > won't > > know what step to blame. > > > > > > > > setStep() makes things very slightly heavier to read, possibly because task > > > status seems more tied to a task than its parent collection class. > > Not sure I've understood this completely (about the parent collection class), > > but I agree that setStepName makes the code heavier. However, I believe that > > this is an absolutely necessary thing to catch hard-to-repro sutuations. > https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:170: * task. Otherwise, store the task for future execution. silently ignoring the task will always be ok? seems like it, just asking.
ok, looking good overall. maybe one high-level thing for in-person discussion, but I think I can take a break for the Task functionality to be pulled to another file (will wait for other reviewers to say the same). https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:237: * Shows a notification and remember information associated with it. tiny nit: s/remember/remembers https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:408: tasks.finish(); would it be useful in the future to have finish() accept an error object/string?
https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); On 2013/03/11 18:11:04, rgustafson wrote: > If it's stuff that should never happen and this will be removed before release, > why isn't this just contained in tests? Those tests will continue to exist past > release, so you can continue to ensure that problems aren't coming up. > > On 2013/03/08 21:11:58, vadimt wrote: > > On 2013/03/07 23:25:13, Travis Skare wrote: > > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > > console.assert()? > > > > > > bringing this comment back from the dead. Would console.assert() work > instead > > of > > > console.error? Even supports a text argument like the above. > > > > > > Or even use console.assert() instead of this function altogether--do we > really > > > want alerts bugging people, especially since the flag that would let you > stop > > > the extension (enable debugging of component extensions) is different from > the > > > one that lets you turn it on (enable chrome now)? > > > > > > Also I know this is isolated but consider not having this be something > called > > > assert() that does something slightly different from a standard function > like > > > console.assert() > > > > Renamed assert to verify. > > > > This assert is for things that should absolutely not happen, and if this > > happens, we are in a big trouble (note that this will be removed before the > > release). So I want our testers to know about such events immediately. > > console.assert doesn't show a popup. So, I cannot use it here instead of > alert. > > > > Now that we have to check the condition anyways, console.error seems adequate. > > > > I don't expect these assert to bug people a lot because (see above) this > > absolutely shouldn't happen. > > > > People will see and report to us the additional information included in the > > assert, which will help to identify issues. So, showing asserts makes sense > even > > when debugging component extensions is off (note that early users often won't > > use the component extension, but use the CRX extension). > The assert shouldn't happen in a different sense :) Asserts stand for error condition that shouldn't happen if there were no bugs. We need this code to catch bugs. If we move it to tests, we won't learn about bugs in real code. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:85: alert(errorText); On 2013/03/11 18:37:12, Travis Skare wrote: > re: the alert() line, would it be ok to not show alerts to users from the ether > without some opt-in (special extension, localStorage flag)? > > I think we talked about this in person briefly and there's some mention upthread > too, just asking explicitly here. > > On 2013/03/11 18:11:04, rgustafson wrote: > > If it's stuff that should never happen and this will be removed before > release, > > why isn't this just contained in tests? Those tests will continue to exist > past > > release, so you can continue to ensure that problems aren't coming up. > > > > On 2013/03/08 21:11:58, vadimt wrote: > > > On 2013/03/07 23:25:13, Travis Skare wrote: > > > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > > > console.assert()? > > > > > > > > bringing this comment back from the dead. Would console.assert() work > > instead > > > of > > > > console.error? Even supports a text argument like the above. > > > > > > > > Or even use console.assert() instead of this function altogether--do we > > really > > > > want alerts bugging people, especially since the flag that would let you > > stop > > > > the extension (enable debugging of component extensions) is different from > > the > > > > one that lets you turn it on (enable chrome now)? > > > > > > > > Also I know this is isolated but consider not having this be something > > called > > > > assert() that does something slightly different from a standard function > > like > > > > console.assert() > > > > > > Renamed assert to verify. > > > > > > This assert is for things that should absolutely not happen, and if this > > > happens, we are in a big trouble (note that this will be removed before the > > > release). So I want our testers to know about such events immediately. > > > console.assert doesn't show a popup. So, I cannot use it here instead of > > alert. > > > > > > Now that we have to check the condition anyways, console.error seems > adequate. > > > > > > I don't expect these assert to bug people a lot because (see above) this > > > absolutely shouldn't happen. > > > > > > People will see and report to us the additional information included in the > > > assert, which will help to identify issues. So, showing asserts makes sense > > even > > > when debugging component extensions is off (note that early users often > won't > > > use the component extension, but use the CRX extension). > > > RE: re: the alert() line Per offline talk, leaving alert for now. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:128: var entry = queue[0]; On 2013/03/11 18:37:12, Travis Skare wrote: > so long as whatever the assert() is replaced with handles the failure gracefully > -- ideally by early-return and possibly adding to an error stat... prefer not to > depend on exceptions logged to the console for control flow. > > On 2013/03/08 21:11:58, vadimt wrote: > > On 2013/03/07 23:25:13, Travis Skare wrote: > > > On 2013/03/06 22:35:57, Travis Skare wrote: > > > > I still like bailing out on an empty queue (or like rgustafson says, > enforce > > > the > > > > constraint via a code path) in case someone didn't read the comments for > > every > > > > function he/she calls; otherwise the function will end halfway through > with > > an > > > > exception after changing state (i.e. setting an alarm) > > > > > > > > On 2013/03/06 21:04:38, vadimt wrote: > > > > > On 2013/03/02 00:07:05, Travis Skare wrote: > > > > > > maybe bail out if queue is empty here? > > > > > > > > > > The method will be called only when the queue is not empty. I've added a > > > > comment > > > > > the this is a pre-req. > > > > > > > > > > [bringing this into latest round of comments too] > > > > Per the offline talk, seems we are proceeding as is? > Added to verify(): // TODO(vadimt): Make sure the execution doesn't continue after this call. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { On 2013/03/11 18:11:05, rgustafson wrote: > Are we really expecting our event page being improperly evicted being a problem? > I know that we had a problem with the location api, but that was also not some > corner case that was difficult to catch. Are there some other examples of when > this would be useful that can't be caught in tests? > > On 2013/03/08 21:11:58, vadimt wrote: > > > would UMA error counters be slightly more granular than step name? > > May be, yes, may be, no. We'll have to decide then. > > > > > > > > overall -- if you expect the traces to be useful in the future, ok, but we > > > instrumented/added breakpoints after the fact to debug the G+ extensions and > > it > > > worked out pretty well. You might have enough info between queue[0].name and > > the > > > call stack to debug this on local machines. > > One situation is when the even page unloads, and there are incomplete tasks. > > This may mean that the event chain was broken. We want (1) a popup appear (see > > 'verify()' implementation); (2) this popup should contain the step name to > > indicate where the event chain was broken. If we don't set step names, we > won't > > know what step to blame. > > > > > > > > setStep() makes things very slightly heavier to read, possibly because task > > > status seems more tied to a task than its parent collection class. > > Not sure I've understood this completely (about the parent collection class), > > but I agree that setStepName makes the code heavier. However, I believe that > > this is an absolutely necessary thing to catch hard-to-repro sutuations. > If I knew situations that cannot be caught with tests, I would write tests for them :) We check 2 things: 1. Event page is unloaded, and a task is still running. This means that the chain of event was broken, and the task didn't get to the finish() call. We must have an assert for this. 2. Event page doesn't get unloaded in a reasonable time. This likely means that the task or task manager is in an infinitely looping chain of events. Both situations are so important that we need to show alert in before-release versions, and increase UMA counters after the release. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { On 2013/03/11 18:37:12, Travis Skare wrote: > re: 'parent name' I meant hypothetically > myTask.setInfo(debugString) > might be clearer than > taskManager.setStep('') > but we don't have properties of task objects like that. > > overall setStep() seems slightly obtuse to read (for me at least) just because > it appears at somewhat random places in the code (legitimately), and the name > seems like it should have side effects rather than setting a debug string. I'd > still suggest instrumenting later or using private builds vs. shipping this, but > again you're the expert on what can go wrong. > > > On 2013/03/08 21:11:58, vadimt wrote: > > > would UMA error counters be slightly more granular than step name? > > May be, yes, may be, no. We'll have to decide then. > > > > > > > > overall -- if you expect the traces to be useful in the future, ok, but we > > > instrumented/added breakpoints after the fact to debug the G+ extensions and > > it > > > worked out pretty well. You might have enough info between queue[0].name and > > the > > > call stack to debug this on local machines. > > One situation is when the even page unloads, and there are incomplete tasks. > > This may mean that the event chain was broken. We want (1) a popup appear (see > > 'verify()' implementation); (2) this popup should contain the step name to > > indicate where the event chain was broken. If we don't set step names, we > won't > > know what step to blame. > > > > > > > > setStep() makes things very slightly heavier to read, possibly because task > > > status seems more tied to a task than its parent collection class. > > Not sure I've understood this completely (about the parent collection class), > > but I agree that setStepName makes the code heavier. However, I believe that > > this is an absolutely necessary thing to catch hard-to-repro sutuations. > RE: re: 'parent name' I meant hypothetically In this case, we want this code to evolve into something that we ship. Agreed, these calls don't make code more beautiful, but this is how things often have to be done in this imperfect world... For now, renaming setStepName into debugSetStepName. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:200: function setStep(step) { On 2013/03/11 19:46:26, Travis Skare wrote: > ignore setStep() naming comment; resolved in next patch. > > On 2013/03/11 18:37:12, Travis Skare wrote: > > re: 'parent name' I meant hypothetically > > myTask.setInfo(debugString) > > might be clearer than > > taskManager.setStep('') > > but we don't have properties of task objects like that. > > > > overall setStep() seems slightly obtuse to read (for me at least) just because > > it appears at somewhat random places in the code (legitimately), and the name > > seems like it should have side effects rather than setting a debug string. I'd > > still suggest instrumenting later or using private builds vs. shipping this, > but > > again you're the expert on what can go wrong. > > > > > > On 2013/03/08 21:11:58, vadimt wrote: > > > > would UMA error counters be slightly more granular than step name? > > > May be, yes, may be, no. We'll have to decide then. > > > > > > > > > > > overall -- if you expect the traces to be useful in the future, ok, but we > > > > instrumented/added breakpoints after the fact to debug the G+ extensions > and > > > it > > > > worked out pretty well. You might have enough info between queue[0].name > and > > > the > > > > call stack to debug this on local machines. > > > One situation is when the even page unloads, and there are incomplete tasks. > > > This may mean that the event chain was broken. We want (1) a popup appear > (see > > > 'verify()' implementation); (2) this popup should contain the step name to > > > indicate where the event chain was broken. If we don't set step names, we > > won't > > > know what step to blame. > > > > > > > > > > > setStep() makes things very slightly heavier to read, possibly because > task > > > > status seems more tied to a task than its parent collection class. > > > Not sure I've understood this completely (about the parent collection > class), > > > but I agree that setStepName makes the code heavier. However, I believe that > > > this is an absolutely necessary thing to catch hard-to-repro sutuations. > > > (In a gloom voice:) Too late. Already renamed to debugSetStepName, which is quite adequate to the current scenario. Will rename again as the semantics changes. https://codereview.chromium.org/12316075/diff/9002/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:483: updateNotificationsCards(); On 2013/03/11 18:11:05, rgustafson wrote: > What exactly was their promise? A set followed by a get will always include the > new set? Or something more/less specific than that? > On 2013/03/06 21:04:38, vadimt wrote: > > On 2013/03/04 22:31:27, rgustafson wrote: > > > Couldn't retryDelaySeconds not be set when update tries to fetch it now? > > > > I had a long conversation with extensions guys who promised me that this won't > > happen. They'll document this, and add a test. > Literally: If get is executed after set(X), 'get' will receive X. From another mail: set(1); get; set(2); get-callback; will feed get-callback with 1. =================================== From another mail: the behavior that was confirmed by them: 1. We use the fact that you can 'get' the value set by 'set' without waiting for 'set's callback. This is not cutting a corner: function eventHandler1() { set() } function eventHandler2() { get() } Assuming that any of these event handlers can be executed at any moment, it really helps to know that eventHandler2() following eventHandler1() gets a correct value even if it gets executed before set's callback. https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:143: function doConflict(newTaskName, queuedTaskName) { On 2013/03/11 18:11:05, rgustafson wrote: > Things starting with do generally mean taking action rather than coming up with > a boolean value. > > isConflicting > hasConflict > or just forget the verb with tasksConflict Done. https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:170: * task. Otherwise, store the task for future execution. On 2013/03/11 19:46:26, Travis Skare wrote: > silently ignoring the task will always be ok? > seems like it, just asking. TaskManager::areConflicting() returns true if: (1) It's necessary to ignore a task; AND (2) It's safe to ignore this task. So, yes. https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:237: * Shows a notification and remember information associated with it. On 2013/03/11 19:53:07, Travis Skare wrote: > tiny nit: s/remember/remembers Done. https://codereview.chromium.org/12316075/diff/24001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:408: tasks.finish(); On 2013/03/11 19:53:07, Travis Skare wrote: > would it be useful in the future to have finish() accept an error object/string? At this point, it doesn't seem so. finish() is to tell the task manager to start next task, and for this it doesn't need to know how happy the previous task was.
Moved utilities into a separate file. Please review.
thanks! https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:28: any reason for just one line break between all of these? https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:67: * Checks if a new task can't be added to a task queue that contains an check comment -- this function doesn't really know about queues. "Checks if two tasks cannot be scheduled simultaneously" or similar, (just for the idea; that's not a great suggestion) also, tiny nit -- spacing on jsdoc comment asterisks. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:76: // If a card update is requested while an old update is still in the could a "var updatePending" check be used in this file instead of passing a function into your class? https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:258: tasks.finish(); would it be possible to design things in a way that authors don't need to call tasks.finish() before every return? Seems error-prone, e.g. for incremental changes made a year from now. we can talk in person. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:282: if (byUser) { probably up to personal preference, but early return? if (!byUser) return; // rest of function here ? https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:19: // TODO(vadimt): Make sure the execution doesn't continue after this call. how? why? don't mean that to sound short, just curious what you're trying to accomplish. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:49: * @type {Array.<Object.<TaskName, function()>>} the TaskName typedef might belong in this file. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:70: // Starts the oldest queued task, but doesn't remove it from the queue. I forget the long and storied history of this comment, but I think inside a function: "Start the [...] task, but don't remove it [...]" https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:91: * Adds a new task. If another task is not running, run the task immediately. nit: s/run/runs. s/ignore/ignores and s/store/stores below https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:95: * @param {function()} task Code of the task. code of the task -> "function to run" or similar? https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:109: * Completes the current task and start the next queued task if available. s/start/starts https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:112: verify(queue.length >= 1, 'tasks.finish: The task queue is empty.'); referencing comment on verify() way up at the top of this file, here you might want to just early-return rather than preventing execution, for example. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:126: function debugSetStepName(step) { would this ever do anything more? If so, this class's code could benefit from using debugSetStepName so it's set in one place and explicitly called out as debug. Overall though it still sort of complicates things on the client side. Could a similar effect be accomplished using console.log messages, considering the tasks will be serial (no webworkers or timeslicing/fiber-like behavior)? https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:151: submit: submit, submit -> pushTask, queueTask, addTask, schedule() or similar?
https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:110: */ currently clients need to be aware of this function: function someTask() { doSomethingAsyncy(function() { tasks.finish(); }); } which I believe -makes everything that finishes a chain of functions need to know about the task manager. -requires that code that runs in a task is only for use in a Task. could you instead use callbacks and eliminate .finish() from the API? e.g. client code would look like function someTask(callback) { doSomethingAsyncy(function() { callback(); }); } Then in the task manager, you can do something like function TaskManager() { [...] function runNext() { var task = queue[0]; task(function(err) { if (err) handle(err); finish(); }); } } // TaskManager callback() can often be of the form callback(err, optionalParamsAndStuff); where the first arg 'err' is undefined/null on success. also, if you don't need to do anything special there, finish can be your actual callback. runNext() { var task = queue[0]; task(finish); }
https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:19: // TODO(vadimt): Make sure the execution doesn't continue after this call. This was in relation to wanting to early return on certain things that were asserted from what I understand. (?) That problem I don't think can wait, but I don't know how you'll accomplish that here. On 2013/03/14 00:48:50, Travis Skare wrote: > how? why? > don't mean that to sound short, just curious what you're trying to accomplish. https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:62: var UPDATE_CARDS_TASK_NAME = 'update-cards'; Why did TaskName go away? Why are these and the task-related function below not in the other file? You'd have to reference them in both files, but I don't see a reason why we'd need another areTasksConflicting-type function that we would be using for task management. https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:80: extra line https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:23: alert(errorText); I've become worried about this alert for a different reason. Some browsers block the currently executing code on alert, but continue to run event handlers if an event comes back. I haven't been able to find out if Chrome does this, but it could create stranger bugs.
https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:28: On 2013/03/14 00:48:50, Travis Skare wrote: > any reason for just one line break between all of these? Done. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:67: * Checks if a new task can't be added to a task queue that contains an On 2013/03/14 00:48:50, Travis Skare wrote: > check comment -- this function doesn't really know about queues. "Checks if two > tasks cannot be scheduled simultaneously" or similar, (just for the idea; that's > not a great suggestion) > > also, tiny nit -- spacing on jsdoc comment asterisks. Done. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:76: // If a card update is requested while an old update is still in the On 2013/03/14 00:48:50, Travis Skare wrote: > could a "var updatePending" check be used in this file instead of passing a > function into your class? Technically, yes. But I trying to keep the state (that you have to maintain, have bugs etc) to the minimum, if you don't mind. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:258: tasks.finish(); On 2013/03/14 00:48:50, Travis Skare wrote: > would it be possible to design things in a way that authors don't need to call > tasks.finish() before every return? Seems error-prone, e.g. for incremental > changes made a year from now. > > we can talk in person. Done. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:282: if (byUser) { On 2013/03/14 00:48:50, Travis Skare wrote: > probably up to personal preference, but early return? > > if (!byUser) > return; > > // rest of function here > > ? Done. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:19: // TODO(vadimt): Make sure the execution doesn't continue after this call. On 2013/03/14 00:48:50, Travis Skare wrote: > how? why? > don't mean that to sound short, just curious what you're trying to accomplish. Well, you asked to make sure that in startFirst(), we check that the queue is not empty, and if empty, don't execute the rest of the method. I've added 'verify()' call into startFirst() and todo item into verify() to accomplish this. Likely, I'll throw an exception here. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:19: // TODO(vadimt): Make sure the execution doesn't continue after this call. On 2013/03/14 19:05:20, rgustafson wrote: > This was in relation to wanting to early return on certain things that were > asserted from what I understand. (?) That problem I don't think can wait, but I > don't know how you'll accomplish that here. > On 2013/03/14 00:48:50, Travis Skare wrote: > > how? why? > > don't mean that to sound short, just curious what you're trying to accomplish. > Correct. I'll accomplish this by throwing an exception. I don't do this right now because this is required for this change (that I need to unblock the next CL, which is retry-dismiss), and it requires some research (should I throw string, which works in the context of HTML5 callbacks, or a structure containing error message and a stack, which works in the case of Chrome API callbacks). https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:49: * @type {Array.<Object.<TaskName, function()>>} On 2013/03/14 00:48:50, Travis Skare wrote: > the TaskName typedef might belong in this file. Doesn't it have to list all task names, which doesn't belong to this layer? https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:70: // Starts the oldest queued task, but doesn't remove it from the queue. On 2013/03/14 00:48:50, Travis Skare wrote: > I forget the long and storied history of this comment, but I think inside a > function: > > "Start the [...] task, but don't remove it [...]" > Done. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:91: * Adds a new task. If another task is not running, run the task immediately. On 2013/03/14 00:48:50, Travis Skare wrote: > nit: s/run/runs. s/ignore/ignores and s/store/stores below Done. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:95: * @param {function()} task Code of the task. On 2013/03/14 00:48:50, Travis Skare wrote: > code of the task -> "function to run" or similar? Done. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:109: * Completes the current task and start the next queued task if available. On 2013/03/14 00:48:50, Travis Skare wrote: > s/start/starts Done. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:112: verify(queue.length >= 1, 'tasks.finish: The task queue is empty.'); On 2013/03/14 00:48:50, Travis Skare wrote: > referencing comment on verify() way up at the top of this file, here you might > want to just early-return rather than preventing execution, for example. I'll build a general solution (likely, throwing an exception) into verify(), instead of having individual 'return's here and there. https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:126: function debugSetStepName(step) { On 2013/03/14 00:48:50, Travis Skare wrote: > would this ever do anything more? If so, this class's code could benefit from > using debugSetStepName so it's set in one place and explicitly called out as > debug. > > Overall though it still sort of complicates things on the client side. Could a > similar effect be accomplished using console.log messages, considering the tasks > will be serial (no webworkers or timeslicing/fiber-like behavior)? It may or may not do additional things in future. But imagine that I add a check that the task queue is not empty here. Then, it won't be usable from inside if the class. Generally, there is a style (I don't necessary support it in all cases) saying "don't use public methods by a class' implementation". There is also a general agreement that the class doesn't have to always use publicly exposed setters/getters, for example, if they generate events or check preconditions. Speaking of console.log, we want step name appear on our verify() popups and be reported by early adopters, which cannot be achieved using console.log(). https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:151: submit: submit, On 2013/03/14 00:48:50, Travis Skare wrote: > submit -> pushTask, queueTask, addTask, schedule() or similar? Done. https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:62: var UPDATE_CARDS_TASK_NAME = 'update-cards'; On 2013/03/14 19:05:20, rgustafson wrote: > Why did TaskName go away? > > Why are these and the task-related function below not in the other file? You'd > have to reference them in both files, but I don't see a reason why we'd need > another areTasksConflicting-type function that we would be using for task > management. http://en.wikipedia.org/wiki/Layer_(object-oriented_design) For the sake of hygiene, readability, maintainability etc, it makes sense to follow OOP rules even in the case of such a simple extension. The contents of utility.js is a layer that is used by background.js. For the above benefits, I want to design it as if it was reusable, even if it is's not going to be used this way. Then, I cannot have TaskName, which list all concrete task names, in this utility.js. Having it in background.js doesn't make sense too, because no method in background.js takes it as a parameter. https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:80: On 2013/03/14 19:05:20, rgustafson wrote: > extra line It makes sense to separate groups of operators with empty lines for readability. I feel that this is a case where this is appropriate. https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:23: alert(errorText); On 2013/03/14 19:05:20, rgustafson wrote: > I've become worried about this alert for a different reason. Some browsers block > the currently executing code on alert, but continue to run event handlers if an > event comes back. I haven't been able to find out if Chrome does this, but it > could create stranger bugs. In most cases,the second alert won't happen because the current task is still running, and the other task won't start. But even if this happens, the new alert won't be shown till the first one is closed. Showing a prominent error dialog is so useful that I'd suggest living with it for a while. https://codereview.chromium.org/12316075/diff/49001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:110: */ On 2013/03/14 01:19:53, Travis Skare wrote: > currently clients need to be aware of this function: > > function someTask() { > doSomethingAsyncy(function() { > tasks.finish(); > }); > } > > which I believe > -makes everything that finishes a chain of functions need to know about the task > manager. > -requires that code that runs in a task is only for use in a Task. > > could you instead use callbacks and eliminate .finish() from the API? e.g. > client code would look like > > function someTask(callback) { > doSomethingAsyncy(function() { > callback(); > }); > } > > Then in the task manager, you can do something like > > function TaskManager() { > [...] > > function runNext() { > var task = queue[0]; > task(function(err) { > if (err) > handle(err); > finish(); > }); > } > > } // TaskManager > > callback() can often be of the form > callback(err, optionalParamsAndStuff); > where the first arg 'err' is undefined/null on success. > > > also, if you don't need to do anything special there, finish can be your actual > callback. > > runNext() { > var task = queue[0]; > task(finish); > } Done.
https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:49: * @type {Array.<Object.<TaskName, function()>>} ok... probably not an issue since we're not running JSCompiler to check and minify, but IIRC it would complain about not knowing what a TaskName is here, so at this level a string or some such prevents this from having dependencies on a higher level. On 2013/03/14 22:56:05, vadimt wrote: > On 2013/03/14 00:48:50, Travis Skare wrote: > > the TaskName typedef might belong in this file. > > Doesn't it have to list all task names, which doesn't belong to this layer? https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:130: tasks.debugSetStepName( consider debugSetStepName vs setDebugStepName (up to you) https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:185: callback(); this works but you could include error state if wanted: function callback(errorObject, data) {} so you'd know to do backoff if necessary. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:237: function() { requestNotificationCards(''); }); [no response required] this is ok (not totally sure about the spacing) but in case you start to do more with callbacks, know that you can do requestNotificationCards.bind(this, ''); natively in Chrome. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:293: request.onloadend = function() { .onloadend = callback; should work if you're not doing more stuff. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:19: // TODO(vadimt): Make sure the execution doesn't continue after this call. (from previous discussion) callers will be able to catch of course. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:30: * checks if a new task can't be added to a task queue that contains an comment-on-comment: this checks if two tasks are compatible or not, versus verifying a task against a queue. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:32: * @return {Object} Set of methods to manage tasks. set of methods to manage tasks -> "TaskManager interface" perhaps. I'm not too sure here though. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:96: parameter. tiny nit: one more asterisk (sorry!)
https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/44001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:49: * @type {Array.<Object.<TaskName, function()>>} On 2013/03/19 18:51:39, Travis Skare wrote: > ok... probably not an issue since we're not running JSCompiler to check and > minify, but IIRC it would complain about not knowing what a TaskName is here, so > at this level a string or some such prevents this from having dependencies on a > higher level. > > On 2013/03/14 22:56:05, vadimt wrote: > > On 2013/03/14 00:48:50, Travis Skare wrote: > > > the TaskName typedef might belong in this file. > > > > Doesn't it have to list all task names, which doesn't belong to this layer? > Already done. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:130: tasks.debugSetStepName( On 2013/03/19 18:51:39, Travis Skare wrote: > consider debugSetStepName vs setDebugStepName (up to you) In this case, 'debug' is prefix for all debug-only methods (yes, there is only 1 of them). https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:185: callback(); On 2013/03/19 18:51:39, Travis Skare wrote: > this works but you could include error state if wanted: > function callback(errorObject, data) {} > > so you'd know to do backoff if necessary. > > This callback just closes a critical section. You don't pass a success flag to 'LeaveCriticalSection' call. The backoff is done outside of the critical section implementation. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:293: request.onloadend = function() { On 2013/03/19 18:51:39, Travis Skare wrote: > .onloadend = callback; > should work if you're not doing more stuff. This would use probably not best feature of JS, namely, being able skip a function parameters or pass as callback a method that has a different prototype. This wouldn't work in C++, and for good. I've added 'event' params to onloadend callbacks to show what are real prototypes and to make code and debugging clearer. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:19: // TODO(vadimt): Make sure the execution doesn't continue after this call. On 2013/03/19 18:51:39, Travis Skare wrote: > (from previous discussion) > callers will be able to catch of course. Not quite sure what is the action item for this comment :) https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:30: * checks if a new task can't be added to a task queue that contains an On 2013/03/19 18:51:39, Travis Skare wrote: > comment-on-comment: this checks if two tasks are compatible or not, versus > verifying a task against a queue. This method is not necessary symmetrical, so I need to tell which on is in the queue, and which is not. https://codereview.chromium.org/12316075/diff/54001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:32: * @return {Object} Set of methods to manage tasks. On 2013/03/19 18:51:39, Travis Skare wrote: > set of methods to manage tasks -> "TaskManager interface" perhaps. I'm not too > sure here though. Done.
thanks for the edits. Probably a good idea to have an OWNER skim this, at least at a high level. Might have more comments in the event of structural changes.
I'm okay with passing to owner at this point also, but will keep on eye on it to lgtm later if there are changes.
arv@, please provide owner's approval.
sky@, please provide owner's review for the .grd file.
arv is an OWNER for the grd file too, removing myself.
https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:84: var tasks = TaskManager(areTasksConflicting); missing new? Capital letter implies constructor and these should never be called without new (builtins are exceptions) https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:181: request.onloadend = function(event) { Awesome. This is the magic sauce I've been wanting. https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:237: function() { requestNotificationCards('', callback); }); newline https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:295: } ; https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:34: function TaskManager(areConflicting) { make this into a class? https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:83: for (var i = 0; i < queue.length; ++i) only if has optional {}
https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:84: var tasks = TaskManager(areTasksConflicting); On 2013/03/21 21:40:21, arv wrote: > missing new? > > Capital letter implies constructor and these should never be called without new > (builtins are exceptions) Done. https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:237: function() { requestNotificationCards('', callback); }); On 2013/03/21 21:40:21, arv wrote: > newline Done. https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:295: } On 2013/03/21 21:40:21, arv wrote: > ; Done. https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:34: function TaskManager(areConflicting) { On 2013/03/21 21:40:21, arv wrote: > make this into a class? Here, I want to create a collection of methods with some hidden state and some hidden helper methods. The implemented approach just does it using only crispy clear JS features. Creating a class would mean probably more lines of code and using stranger JS features, such as prototypes, 'this' pointers and trickily making classes out of functions. This could be good, I guess, if we needed more, like inheritance, but all this is not necessary here. https://codereview.chromium.org/12316075/diff/59001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:83: for (var i = 0; i < queue.length; ++i) On 2013/03/21 21:40:21, arv wrote: > only if has optional {} Done.
LGTM I don't mind the closure pattern but it is less efficient but you are not creating a lot of these.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/12316075/71001
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/12316075/71001
Message was sent while issue was closed.
Change committed as 190023
Message was sent while issue was closed.
Updated TEST. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
