Processing timefences from the server.

Processing timefences from the server.
Timefences, or triggers, are commands inside cards data that tell the client to show / hide the card at certain time without making a server request.

BUG=164227
TEST=Need to discuss in person.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214058

[rgustafson, 2013-07-19 23:37:12]

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:118:
tasks.instrumentApiFunction(chrome.alarms, 'getAll', 0);
Where is alarms.getAll used?

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:260: // TODO(vadimt): Figure
out what to do when notifications are disabled for
Why was this un-tabbed? It should be on the level on console.log

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards.js:15: var cardShowPrefixLength =
cardShowPrefix.length;
Does this actually save anything with the page being loaded and unloaded often?

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards.js:107:
chrome.alarms.clear(cardHidePrefix + card.notificationId);
I don't like the fact that this (and a few other places where clear is called)
fills the console log with errors every time there is a refresh because of
failed alarm deletes. Check that the alarm exists before deleting?

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards.js:121:
chrome.notifications.clear(card.notificationId, function() {});
This is a weird case. What if there is a low priority version of the card and we
want to show a high priority version at the given time? It doesn't seem like
clearing is the right thing here.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/cards_unittest.gtestjs (right):

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards_unittest.gtestjs:316:
TEST_F('GoogleNowCardsUnitTest', 'onAlarmUnrecognized', function() {
This test isn't really doing anything?

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards_unittest.gtestjs:348: 1, {
The { on this line looks off. new line?

https://codereview.chromium.org/19749007/diff/2001/chrome/test/data/webui/tes...
File chrome/test/data/webui/test_api.js (right):

https://codereview.chromium.org/19749007/diff/2001/chrome/test/data/webui/tes...
chrome/test/data/webui/test_api.js:1608: * value iff their JSON represenations
are same.
Following line *s should line up with the first * after /

[rgustafson, 2013-07-22 17:48:07]

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:244: if (typeof
parsedResponse.expiration_timestamp_seconds != 'number') {
Can we change expiration_timestamp_seconds to next_poll_seconds? Right now it is
being sent as both by the server. The former doesn't make any sense since its
not a timestamp.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:315:
updateCardsAttempts.start(parsedResponse.expiration_timestamp_seconds);
expiration_timestamp_seconds to next_poll_seconds

[vadimt, 2013-07-22 19:22:51]

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:118:
tasks.instrumentApiFunction(chrome.alarms, 'getAll', 0);
On 2013/07/19 23:37:12, rgustafson wrote:
> Where is alarms.getAll used?

Done.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:244: if (typeof
parsedResponse.expiration_timestamp_seconds != 'number') {
On 2013/07/22 17:48:07, rgustafson wrote:
> Can we change expiration_timestamp_seconds to next_poll_seconds? Right now it
is
> being sent as both by the server. The former doesn't make any sense since its
> not a timestamp.

Done.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:260: // TODO(vadimt): Figure
out what to do when notifications are disabled for
On 2013/07/19 23:37:12, rgustafson wrote:
> Why was this un-tabbed? It should be on the level on console.log

Done. I probably need to see my doctor.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/background.js:315:
updateCardsAttempts.start(parsedResponse.expiration_timestamp_seconds);
On 2013/07/22 17:48:07, rgustafson wrote:
> expiration_timestamp_seconds to next_poll_seconds

Done.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards.js:15: var cardShowPrefixLength =
cardShowPrefix.length;
On 2013/07/19 23:37:12, rgustafson wrote:
> Does this actually save anything with the page being loaded and unloaded
often?

Done.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards.js:107:
chrome.alarms.clear(cardHidePrefix + card.notificationId);
On 2013/07/19 23:37:12, rgustafson wrote:
> I don't like the fact that this (and a few other places where clear is called)
> fills the console log with errors every time there is a refresh because of
> failed alarm deletes. Check that the alarm exists before deleting?

Added TODO item. With status quo this is hard to do because the task execution
will fork here. Current tasks are linear.
But as we switch to the model when you don't have to invoke task-provided
callback, this will become easy.
Or, a'll simply speak to extension folks, asking them to print these diags as
non-error output.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards.js:121:
chrome.notifications.clear(card.notificationId, function() {});
On 2013/07/19 23:37:12, rgustafson wrote:
> This is a weird case. What if there is a low priority version of the card and
we
> want to show a high priority version at the given time? It doesn't seem like
> clearing is the right thing here.

Done.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
File chrome/browser/resources/google_now/cards_unittest.gtestjs (right):

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards_unittest.gtestjs:316:
TEST_F('GoogleNowCardsUnitTest', 'onAlarmUnrecognized', function() {
On 2013/07/19 23:37:12, rgustafson wrote:
> This test isn't really doing anything?

This alarm handler will get all alarms, for example, dismissal ones. We want to
check that we don't do anything on such alarms.

https://codereview.chromium.org/19749007/diff/2001/chrome/browser/resources/g...
chrome/browser/resources/google_now/cards_unittest.gtestjs:348: 1, {
On 2013/07/19 23:37:12, rgustafson wrote:
> The { on this line looks off. new line?

Done.

https://codereview.chromium.org/19749007/diff/2001/chrome/test/data/webui/tes...
File chrome/test/data/webui/test_api.js (right):

https://codereview.chromium.org/19749007/diff/2001/chrome/test/data/webui/tes...
chrome/test/data/webui/test_api.js:1608: * value iff their JSON represenations
are same.
On 2013/07/19 23:37:12, rgustafson wrote:
> Following line *s should line up with the first * after /

Done.

[rgustafson, 2013-07-23 18:14:02]

https://codereview.chromium.org/19749007/diff/10001/chrome/test/data/webui/te...
File chrome/test/data/webui/test_api.js (right):

https://codereview.chromium.org/19749007/diff/10001/chrome/test/data/webui/te...
chrome/test/data/webui/test_api.js:1618: * Checks that JSON represenation of the
actual and expected arguments are
Here and everything below too.

[skare_, 2013-07-23 19:55:55]

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:104: // this doesn't output an
error to console.
up to you, but this might be worth checking for this cl.

https://codereview.chromium.org/19749007/diff/27001/chrome/test/data/webui/te...
File chrome/test/data/webui/test_api.js (right):

https://codereview.chromium.org/19749007/diff/27001/chrome/test/data/webui/te...
chrome/test/data/webui/test_api.js:1618: * Checks that JSON represenation of the
actual and expected arguments are
check alignment of *'s here and next two functions down.

[vadimt, 2013-07-23 20:03:27]

https://codereview.chromium.org/19749007/diff/10001/chrome/test/data/webui/te...
File chrome/test/data/webui/test_api.js (right):

https://codereview.chromium.org/19749007/diff/10001/chrome/test/data/webui/te...
chrome/test/data/webui/test_api.js:1618: * Checks that JSON represenation of the
actual and expected arguments are
On 2013/07/23 18:14:02, rgustafson wrote:
> Here and everything below too.

Done.

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:104: // this doesn't output an
error to console.
On 2013/07/23 19:55:56, Travis Skare wrote:
> up to you, but this might be worth checking for this cl.

This depends on another CL (task improvements) that allows branching tasks.
Or, extension guys could make this message non-error.
Both are not a good match for this CL.

https://codereview.chromium.org/19749007/diff/27001/chrome/test/data/webui/te...
File chrome/test/data/webui/test_api.js (right):

https://codereview.chromium.org/19749007/diff/27001/chrome/test/data/webui/te...
chrome/test/data/webui/test_api.js:1618: * Checks that JSON represenation of the
actual and expected arguments are
On 2013/07/23 19:55:56, Travis Skare wrote:
> check alignment of *'s here and next two functions down.

Done.

[rgustafson, 2013-07-23 21:20:40]

lgtm

[skare_, 2013-07-23 21:25:52]

couple more high level things, might not require any action, just sending out
because I have to review the test still and we can parallelize.

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:35: * @param {Object}
cardCreateInfo Google Now card represented as a set of
worth typedef'ing this or documenting the structure? Or will the scope be
limited to pretty much what's in the code at this point?

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:104: // this doesn't output an
error to console.
On 2013/07/23 20:03:28, vadimt wrote:
> On 2013/07/23 19:55:56, Travis Skare wrote:
> > up to you, but this might be worth checking for this cl.
> 
> This depends on another CL (task improvements) that allows branching tasks.
> Or, extension guys could make this message non-error.
> Both are not a good match for this CL.

[no action required]
ah ok, I think I misunderstood "make sure" -- my original reading was that you'd
want to experiment with clearing nonexistent alarms to see what the outcome was,
rather than already knowing it would raise an exception or log an error.

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:147:
chrome.alarms.clear(cardHidePrefix + cardId);
similar to the above, the behavior for a nonexistent alarm is to console.error
but not suspend execution? that is, both of these statements will run even if
(showPrefix+id) is not an alarm?

[vadimt, 2013-07-23 22:42:33]

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:35: * @param {Object}
cardCreateInfo Google Now card represented as a set of
On 2013/07/23 21:25:53, Travis Skare wrote:
> worth typedef'ing this or documenting the structure? Or will the scope be
> limited to pretty much what's in the code at this point?

1. Scope will be as limited as it is now. This structure won't be used outside
of card manager.
2. The structure contains notification fields received from the server, so we
anyways cannot fully define it.

https://codereview.chromium.org/19749007/diff/27001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:147:
chrome.alarms.clear(cardHidePrefix + cardId);
On 2013/07/23 21:25:53, Travis Skare wrote:
> similar to the above, the behavior for a nonexistent alarm is to console.error
> but not suspend execution? that is, both of these statements will run even if
> (showPrefix+id) is not an alarm?

It's all about red diagnostic output. But the execution happily continues.

[skare_, 2013-07-24 23:49:36]

lgtm

[vadimt, 2013-07-25 00:20:15]

OWNERs, please provide approvals:

arv@:
  chrome/browser/resources/*

sky@:
  chrome/chrome_tests_unit.gypi
  chrome/test/data/webui/test_api.js

NOTES:
1. Changes in test_api.js are simply moving code from background_test_util.js
2. sky@, IIRC, you are not a JS person. arv@, can I ask you to look at
test_api.js changes, while sky@ could just provide a rubberstamp for it?

[sky, 2013-07-25 14:51:22]

LGTM as long as arv is happy

https://codereview.chromium.org/19749007/diff/37001/chrome/test/data/webui/te...
File chrome/test/data/webui/test_api.js (right):

https://codereview.chromium.org/19749007/diff/37001/chrome/test/data/webui/te...
chrome/test/data/webui/test_api.js:1634: }
nit: add a trailing ',' (same as 323 above).

[vadimt, 2013-07-25 20:15:20]

https://codereview.chromium.org/19749007/diff/37001/chrome/test/data/webui/te...
File chrome/test/data/webui/test_api.js (right):

https://codereview.chromium.org/19749007/diff/37001/chrome/test/data/webui/te...
chrome/test/data/webui/test_api.js:1634: }
On 2013/07/25 14:51:23, sky wrote:
> nit: add a trailing ',' (same as 323 above).

Done.

[vadimt, 2013-07-25 20:23:22]

arv@ is OOF.
xiyuan, please provide OWNER's review (please see what I asked arv@ to do in an
earlier note)

[arv (Not doing code reviews), 2013-07-25 21:45:27]

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:5: 'use strict';
Are these files getting concatted?

Usually, it is bad to put 'use strict' at the top level. Can you put this in an
IIFE?

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:13: function buildCardManager() {
don't name things "manager"

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:13: function buildCardManager() {
Use constructor and prototypes if you need multiple objects.

Use the module pattern for a single object

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards_unittest.gtestjs (right):

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards_unittest.gtestjs:24: var
testNotification = { testNotificationField: 'TEST NOTIFICATION VALUE' };
no ws after { nor before }

[vadimt, 2013-07-25 21:48:29]

Oops, arv@ is in today. xiyuan, please do nothing for now!

[vadimt, 2013-07-25 22:57:20]

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:5: 'use strict';
On 2013/07/25 21:45:27, arv wrote:
> Are these files getting concatted?
> 
> Usually, it is bad to put 'use strict' at the top level. Can you put this in
an
> IIFE?

This is one of a page's scripts. Another script of this page depends on its
functions.
I tried surrounding the whole file with an anonymous function, but then exported
methods become local, and I cannot use them outside of the file.
Did you mean something different from a huge anonymous function?

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:13: function buildCardManager() {
On 2013/07/25 21:45:27, arv wrote:
> don't name things "manager"

Done.

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:13: function buildCardManager() {
On 2013/07/25 21:45:27, arv wrote:
> Use constructor and prototypes if you need multiple objects.
> 
> Use the module pattern for a single object

Module doesn't work well. It creates a variable right here.
However, in my unit tests, I want to call buildCardManager() and check that this
method has correct side effects (like hooking to certain events). I cannot
beautifully do this with a module since by the time a test runs, the module is
already created.

Alternatively, I could declare this as a constructor and a prototype, but this
is inadequate because I create only 1 instance. So, it seems like the current
approach is the most adequate.
Convincing?

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards_unittest.gtestjs (right):

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards_unittest.gtestjs:24: var
testNotification = { testNotificationField: 'TEST NOTIFICATION VALUE' };
On 2013/07/25 21:45:27, arv wrote:
> no ws after { nor before }

Done.

[skare_, 2013-07-26 00:57:29]

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:5: 'use strict';
On 2013/07/25 22:57:20, vadimt wrote:
> On 2013/07/25 21:45:27, arv wrote:
> > Are these files getting concatted?
> > 
> > Usually, it is bad to put 'use strict' at the top level. Can you put this in
> an
> > IIFE?
> 
> This is one of a page's scripts. Another script of this page depends on its
> functions.
> I tried surrounding the whole file with an anonymous function, but then
exported
> methods become local, and I cannot use them outside of the file.
> Did you mean something different from a huge anonymous function?

(function(){
  window['thefunction'] = thefunction?
})();
?
for reference see also goog.exportSymbol and @export from Closure (though we
don't use it here)

[vadimt, 2013-07-26 01:08:54]

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
File chrome/browser/resources/google_now/cards.js (right):

https://codereview.chromium.org/19749007/diff/47001/chrome/browser/resources/...
chrome/browser/resources/google_now/cards.js:5: 'use strict';
On 2013/07/26 00:57:29, Travis Skare wrote:
> On 2013/07/25 22:57:20, vadimt wrote:
> > On 2013/07/25 21:45:27, arv wrote:
> > > Are these files getting concatted?
> > > 
> > > Usually, it is bad to put 'use strict' at the top level. Can you put this
in
> > an
> > > IIFE?
> > 
> > This is one of a page's scripts. Another script of this page depends on its
> > functions.
> > I tried surrounding the whole file with an anonymous function, but then
> exported
> > methods become local, and I cannot use them outside of the file.
> > Did you mean something different from a huge anonymous function?
> 
> (function(){
>   window['thefunction'] = thefunction?
> })();
> ?
> for reference see also goog.exportSymbol and @export from Closure (though we
> don't use it here)

Since I use unit testing, I want to export every function, to be able to test
and mock individual functions. Then, the code (if we apply this rule everywhere)
would become so ugly that the benefits of placing 'use strict' in an IIFE fade.

Speaking of benefits. Given that this code entirely belongs to the extension and
won't be used outside of it, there are not so many benefits in such isolation...

Anyways, technically, I can do what you ask, but I feel that the bottom line
will be losing rather than gaining.

Just let me know if you confirm that you still want me to do this.

[xiyuan, 2013-07-26 17:40:31]

LGTM

[commit-bot: I haz the power, 2013-07-26 17:47:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vadimt@chromium.org/19749007/57001

[commit-bot: I haz the power, 2013-07-27 23:49:32]

Change committed as 214058