Cleaning data associated with the card upon deletion.

chrome/browser/resources/google_now/cards.js

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 'use strict';
 
 /**
  * Show/hide trigger in a card.
  *
  * @typedef {{
@@ skipping 34 matching lines @@
  * @typedef {{
  *   notification: Object,
  *   hideTime: number=,
  *   version: number,
  *   previousVersion: number=
  * }}
  */
 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.
  */
 function buildCardSet() {
   var cardShowPrefix = 'card-show-';
   var cardHidePrefix = 'card-hide-';
 
   /**
    * Schedules hiding a notification.
    * @param {string} cardId Card ID.
@@ skipping 27 matching lines @@
           cardCreateInfo.notification,
           function(newNotificationId) {
             if (!newNotificationId || chrome.runtime.lastError) {
               var errorMessage = chrome.runtime.lastError &&
                                   chrome.runtime.lastError.message;
               console.error('notifications.create: ID=' + newNotificationId +
                             ', ERROR=' + errorMessage);
               return;
             }
 
-              scheduleHiding(cardId, cardCreateInfo.hideTime);
+            scheduleHiding(cardId, cardCreateInfo.hideTime);
           });
     } else {
       // Update existing notification.
       instrumented.notifications.update(
           cardId,
           cardCreateInfo.notification,
           function(wasUpdated) {
             if (!wasUpdated || chrome.runtime.lastError) {
               var errorMessage = chrome.runtime.lastError &&
                                   chrome.runtime.lastError.message;
               console.error('notifications.update: UPDATED=' + wasUpdated +
                             ', ERROR=' + errorMessage);
               return;
             }
 
-              scheduleHiding(cardId, cardCreateInfo.hideTime);
+            scheduleHiding(cardId, cardCreateInfo.hideTime);
           });
     }
   }
 
   /**
    * Updates/creates a card notification with new data.
    * @param {string} cardId Card ID.
    * @param {MergedCard} card Google Now from the server.
    * @param {number=} previousVersion The version of the shown card with
    *     this id, if it exists, undefined otherwise.
@@ skipping 32 matching lines @@
     return {
       actionUrls: card.actionUrls,
       cardCreateInfo: cardCreateInfo,
       dismissals: card.dismissals
     };
   }
 
   /**
    * 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) {
     console.log('cardManager.onAlarm ' + JSON.stringify(alarm));
 
     if (alarm.name.indexOf(cardShowPrefix) == 0) {
       // Alarm to show the card.
       var cardId = alarm.name.substring(cardShowPrefix.length);
       instrumented.storage.local.get('notificationsData', function(items) {
         console.log('cardManager.onAlarm.get ' + JSON.stringify(items));
         if (!items || !items.notificationsData)
           return;
         var notificationData = items.notificationsData[cardId];
         if (!notificationData)
           return;
 
         showNotification(cardId, notificationData.cardCreateInfo);
       });
     } 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);
+      });
     }
   });
 
   return {
     update: update,
     clear: clear
   };
 }