Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
107033002:
Combining cards instead of merging (Closed)
|
Created:
7 years ago by vadimt Modified:
6 years, 10 months ago CC:
chromium-reviews, arv+watch_chromium.org, govind1 Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCombining cards instead of merging.
When there are several cards with same Chrome ID, we now not simply choose the one with the highest priority and call it a day.
Instead, for every moment of time, we show the card with the highest priority, chosen from ones that should be visible at this moment.
Also, added Closure annotations to enable static type analysis.
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=239389
Patch Set 1 #Patch Set 2 : Remove broken unit tests #
Total comments: 23
Patch Set 3 : robliao@ comments #Patch Set 4 : More robliao@ comments #
Total comments: 12
Patch Set 5 : skare@ pre-caffeine comments #Patch Set 6 : (forgot to save a file) #
Total comments: 2
Patch Set 7 : rgustafson's comments #
Total comments: 12
Patch Set 8 : More rgustafson's notes #
Total comments: 2
Patch Set 9 : More robliao@ comments #Patch Set 10 : rgistafson's verbal comment #
Messages
Total messages: 18 (0 generated)
https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/cards_unittest.gtestjs (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/cards_unittest.gtestjs:78: Don't we need new tests for the different functionality?
https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:140: var StorageGroup; Should this be NotificationGroup? https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:296: * @param {Object.<string, CombinedCard>} cards Map from chromeNotificationId to Does the annotation system work on non-object types (like strings?) That way string here could be chromeNotificatoinId. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:357: function combineGroup(combinedCards, storageGroup) { storageGroup -> notificationGroup https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:375: combinedCards[receivedNotification.chromeNotificationId] = combinedCard; Alternatively for the above lines... combinedCards[receivedNotification.chromeNotificationId] = combinedCards[receivedNotification.chromeNotificationId] || []; var combinedCard = combinedCards[receivedNotification.chromeNotificationId]; combinedCard.push(uncombinedNotification); // Omit last line https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:442: function parseAndShowNotificationCards(response, onCardShown) { While we're here, it may be clearer to do something like the below in lieu of the *AndShow pattern... function showNotificationCards(onCardsShown) { requestNotificationCards(..., function(response) { parseNotificationCards(response, function (parsed) { combineNotificationCards(parsed, function (combined) { displayNotificationCards(combined, onCardsShown); } } } } Now, you can do isolated testing of each step as well. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:473: if (now - items.recentDismissals[notificationId] < Could be clearer to do... var retentionTimeMs = now - items.recentDismissals[notificationId]; if (retentionTimeMs < DISMISS_RETENTION_TIME_MS) https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:155: function enumerateUncombinedCards(combinedCard, timestamp, callback) { Perhaps enumerateUncombinedNotifications?
https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:140: var StorageGroup; On 2013/12/05 23:27:46, robliao wrote: > Should this be NotificationGroup? No. We transform a received group before storing (adding cards). Name "NotificationGroup" doesn't reflect the fact that this is the group as it's stored, not as it's received. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:296: * @param {Object.<string, CombinedCard>} cards Map from chromeNotificationId to On 2013/12/05 23:27:46, robliao wrote: > Does the annotation system work on non-object types (like strings?) That way > string here could be chromeNotificatoinId. Done. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:357: function combineGroup(combinedCards, storageGroup) { On 2013/12/05 23:27:46, robliao wrote: > storageGroup -> notificationGroup See above. It's important that this is StorageGroup. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:375: combinedCards[receivedNotification.chromeNotificationId] = combinedCard; On 2013/12/05 23:27:46, robliao wrote: > Alternatively for the above lines... > combinedCards[receivedNotification.chromeNotificationId] = > combinedCards[receivedNotification.chromeNotificationId] || []; > var combinedCard = combinedCards[receivedNotification.chromeNotificationId]; > combinedCard.push(uncombinedNotification); > // Omit last line The suggested code is longer, since there are 3 indexings. Current code has 2. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:442: function parseAndShowNotificationCards(response, onCardShown) { On 2013/12/05 23:27:46, robliao wrote: > While we're here, it may be clearer to do something like the below in lieu of > the *AndShow pattern... > > function showNotificationCards(onCardsShown) { > requestNotificationCards(..., function(response) { > parseNotificationCards(response, function (parsed) { > combineNotificationCards(parsed, function (combined) { > displayNotificationCards(combined, onCardsShown); > } > } > } > } > > Now, you can do isolated testing of each step as well. I'd prefer to not do this now. I want to land this CL ASAP. I agree that this is good for testing, so let's combine this with the testing CL. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:473: if (now - items.recentDismissals[notificationId] < On 2013/12/05 23:27:46, robliao wrote: > Could be clearer to do... > > var retentionTimeMs = now - items.recentDismissals[notificationId]; > if (retentionTimeMs < DISMISS_RETENTION_TIME_MS) Done. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:155: function enumerateUncombinedCards(combinedCard, timestamp, callback) { On 2013/12/05 23:27:46, robliao wrote: > Perhaps enumerateUncombinedNotifications? Done.
https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:140: var StorageGroup; StoredNotificationGroup would be better then. On 2013/12/06 02:32:38, vadimt wrote: > On 2013/12/05 23:27:46, robliao wrote: > > Should this be NotificationGroup? > > No. We transform a received group before storing (adding cards). Name > "NotificationGroup" doesn't reflect the fact that this is the group as it's > stored, not as it's received. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:375: combinedCards[receivedNotification.chromeNotificationId] = combinedCard; Only reason I had to suggest a change is I had to verify that the assignment on this line was indeed doing the right thing. On 2013/12/06 02:32:38, vadimt wrote: > On 2013/12/05 23:27:46, robliao wrote: > > Alternatively for the above lines... > > combinedCards[receivedNotification.chromeNotificationId] = > > combinedCards[receivedNotification.chromeNotificationId] || []; > > var combinedCard = combinedCards[receivedNotification.chromeNotificationId]; > > combinedCard.push(uncombinedNotification); > > // Omit last line > > The suggested code is longer, since there are 3 indexings. Current code has 2.
https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:140: var StorageGroup; On 2013/12/06 02:38:38, robliao wrote: > StoredNotificationGroup would be better then. > On 2013/12/06 02:32:38, vadimt wrote: > > On 2013/12/05 23:27:46, robliao wrote: > > > Should this be NotificationGroup? > > > > No. We transform a received group before storing (adding cards). Name > > "NotificationGroup" doesn't reflect the fact that this is the group as it's > > stored, not as it's received. > Done. https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:375: combinedCards[receivedNotification.chromeNotificationId] = combinedCard; On 2013/12/06 02:38:38, robliao wrote: > Only reason I had to suggest a change is I had to verify that the assignment on > this line was indeed doing the right thing. > On 2013/12/06 02:32:38, vadimt wrote: > > On 2013/12/05 23:27:46, robliao wrote: > > > Alternatively for the above lines... > > > combinedCards[receivedNotification.chromeNotificationId] = > > > combinedCards[receivedNotification.chromeNotificationId] || []; > > > var combinedCard = combinedCards[receivedNotification.chromeNotificationId]; > > > combinedCard.push(uncombinedNotification); > > > // Omit last line > > > > The suggested code is longer, since there are 3 indexings. Current code has 2. > Good point. I changed the order. Hopefully, now it seems better.
hi Vadim -- need to take another look at this later with more coffee, sorry -- but had one question in cards.js regarding the chrome storage behavior you discovered. thanks! https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:105: * Group as it's received from the server. [optional] was here before, but you can eliminate "it's" if you want https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:240: // be stored correctly to chrome.storage. side question: is this known? intended? if no, worth filing a bug for?
https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:105: * Group as it's received from the server. On 2013/12/06 03:44:00, Travis Skare wrote: > [optional] was here before, but you can eliminate "it's" if you want Done. https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:240: // be stored correctly to chrome.storage. On 2013/12/06 03:44:00, Travis Skare wrote: > side question: is this known? intended? if no, worth filing a bug for? This behavior makes sense to me. This is a good way to avoid infinite serialization if the data structure contains loop. It seems to work like: if you serialize same object second time, serialize it as null.
preliminary nit comments. I promise I've looked at it in more depth than just this. :) https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:143: * Pending (not yet successfully sent) dismissal for an unmerged notification. unmerged notification -> received notification to match new terminology. https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:18: * ID of an undividual (uncombined) notification. individual https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:35: * Urls that need to be opened when clicking at notification or its buttons. clicking a https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:70: * Received notification, and absolute show/hide times. Describe what the actual object's purpose is instead of what's inside that we can see right below. https://codereview.chromium.org/107033002/diff/90001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/90001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:482: for (var i = 0; i != parsedResponse.notifications.length; ++i) { nit: why not <?
https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:143: * Pending (not yet successfully sent) dismissal for an unmerged notification. On 2013/12/06 20:01:17, rgustafson wrote: > unmerged notification -> received notification to match new terminology. Done. https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:18: * ID of an undividual (uncombined) notification. On 2013/12/06 20:01:17, rgustafson wrote: > individual Fixed, but the old one was better. https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:35: * Urls that need to be opened when clicking at notification or its buttons. On 2013/12/06 20:01:17, rgustafson wrote: > clicking a Done. https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:70: * Received notification, and absolute show/hide times. On 2013/12/06 20:01:17, rgustafson wrote: > Describe what the actual object's purpose is instead of what's inside that we > can see right below. Done. https://codereview.chromium.org/107033002/diff/90001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/90001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:482: for (var i = 0; i != parsedResponse.notifications.length; ++i) { On 2013/12/06 20:01:17, rgustafson wrote: > nit: why not <? This was a protection from incorrectly written loops. If it jumps over the terminal value, it will loop forever, and we'll catch this. But no one seems to understand me :( (and Dijkstra who introduced this as a principle of a weakest continuation condition).
https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:442: function parseAndShowNotificationCards(response, onCardShown) { On 2013/12/06 02:32:38, vadimt wrote: > On 2013/12/05 23:27:46, robliao wrote: > > While we're here, it may be clearer to do something like the below in lieu of > > the *AndShow pattern... > > > > function showNotificationCards(onCardsShown) { > > requestNotificationCards(..., function(response) { > > parseNotificationCards(response, function (parsed) { > > combineNotificationCards(parsed, function (combined) { > > displayNotificationCards(combined, onCardsShown); > > } > > } > > } > > } > > > > Now, you can do isolated testing of each step as well. > > I'd prefer to not do this now. I want to land this CL ASAP. I agree that this is > good for testing, so let's combine this with the testing CL. +1 on this. https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:375: combinedCard.push(uncombinedNotification); There were comments on this before, but I find it more straightforward to do retrieve/update/assign than retrieve/assign/update. https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:132: // Try updating the notification. So the only way to make a notification retoast now is for a higher priority notification to come in? Or change the id of the notification? Just checking, version is already dead on the server anyways. If it's not used anywhere on the client, you might want to remove it from the definition of what the server gives since maybe someday it will get removed. https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:162: * Enumerates uncombined notifications in a combined card, determining for Enumerating implies listing everything. A list is not provided here. Iterate? https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:208: // If the uncombined notification is visible now and set the winning card Is this the right tab level for a function that is a parameter? If not, you might want to consider naming it. https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:276: // was last time updated. was last updated https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:298: * Removes a card information from 'notificationGroups'. Removes card information
https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:442: function parseAndShowNotificationCards(response, onCardShown) { On 2013/12/06 23:35:16, rgustafson wrote: > On 2013/12/06 02:32:38, vadimt wrote: > > On 2013/12/05 23:27:46, robliao wrote: > > > While we're here, it may be clearer to do something like the below in lieu > of > > > the *AndShow pattern... > > > > > > function showNotificationCards(onCardsShown) { > > > requestNotificationCards(..., function(response) { > > > parseNotificationCards(response, function (parsed) { > > > combineNotificationCards(parsed, function (combined) { > > > displayNotificationCards(combined, onCardsShown); > > > } > > > } > > > } > > > } > > > > > > Now, you can do isolated testing of each step as well. > > > > I'd prefer to not do this now. I want to land this CL ASAP. I agree that this > is > > good for testing, so let's combine this with the testing CL. > > +1 on this. Ack https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:375: combinedCard.push(uncombinedNotification); On 2013/12/06 23:35:16, rgustafson wrote: > There were comments on this before, but I find it more straightforward to do > retrieve/update/assign than retrieve/assign/update. Done. https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:132: // Try updating the notification. On 2013/12/06 23:35:16, rgustafson wrote: > So the only way to make a notification retoast now is for a higher priority > notification to come in? Or change the id of the notification? Just checking, > version is already dead on the server anyways. --- Correct > > If it's not used anywhere on the client, you might want to remove it from the > definition of what the server gives since maybe someday it will get removed. ---- Done https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:162: * Enumerates uncombined notifications in a combined card, determining for On 2013/12/06 23:35:16, rgustafson wrote: > Enumerating implies listing everything. A list is not provided here. Iterate? Done. https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:208: // If the uncombined notification is visible now and set the winning card On 2013/12/06 23:35:16, rgustafson wrote: > Is this the right tab level for a function that is a parameter? If not, you > might want to consider naming it. Done. https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:276: // was last time updated. On 2013/12/06 23:35:16, rgustafson wrote: > was last updated Done. https://codereview.chromium.org/107033002/diff/110001/chrome/browser/resource... chrome/browser/resources/google_now/cards.js:298: * Removes a card information from 'notificationGroups'. On 2013/12/06 23:35:16, rgustafson wrote: > Removes card information Done.
Provisional LGTM: * Needs Unit Tests * Refactor the *AndShowNotification pattern. https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:240: // be stored correctly to chrome.storage. On 2013/12/06 18:58:49, vadimt wrote: > On 2013/12/06 03:44:00, Travis Skare wrote: > > side question: is this known? intended? if no, worth filing a bug for? > > This behavior makes sense to me. This is a good way to avoid infinite > serialization if the data structure contains loop. It seems to work like: if you > serialize same object second time, serialize it as null. This sounds like a gotcha of the storage system and should be documented. I suspect they're storing objects as JSON underneath, which doesn't do so well with circular structures. Alternatively, they can simply just handle loops (costs a bit more in the serialized representation, but very easy to store). https://codereview.chromium.org/107033002/diff/130001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/130001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:460: console.log('parseAndShowNotificationCards-get ' + JSON.stringify(items)); Style: These need to be tabbed over two spaces.
https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/107033002/diff/50001/chrome/browser/resources... chrome/browser/resources/google_now/cards.js:240: // be stored correctly to chrome.storage. I'll let them know. https://codereview.chromium.org/107033002/diff/130001/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/107033002/diff/130001/chrome/browser/resource... chrome/browser/resources/google_now/background.js:460: console.log('parseAndShowNotificationCards-get ' + JSON.stringify(items)); On 2013/12/07 00:51:29, robliao wrote: > Style: These need to be tabbed over two spaces. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/107033002/170001
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/107033002/170001
Message was sent while issue was closed.
Change committed as 239389 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
