Cleaning data associated with the card upon deletion.

chrome/browser/resources/google_now/cards.js

Index: chrome/browser/resources/google_now/cards.js
diff --git a/chrome/browser/resources/google_now/cards.js b/chrome/browser/resources/google_now/cards.js
index 5c7845c3a31053ee69903d35e0c00c23695e8765..42925e3ee54a444e8eed82b7bbd1d37870caba68 100644
--- a/chrome/browser/resources/google_now/cards.js
+++ b/chrome/browser/resources/google_now/cards.js
@@ -51,6 +51,11 @@ var MergedCard;
  */
 var CardCreateInfo;
 
+/**
+ * Names for tasks that can be created by the this file.
+ */
+var CLEAR_CARD_TASK_NAME = 'clear-card';
+
 /**
  * Builds an object to manage notification card set.
  * @return {Object} Card set interface.
@@ -99,7 +104,7 @@ function buildCardSet() {
               return;
             }
 
-              scheduleHiding(cardId, cardCreateInfo.hideTime);
+            scheduleHiding(cardId, cardCreateInfo.hideTime);
           });
     } else {
       // Update existing notification.
@@ -115,7 +120,7 @@ function buildCardSet() {
               return;
             }
 
-              scheduleHiding(cardId, cardCreateInfo.hideTime);
+            scheduleHiding(cardId, cardCreateInfo.hideTime);
           });
     }
   }
@@ -168,13 +173,39 @@ function buildCardSet() {
   /**
    * Removes a card notification.
    * @param {string} cardId Card ID.
+   * @param {boolean} clearStorage True if the information associated with the
+   *     card should be erased from chrome.storage.
    */
-  function clear(cardId) {
+  function clear(cardId, clearStorage) {
     console.log('cardManager.clear ' + cardId);
 
     chrome.notifications.clear(cardId, function() {});
     chrome.alarms.clear(cardShowPrefix + cardId);
     chrome.alarms.clear(cardHidePrefix + cardId);
+
+    if (clearStorage) {
+      instrumented.storage.local.get(
+          ['notificationsData', 'notificationGroups'],
+          function(items) {
+            items = items || {};
+            items.notificationsData = items.notificationsData || {};
+            items.notificationGroups = items.notificationGroups || {};
+
+            delete items.notificationsData[cardId];

[rgustafson, 2013/09/27 18:57:20]

Do you have to use delete vs setting to null in this case? Do we ever loop
through this data or check if it contains something?

[vadimt, 2013/09/27 20:10:56]

We'll never attempt to access this data after clearing.

[rgustafson, 2013/09/27 20:19:28]

Then style says to set to null instead of delete.
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#delete

[vadimt, 2013/09/27 20:25:24]

"The delete keyword should be avoided except when it is necessary to remove a
property from an object's iterated list of keys,"
This is a map, so it's appropriate to just delete an entry. In addition,
performance is not important here, since this will happen on relatively rate
events.

[rgustafson, 2013/09/27 22:53:01]

It has a list of keys. It isn't necessary to remove the property from the list
of keys though. The code is only directly accessing specific keys, which would
behave the same way if you were to null it out. The style guide rule applies.

I remember back in the day there was a place where we were allowed to use delete
because fields had to disappear, but this is not the case here.

[robliao, 2013/09/27 22:59:13]

The concern here is that deleting is slower than setting to null. That's why
this rule exists.

If setting the field to null doesn't have an adverse impact on the code, that
would be more appropriate.
If there are many instances of iterating through the property names or property
existence checks, then deleting would be more appropriate.

On 2013/09/27 22:53:01, rgustafson wrote:

[vadimt, 2013/09/30 16:56:17]

FWIW: I'm not null-ing it.
This is a map. No one deletes elements from a map by null-ing. It will grow
indefinitely, potentially. Also, it's not iterable now, but may become iterable
at any moment. Assigning null is not future-proof, therefore.
Remember also that if some API calls fail, and the data becomes inconsistent,
it's possible that this field will be accessed. It's not a good idea to have
paranoid checks for null everywhere in the code.
Also, null-ing would create a strange contract for this map, not just map, but a
map with some elements set to null. This is complex and error-prone.
Also, given that in order for this code to be executed, an event page needs
likely to be loaded, the value read from chrome storage and written back. The
perf effect will be 0, the uglification of the extension will be noticeable.

[rgustafson, 2013/09/30 18:44:02]

You still send reviews to arv, right? If the style guide is for more than just
laughs, I guess he'll say something.

None of the issues you listed are problems in the current code unless we somehow
manage to send infinite cards in one go since the object is replaced every time
we get new cards. Every access of the data I see already checks for null in some
fashion, probably to prevent errors that cause inconsistent data as you said.

+
+            for (var groupName in items.notificationGroups) {
+              var group = items.notificationGroups[groupName];
+              for (var i = 0; i != group.cards.length; ++i) {
+                if (group.cards[i].chromeNotificationId == cardId) {
+                  group.cards.splice(i, 1);
+                  break;
+                }
+              }
+            }
+
+            chrome.storage.local.set(items);
+          });
+    }
   }
 
   instrumented.alarms.onAlarm.addListener(function(alarm) {
@@ -195,8 +226,10 @@ function buildCardSet() {
       });
     } else if (alarm.name.indexOf(cardHidePrefix) == 0) {
       // Alarm to hide the card.
-      var cardId = alarm.name.substring(cardHidePrefix.length);
-      clear(cardId);
+      tasks.add(CLEAR_CARD_TASK_NAME, function() {
+        var cardId = alarm.name.substring(cardHidePrefix.length);
+        clear(cardId, true);
+      });
     }
   });