Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
24924002:
Switching getting/dismissing cards to new protocol (Closed)
|
Created:
7 years, 2 months ago by vadimt Modified:
7 years, 2 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSwitching getting/dismissing cards to new protocol.
BUG=164227
TEST=No
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=227817
Patch Set 1 #Patch Set 2 : Typo #
Total comments: 33
Patch Set 3 : robliao comments. #
Total comments: 6
Patch Set 4 : rgustafson's notes #Patch Set 5 : Making actionUrls optional #
Total comments: 31
Patch Set 6 : More rgustafson's comments #
Messages
Total messages: 22 (0 generated)
https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:105: * version: number, This is listed as optional in the protobuf on the server. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:107: * actionUrls: Object, So this this https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:108: * dismissal: Object And this https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:266: * key is chromeNotificationId. The notification key is chromeNotificationId https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:268: function showNotificationCards(cards) { The server has removed the cards terminology and used notifications instead. Shouldn't we be doing the same here? https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:344: // code is no longer necessary. Is this code still necessary? https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:430: var nextPollTime = null; Units. Sounds like milliseconds? https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:434: nextPollTime = nextPollTime === null ? We haven't been using === much (even though it's likely what we should use in most if not all of our uses of ==). Use == for consistency. nextPollTime is not likely to be undefined especially since initialized to null and group.nextPollTime is shouldn't be undefined. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:435: group.nextPollTime : Math.min(group.nextPollTime, nextPollTime); What are the value guarantees of group.nextPollTime? https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:438: verify(nextPollTime !== null, 'scheduleNextPoll: nextPollTime is null'); Same here. Alternatively typeof nextPollTime == 'number' https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:497: now + receivedGroup.nextPollSeconds * MS_IN_SECOND; Parens (Explicit order of ops) https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background_unittest.gtestjs:92: dismissals: [{notificationId: testNotificationId, parameters: testDismissal1}], Add Linebreak https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background_unittest.gtestjs:108: // Wire card priority is greater than merged card one. Wire card rank is Do we have a case where wire card priority == merged card priority https://codereview.chromium.org/24924002/diff/2001/chrome/test/data/webui/tes... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/24924002/diff/2001/chrome/test/data/webui/tes... chrome/test/data/webui/test_api.js:735: new RegExp(stackInfo.callerName + '\\((.|\\n|\\r)*?\\);', 'g'); Windows is sending \r's?
https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:105: * version: number, On 2013/09/27 19:41:55, Robert Liao wrote: > This is listed as optional in the protobuf on the server. Contacted the Server Person. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:107: * actionUrls: Object, On 2013/09/27 19:41:55, Robert Liao wrote: > So this this See above. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:108: * dismissal: Object On 2013/09/27 19:41:55, Robert Liao wrote: > And this See above. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:266: * key is chromeNotificationId. On 2013/09/27 19:41:55, Robert Liao wrote: > The notification key is chromeNotificationId Done. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:268: function showNotificationCards(cards) { On 2013/09/27 19:41:55, Robert Liao wrote: > The server has removed the cards terminology and used notifications instead. > Shouldn't we be doing the same here? May be. But we already have cards.js etc, which should be renamed with all its guts. Also, it's very hard to rename fields and variable in JS, because of lack of static checks. We propbably should do this opportunistically, but I feel that not now. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:344: // code is no longer necessary. On 2013/09/27 19:41:55, Robert Liao wrote: > Is this code still necessary? We should keep it till we know how the final state machine will look like. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:430: var nextPollTime = null; On 2013/09/27 19:41:55, Robert Liao wrote: > Units. Sounds like milliseconds? I don't think that units should be used for absolute times. It's time in whatever sense JS give to it. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:434: nextPollTime = nextPollTime === null ? On 2013/09/27 19:41:55, Robert Liao wrote: > We haven't been using === much (even though it's likely what we should use in > most if not all of our uses of ==). Use == for consistency. nextPollTime is not > likely to be undefined especially since initialized to null and > group.nextPollTime is shouldn't be undefined. Done. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:435: group.nextPollTime : Math.min(group.nextPollTime, nextPollTime); On 2013/09/27 19:41:55, Robert Liao wrote: > What are the value guarantees of group.nextPollTime? Pretty much arbitrary time; can be in the past, if storage.set for recent updates consistently failed. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:438: verify(nextPollTime !== null, 'scheduleNextPoll: nextPollTime is null'); On 2013/09/27 19:41:55, Robert Liao wrote: > Same here. Alternatively typeof nextPollTime == 'number' Done. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:497: now + receivedGroup.nextPollSeconds * MS_IN_SECOND; On 2013/09/27 19:41:55, Robert Liao wrote: > Parens (Explicit order of ops) Order of ops is self-explaining. Is this in style guide? https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background_unittest.gtestjs:92: dismissals: [{notificationId: testNotificationId, parameters: testDismissal1}], On 2013/09/27 19:41:55, Robert Liao wrote: > Add Linebreak Done. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background_unittest.gtestjs:108: // Wire card priority is greater than merged card one. Wire card rank is On 2013/09/27 19:41:55, Robert Liao wrote: > Do we have a case where wire card priority == merged card priority No; I just didn't think it's an interesting case to test. We need to stop at some point :) https://codereview.chromium.org/24924002/diff/2001/chrome/test/data/webui/tes... File chrome/test/data/webui/test_api.js (right): https://codereview.chromium.org/24924002/diff/2001/chrome/test/data/webui/tes... chrome/test/data/webui/test_api.js:735: new RegExp(stackInfo.callerName + '\\((.|\\n|\\r)*?\\);', 'g'); On 2013/09/27 19:41:55, Robert Liao wrote: > Windows is sending \r's? Yep.
Pending server discussion https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:497: now + receivedGroup.nextPollSeconds * MS_IN_SECOND; Self-explaining, but not everyone has the order memorized (hence I prefer to be explicit when using binary operators). Oddly enough, this is explicitly called out in the Java style guide but not the Javascript or C++ ones. Javascript just says to use sparingly. On 2013/09/27 21:06:00, vadimt wrote: > On 2013/09/27 19:41:55, Robert Liao wrote: > > Parens (Explicit order of ops) > > Order of ops is self-explaining. Is this in style guide? https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background_unittest.gtestjs:108: // Wire card priority is greater than merged card one. Wire card rank is It may be interesting if the merged card changed in an unexpected way with a wire card of an equal priority. On 2013/09/27 21:06:00, vadimt wrote: > On 2013/09/27 19:41:55, Robert Liao wrote: > > Do we have a case where wire card priority == merged card priority > > No; I just didn't think it's an interesting case to test. We need to stop at > some point :)
https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:497: now + receivedGroup.nextPollSeconds * MS_IN_SECOND; On 2013/09/27 21:17:22, Robert Liao wrote: > Self-explaining, but not everyone has the order memorized (hence I prefer to be > explicit when using binary operators). > > Oddly enough, this is explicitly called out in the Java style guide but not the > Javascript or C++ ones. > > Javascript just says to use sparingly. > On 2013/09/27 21:06:00, vadimt wrote: > > On 2013/09/27 19:41:55, Robert Liao wrote: > > > Parens (Explicit order of ops) > > > > Order of ops is self-explaining. Is this in style guide? > OK, let's use sparingly, which mean not using :) OK? https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background_unittest.gtestjs:108: // Wire card priority is greater than merged card one. Wire card rank is On 2013/09/27 21:17:22, Robert Liao wrote: > It may be interesting if the merged card changed in an unexpected way with a > wire card of an equal priority. > On 2013/09/27 21:06:00, vadimt wrote: > > On 2013/09/27 19:41:55, Robert Liao wrote: > > > Do we have a case where wire card priority == merged card priority > > > > No; I just didn't think it's an interesting case to test. We need to stop at > > some point :) > Well, here we know that priority is not used for decision making. We cannot pretend we know nothing about the tested algorithm. Otherwise, we could ask "why not test for the case where priority is the 10-th prime number"? You Never Can Tell With Bees. So, we just get max of 2 priorities, and I believe that 2 cases (> and <) is sufficient.
first round, will come back for more. https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/2001/chrome/browser/resources/g... chrome/browser/resources/google_now/background.js:268: function showNotificationCards(cards) { On 2013/09/27 21:06:00, vadimt wrote: > On 2013/09/27 19:41:55, Robert Liao wrote: > > The server has removed the cards terminology and used notifications instead. > > Shouldn't we be doing the same here? > > May be. But we already have cards.js etc, which should be renamed with all its > guts. Also, it's very hard to rename fields and variable in JS, because of lack > of static checks. > We propbably should do this opportunistically, but I feel that not now. No action, just thoughts: I'm okay with cards existing in the client, it just won't be part of our client-server api terminology. It is a bit confusing notifications -> cards -> notifications, but I can live with it. https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:300: console.log('parseAndShowNotificationCards-delete-check ' + extra spaces https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:530: var now = Date.now(); This is only used once? Should probably put it closer to where it's used, inside the first if. https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... chrome/browser/resources/google_now/cards.js:126: * @param {MergedCard} card Google Now from the server. Google Now card?
https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:300: console.log('parseAndShowNotificationCards-delete-check ' + On 2013/09/30 21:26:09, rgustafson wrote: > extra spaces Done. https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:530: var now = Date.now(); On 2013/09/30 21:26:09, rgustafson wrote: > This is only used once? Should probably put it closer to where it's used, inside > the first if. Done. https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/24924002/diff/10001/chrome/browser/resources/... chrome/browser/resources/google_now/cards.js:126: * @param {MergedCard} card Google Now from the server. On 2013/09/30 21:26:09, rgustafson wrote: > Google Now card? Done.
lgtm
I should really stop having two cls open right next to each other... undo lgtm, still working out stuff.
On 2013/10/01 00:49:28, rgustafson wrote: > I should really stop having two cls open right next to each other... undo lgtm, > still working out stuff. Waiting on the protobuf situation to resolve.
On 2013/10/01 00:59:20, Robert Liao wrote: > On 2013/10/01 00:49:28, rgustafson wrote: > > I should really stop having two cls open right next to each other... undo > lgtm, > > still working out stuff. > > Waiting on the protobuf situation to resolve. Addressing the protobuf story.
https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:108: * dismissal: Object This is also marked as optional.
https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:108: * dismissal: Object On 2013/10/01 20:08:04, Robert Liao wrote: > This is also marked as optional. Will be unmarked as such on the server (see the thread).
lgtm https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:71: if (contentType) We should assert that the content type is allowed for the provided method. For example, this would be disallowed with a GET.
https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:71: if (contentType) On 2013/10/01 21:52:43, Robert Liao wrote: > We should assert that the content type is allowed for the provided method. For > example, this would be disallowed with a GET. Such asserts could make sense in libraries that have many users. Here, it makes more sense just to be accurate when specifying parameters to buildServerRequest().
https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:273: console.log('parseAndShowNotificationCards-get ' + showNotificationCards-get https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:279: console.log('parseAndShowNotificationCards-getAll ' + showNotificationCards-getAll https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:355: * Merges an unmerged notification into a merged card with same ID. Change to "an existing merged card" to make it clearer without having to read the variable descriptions. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:361: * @param {number} cardTimestamp The moment the unmerged card was received. You're using notification as the terminology for data just received form the server and card as the existing data. This would make much more sense as notificationTimestamp. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:362: * @param {number} cardGroupRank Rank of the group of the unmerged card. Same as above. notificationGroupRank https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:390: if (!mergedCard || cardTimestamp > mergedCard.timestamp) { When will this not be true? https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:409: * @param {Object.<string, MergedCard>} mergedCards Set of merged cards. Map ... keyed by id? https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:447: * Parses JSON response from the notification server, shows notifications and nit: comma before and https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:482: nextPollTime: now, This seems kind of dangerous if there is a repeating storage failure. That machine will just keep on requesting, no? https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/cards.js:34: * notification: Object, notificationOptions https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/cards.js:46: * notification: Object, notificationOptions https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/cards.js:102: scheduleHiding(cardId, cardCreateInfo.hideTime); Extra spaces. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/cards.js:118: scheduleHiding(cardId, cardCreateInfo.hideTime); Extra spaces.
https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:273: console.log('parseAndShowNotificationCards-get ' + On 2013/10/04 16:56:47, rgustafson wrote: > showNotificationCards-get Done. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:279: console.log('parseAndShowNotificationCards-getAll ' + On 2013/10/04 16:56:47, rgustafson wrote: > showNotificationCards-getAll Done. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:355: * Merges an unmerged notification into a merged card with same ID. On 2013/10/04 16:56:47, rgustafson wrote: > Change to "an existing merged card" to make it clearer without having to read > the variable descriptions. The merged card may not exist at this moment, in which case mergedCard parameter === undefined. So, we cannot say 'existing'. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:361: * @param {number} cardTimestamp The moment the unmerged card was received. On 2013/10/04 16:56:47, rgustafson wrote: > You're using notification as the terminology for data just received form the > server and card as the existing data. This would make much more sense as > notificationTimestamp. Done. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:362: * @param {number} cardGroupRank Rank of the group of the unmerged card. On 2013/10/04 16:56:47, rgustafson wrote: > Same as above. notificationGroupRank Done. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:390: if (!mergedCard || cardTimestamp > mergedCard.timestamp) { On 2013/10/04 16:56:47, rgustafson wrote: > When will this not be true? Your question assumes that we keep a collection of merged cards, and incrementally merge new cards into it. The method, in fact, is different. We have a collection of all groups, update it after pinging the server, and build merged cards from scratch every time from all groups. Groups may come in any order, for example, newer first. This is how this condition may be not true. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:409: * @param {Object.<string, MergedCard>} mergedCards Set of merged cards. On 2013/10/04 16:56:47, rgustafson wrote: > Map ... keyed by id? Done. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:447: * Parses JSON response from the notification server, shows notifications and On 2013/10/04 16:56:47, rgustafson wrote: > nit: comma before and I thought comma is not used before 'and' in a sequence of similar things (parses, shows and schedules) https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:482: nextPollTime: now, On 2013/10/04 16:56:47, rgustafson wrote: > This seems kind of dangerous if there is a repeating storage failure. That > machine will just keep on requesting, no? Every 5 minutes, see MINIMUM_POLLING_PERIOD_SECONDS. https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/cards.js:34: * notification: Object, On 2013/10/04 16:56:47, rgustafson wrote: > notificationOptions I'd prefer not to rename this. Renaming anything in JS, where there is no static control, and when the variable name is so overloaded, is somewhat dangerous. May be, leave it? https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/cards.js:46: * notification: Object, On 2013/10/04 16:56:47, rgustafson wrote: > notificationOptions See above.
lgtm https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:447: * Parses JSON response from the notification server, shows notifications and On 2013/10/07 20:29:54, vadimt wrote: > On 2013/10/04 16:56:47, rgustafson wrote: > > nit: comma before and > > I thought comma is not used before 'and' in a sequence of similar things > (parses, shows and schedules) FYI: I was taught the exact opposite. The internet thinks either works as long as it doesn't create ambiguity, and there isn't much ambiguity here.
https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:447: * Parses JSON response from the notification server, shows notifications and A comma is required at the second to last item in a list like this in United States English text. It is omitted in newspapers and similar media as well as UK English to save on space. http://en.wikipedia.org/wiki/Serial_comma The Elements of Style (Strunk and White, 4th edition 1999), Rule 2[4] In a series of three or more terms with a single conjunction, use a comma after each term except the last. For example, "red, white, and blue." On 2013/10/08 18:23:27, rgustafson wrote: > On 2013/10/07 20:29:54, vadimt wrote: > > On 2013/10/04 16:56:47, rgustafson wrote: > > > nit: comma before and > > > > I thought comma is not used before 'and' in a sequence of similar things > > (parses, shows and schedules) > > FYI: I was taught the exact opposite. The internet thinks either works as long > as it doesn't create ambiguity, and there isn't much ambiguity here.
lgtm https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/24924002/diff/28001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:447: * Parses JSON response from the notification server, shows notifications and On 2013/10/08 20:48:14, Robert Liao wrote: > A comma is required at the second to last item in a list like this in United > States English text. It is omitted in newspapers and similar media as well as UK > English to save on space. > > http://en.wikipedia.org/wiki/Serial_comma > > The Elements of Style (Strunk and White, 4th edition 1999), Rule 2[4] > In a series of three or more terms with a single conjunction, use a comma after > each term except the last. > For example, "red, white, and blue." > > On 2013/10/08 18:23:27, rgustafson wrote: > > On 2013/10/07 20:29:54, vadimt wrote: > > > On 2013/10/04 16:56:47, rgustafson wrote: > > > > nit: comma before and > > > > > > I thought comma is not used before 'and' in a sequence of similar things > > > (parses, shows and schedules) > > > > FYI: I was taught the exact opposite. The internet thinks either works as long > > as it doesn't create ambiguity, and there isn't much ambiguity here. > +1 for Team Oxford Comma
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/24924002/46001
Message was sent while issue was closed.
Change committed as 227817 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
