Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25097002:
Cleaning data associated with the card upon deletion. (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. |
DescriptionCleaning data associated with the card upon deletion.
BUG=164227
TEST=No
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228055
Patch Set 1 #Patch Set 2 : Cosmetics #
Total comments: 10
Patch Set 3 : CR comments. #Patch Set 4 : CR comments. #Patch Set 5 : Reuploading thanks to glitch. #Patch Set 6 : Reuploading again #Patch Set 7 : ...and again #Patch Set 8 : Rebasing #Patch Set 9 : Fixing missing argument. #
Messages
Total messages: 25 (0 generated)
lgtm
https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards.js:194: delete items.notificationsData[cardId]; 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?
https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards_unittest.gtestjs (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards_unittest.gtestjs:405: tasks_add(tasksAddSavedArgs.match(eq(CLEAR_CARD_TASK_NAME)), tasksAddSavedArgs.match(ANYTHING)). Linebreak
https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards.js:194: delete items.notificationsData[cardId]; On 2013/09/27 18:57:20, rgustafson wrote: > 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? We'll never attempt to access this data after clearing. https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards_unittest.gtestjs (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards_unittest.gtestjs:405: tasks_add(tasksAddSavedArgs.match(eq(CLEAR_CARD_TASK_NAME)), tasksAddSavedArgs.match(ANYTHING)). On 2013/09/27 19:47:11, Robert Liao wrote: > Linebreak Done.
https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards.js:194: delete items.notificationsData[cardId]; On 2013/09/27 20:10:56, vadimt wrote: > On 2013/09/27 18:57:20, rgustafson wrote: > > 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? > > We'll never attempt to access this data after clearing. Then style says to set to null instead of delete. http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#delete
https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards.js:194: delete items.notificationsData[cardId]; On 2013/09/27 20:19:28, rgustafson wrote: > On 2013/09/27 20:10:56, vadimt wrote: > > On 2013/09/27 18:57:20, rgustafson wrote: > > > 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? > > > > We'll never attempt to access this data after clearing. > > Then style says to set to null instead of delete. > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#delete "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.
https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards.js:194: delete items.notificationsData[cardId]; On 2013/09/27 20:25:24, vadimt wrote: > On 2013/09/27 20:19:28, rgustafson wrote: > > On 2013/09/27 20:10:56, vadimt wrote: > > > On 2013/09/27 18:57:20, rgustafson wrote: > > > > 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? > > > > > > We'll never attempt to access this data after clearing. > > > > Then style says to set to null instead of delete. > > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#delete > > "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. 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.
lgtm
https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards.js:194: delete items.notificationsData[cardId]; 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: > On 2013/09/27 20:25:24, vadimt wrote: > > On 2013/09/27 20:19:28, rgustafson wrote: > > > On 2013/09/27 20:10:56, vadimt wrote: > > > > On 2013/09/27 18:57:20, rgustafson wrote: > > > > > 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? > > > > > > > > We'll never attempt to access this data after clearing. > > > > > > Then style says to set to null instead of delete. > > > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#delete > > > > "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. > > 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.
https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards.js:194: delete items.notificationsData[cardId]; 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.
https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... File chrome/browser/resources/google_now/cards.js (right): https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g... chrome/browser/resources/google_now/cards.js:194: delete items.notificationsData[cardId]; On 2013/09/30 16:56:17, vadimt wrote: > 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. 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.
+arv@ arv@, while your CR is not required, could you look at the particular issue with deleting vs. nulling map entries? I strongly believe that we should delete, not null, entries from maps. Your opinion? See here: https://codereview.chromium.org/25097002/diff/3001/chrome/browser/resources/g...
LGTM delete is the right thing to use if the object is used as a map.
lgtm
Thanks everyone for reviews! arv@, thanks for your comment!
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/25097002/9001
Failed to apply patch for chrome/browser/resources/google_now/cards.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/resources/google_now/cards.js
Hunk #2 FAILED at 104.
Hunk #3 FAILED at 120.
2 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/resources/google_now/cards.js.rej
Patch: 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];
+
+ 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);
+ });
}
});
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/25097002/54001
Retried try job too often on win_rel for step(s) browser_tests, chrome_frame_net_tests, chrome_frame_tests, chrome_frame_unittests, content_browsertests, mini_installer_test, nacl_integration, sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/25097002/54001
Message was sent while issue was closed.
Change committed as 228055 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
