Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
207243002:
Google Now Card Processing Pipeline Refactor (Closed)
|
Created:
6 years, 9 months ago by robliao Modified:
6 years, 8 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionGoogle Now Card Processing Pipeline Refactor
This major refactor covers two themes:
1) Use promises to link each pipeline stage together.
2) Decouple the pipeline ordering from each individual function.
Along with that, added unit tests that cover various
stages of the pipeline in isolation.
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261553
Patch Set 1 #Patch Set 2 : Rebase to r260580 #
Total comments: 19
Patch Set 3 : CR Feedback #
Total comments: 1
Patch Set 4 : Capitalization #Patch Set 5 : Bugfix #
Total comments: 4
Patch Set 6 : CR Feedback #Patch Set 7 : Fix Unit Test #Patch Set 8 : Rebase Unit Tests #
Messages
Total messages: 28 (0 generated)
Rebase to r260580
main file, lots o nitpicky https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:459: * Combines notification groups into a set of Chrome notifications and shows minus "and shows them" https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:465: function calculateCards(notificationGroups) { Not liking the name of this. I would like combineCards but there's confusion with combineGroup. The distinction here is between a single group and multiple groups. More verbose, but possible combineCardsFromGroups? https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:477: * Based on a response from the notification server, shows notifications and doesn't show notifications, update comment https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:590: var isBelowThreshold = need the extra var? https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:675: 'getGroupsToRequest-storage-get ' + JSON.stringify(items)); move onto previous line https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:696: return isGoogleNowEnabled().then(function(googleNowEnabled) { The indentation on all the thens is off. Also easier to read if all of the thens start on a new line. (I realize now that there's been skimping on indentation from .s, but this one makes it really obvious) https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1162: localStorage.optedInCheckCount++; Why outside the if? also, counting before = one less. https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1164: optInCheckAttempts.planForNext(function() {}); Make the param opt?
https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:459: * Combines notification groups into a set of Chrome notifications and shows On 2014/04/01 23:51:48, rgustafson wrote: > minus "and shows them" Done. https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:465: function calculateCards(notificationGroups) { On 2014/04/01 23:51:48, rgustafson wrote: > Not liking the name of this. > > I would like combineCards but there's confusion with combineGroup. The > distinction here is between a single group and multiple groups. More verbose, > but possible combineCardsFromGroups? Done. https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:477: * Based on a response from the notification server, shows notifications and On 2014/04/01 23:51:48, rgustafson wrote: > doesn't show notifications, update comment Done. https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:590: var isBelowThreshold = Yup. Seemed the lesser of three evils: // Breaks on lint due to line break after return. return localStorage['explanatoryCardsShown'] < EXPLANATORY_CARDS_LINK_THRESHOLD; // Just looks ugly return localStorage['explanatoryCardsShown'] < EXPLANATORY_CARDS_LINK_THRESHOLD; On 2014/04/01 23:51:48, rgustafson wrote: > need the extra var? https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:675: 'getGroupsToRequest-storage-get ' + JSON.stringify(items)); On 2014/04/01 23:51:48, rgustafson wrote: > move onto previous line Done. https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:696: return isGoogleNowEnabled().then(function(googleNowEnabled) { After a fun discussion, we're going to punt the issue by putting the first then on the next line. On 2014/04/01 23:51:48, rgustafson wrote: > The indentation on all the thens is off. Also easier to read if all of the thens > start on a new line. > > (I realize now that there's been skimping on indentation from .s, but this one > makes it really obvious) https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1162: localStorage.optedInCheckCount++; :-D. Nice catch. This was fixed but never uploaded. On 2014/04/01 23:51:48, rgustafson wrote: > Why outside the if? also, counting before = one less. https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:1164: optInCheckAttempts.planForNext(function() {}); Maybe later. I want to rework the alarms. On 2014/04/01 23:51:48, rgustafson wrote: > Make the param opt?
https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:590: var isBelowThreshold = Huh, that's supposed to be... // Just looks ugly return localStorage['explanatoryCardsShown'] < EXPLANATORY_CARDS_LINK_THRESHOLD; On 2014/04/02 00:42:05, robliao wrote: > Yup. Seemed the lesser of three evils: > > // Breaks on lint due to line break after return. > return > localStorage['explanatoryCardsShown'] < EXPLANATORY_CARDS_LINK_THRESHOLD; > > // Just looks ugly > return localStorage['explanatoryCardsShown'] < EXPLANATORY_CARDS_LINK_THRESHOLD; > > > On 2014/04/01 23:51:48, rgustafson wrote: > > need the extra var? >
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/207243002/40001
The CQ bit was unchecked by commit-bot@chromium.org
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.
lgtm https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background_unittest.gtestjs:884: TEST_F(TEST_NAME, 'showNotificationGroups', function() { show -> Show https://codereview.chromium.org/207243002/diff/40001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/207243002/diff/40001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:695: return googleNowEnabled ? Promise.resolve() : requestAndUpdateOptedIn(); [no action] didn't have to resort to variable renaming ;)
https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... File chrome/browser/resources/google_now/background_unittest.gtestjs (right): https://codereview.chromium.org/207243002/diff/20001/chrome/browser/resources... chrome/browser/resources/google_now/background_unittest.gtestjs:884: TEST_F(TEST_NAME, 'showNotificationGroups', function() { On 2014/04/02 18:14:06, rgustafson wrote: > show -> Show Done.
Bugfix
https://codereview.chromium.org/207243002/diff/80001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/207243002/diff/80001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:491: // Allow this to continue. We still need to do work here to clear On first read, I thought this data would be cleared by stopPollingCards, but that never gets called. Why not build card data removal into onStateChange when we will get into the opt in request state? Most of the work lower in this function and all work in the next then() does not need to be performed. Here and line 658 require scheduleNextPoll(anything, false) functionality. I'd tear the false part of the if statement into a separate function, call that and then continue to reject rather than this fall through.
https://codereview.chromium.org/207243002/diff/80001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/207243002/diff/80001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:491: // Allow this to continue. We still need to do work here to clear I fell into the same trap. The optin portions here are tricky (and worth clearing up). It's an excellent thing to do on a future refactor. On 2014/04/02 22:23:09, rgustafson wrote: > On first read, I thought this data would be cleared by stopPollingCards, but > that never gets called. Why not build card data removal into onStateChange when > we will get into the opt in request state? Most of the work lower in this > function and all work in the next then() does not need to be performed. > > Here and line 658 require scheduleNextPoll(anything, false) functionality. I'd > tear the false part of the if statement into a separate function, call that and > then continue to reject rather than this fall through.
cool, looking good, just one question (am partway through tests now, they're looking good too) https://codereview.chromium.org/207243002/diff/80001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/207243002/diff/80001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:967: chrome.storage.local.clear(); this doesn't clear localStorage, correct? They are stored in different places IIRC and chrome.storage.local is richer, but I'm not 100% on the edge cases and interactions. if localStorage is affected, do we want to keep explanatoryCardsShown? if localStorage is unaffected, maybe update the comment as to what it clears. more generally would this be unexpected for future developers?
https://codereview.chromium.org/207243002/diff/80001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/207243002/diff/80001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:967: chrome.storage.local.clear(); localStorage is unaffected with this clearing. The code currently stores runtime information in chrome.storage.local (cards, timer info, signed in info). localStorage, on the other hand, has been used for persistent things (number of explanatory cards, debug info) and for counters. I've added a comment. On 2014/04/03 00:11:59, Travis Skare wrote: > this doesn't clear localStorage, correct? They are stored in different places > IIRC and chrome.storage.local is richer, but I'm not 100% on the edge cases and > interactions. > > if localStorage is affected, do we want to keep explanatoryCardsShown? > if localStorage is unaffected, maybe update the comment as to what it clears. > > more generally would this be unexpected for future developers?
lgtm sg, thanks
xiyuan: Please provide owner approval of this CL. Thanks!
Fix unit test.
stampy LGTM
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/207243002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Commit queue rejected this change because the description was changed between the time the change entered the commit queue and the time it was ready to commit. You can safely check the commit box again.
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/207243002/140001
Message was sent while issue was closed.
Change committed as 261553 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
