Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11967029:
Initial implementation of Google Now Notifications integration (Closed)
|
Created:
7 years, 11 months ago by vadimt Modified:
7 years, 10 months ago CC:
chromium-reviews, Aaron Boodman, arv (Not doing code reviews), chromium-apps-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitial implementation of Google Now Notifications integration.
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=179497
Patch Set 1 #Patch Set 2 : Cosmetics. #Patch Set 3 : . #Patch Set 4 : Improving initialization. #Patch Set 5 : Cleanup of the initialization. #
Total comments: 22
Patch Set 6 : CL comments #Patch Set 7 : CR comments #
Total comments: 6
Patch Set 8 : #
Total comments: 3
Patch Set 9 : #
Total comments: 20
Patch Set 10 : arv, mpcomplete and sky reviews. #Patch Set 11 : nit #Patch Set 12 : . #
Total comments: 2
Patch Set 13 : arv comments #Patch Set 14 : Fixing presubmit check. #
Messages
Total messages: 29 (0 generated)
thanks, here's some preliminaries. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:6: * @fileoverview The event page for Google Now for Chrome implementation. check indentation on block/jsdoc comments (here and below): /** * */ your IDE/editor can help with this, and it might also have been responsible for shoving everything to the first column in the first place :) https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:35: var HTTP_OK = 200; just 200 or 2xx (probably ok, for GETs that will complete in full, just checking)? if keeping, maybe update comment to reference that it's 200 OK and servers will only send this. HTTP 204 is a valid success response but means there is no content, for example. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:41: var DEFAULT_POLLING_PERIOD_SECONDS = 300; // 5 minutes tiny nit: 2 spaces between code and comments https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:50: var cards = parsedResponse.cards; guard against parsedResponse being empty or having no .cards? https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:76: * @param {object} position Location of this computer. nit: capitalize {Object} if you're using it, but consider a class so you can do @param {LocationInfo} if it gets more complicated. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:80: '?q=' + position.coords.latitude + just checking -- have cgiargs names been established? q= is often a query string or similar. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:89: * @param {object} positionError Position error. you can omit this param if you don't need it. You can also combine this function and its WithLocation friend and check an argument against undefined, but your call if you want to have the condition codified in the function name
Thanks! https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/01/18 21:05:24, Travis Skare wrote: > 2013 Done. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:6: * @fileoverview The event page for Google Now for Chrome implementation. On 2013/01/18 21:05:24, Travis Skare wrote: > check indentation on block/jsdoc comments (here and below): > /** > * > */ > your IDE/editor can help with this, and it might also have been responsible for > shoving everything to the first column in the first place :) Done. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:35: var HTTP_OK = 200; On 2013/01/18 21:05:24, Travis Skare wrote: > just 200 or 2xx (probably ok, for GETs that will complete in full, just > checking)? > > if keeping, maybe update comment to reference that it's 200 OK and servers will > only send this. HTTP 204 is a valid success response but means there is no > content, for example. Done. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:41: var DEFAULT_POLLING_PERIOD_SECONDS = 300; // 5 minutes On 2013/01/18 21:05:24, Travis Skare wrote: > tiny nit: 2 spaces between code and comments Done. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:50: var cards = parsedResponse.cards; On 2013/01/18 21:05:24, Travis Skare wrote: > guard against parsedResponse being empty or having no .cards? Then this will throw an exception, the execution will terminate. We'll still have an alarm scheduled in UpdateNotificationsCards(), and this will secure the next polling. In future CLs, I plan showing some diagnostics though. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:76: * @param {object} position Location of this computer. On 2013/01/18 21:05:24, Travis Skare wrote: > nit: capitalize {Object} if you're using it, but consider a class so you can do > @param {LocationInfo} > if it gets more complicated. Done. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:80: '?q=' + position.coords.latitude + On 2013/01/18 21:05:24, Travis Skare wrote: > just checking -- have cgiargs names been established? > q= is often a query string or similar. Added a TODO item. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:89: * @param {object} positionError Position error. On 2013/01/18 21:05:24, Travis Skare wrote: > you can omit this param if you don't need it. > > You can also combine this function and its WithLocation friend and check an > argument against undefined, but your call if you want to have the condition > codified in the function name I'd prefer to keep the argument for readability. Speaking of combining: we'd need to check the argument for its type (position or position error), which is not simple, and not quite readable. Note that we cannot anyways use HTML5 geolocation API with the event page, so this code is 100% temporary.
First comments, not yet done. Sending these and I will continue on the new revision. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:55: ScheduleNextUpdate(parsedResponse.expiration_timestamp_seconds); Also handle if expiration_timestamp_seconds isn't set (and don't try this if parsedResponse is empty as Travis said above). https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:64: // sure we never get responses like 'Select an account' page. Select account page is already gone. Change to "handle error responses"? The server can respond with errors in json format.
Many thanks! https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:55: ScheduleNextUpdate(parsedResponse.expiration_timestamp_seconds); On 2013/01/18 22:21:00, rgustafson wrote: > Also handle if expiration_timestamp_seconds isn't set (and don't try this if > parsedResponse is empty as Travis said above). The mentioned cases will cause an exception. So, I believe, this is already covered. https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:64: // sure we never get responses like 'Select an account' page. On 2013/01/18 22:21:00, rgustafson wrote: > Select account page is already gone. Change to "handle error responses"? The > server can respond with errors in json format. Done. And I already have an umbrella TODO for handling errors.
https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:55: ScheduleNextUpdate(parsedResponse.expiration_timestamp_seconds); On 2013/01/18 23:05:12, vadimt wrote: > On 2013/01/18 22:21:00, rgustafson wrote: > > Also handle if expiration_timestamp_seconds isn't set (and don't try this if > > parsedResponse is empty as Travis said above). > > The mentioned cases will cause an exception. So, I believe, this is already > covered. yeah, that will work but I think explicit error handling is preferred to depending on not catching exceptions for control flow
On 2013/01/18 23:21:24, Travis Skare wrote: > https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... > File chrome/browser/resources/google_now/background.js (right): > > https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... > chrome/browser/resources/google_now/background.js:55: > ScheduleNextUpdate(parsedResponse.expiration_timestamp_seconds); > On 2013/01/18 23:05:12, vadimt wrote: > > On 2013/01/18 22:21:00, rgustafson wrote: > > > Also handle if expiration_timestamp_seconds isn't set (and don't try this if > > > parsedResponse is empty as Travis said above). > > > > The mentioned cases will cause an exception. So, I believe, this is already > > covered. > > yeah, that will work but I think explicit error handling is preferred to > depending on not catching exceptions for control flow I believe this will be covered by the "Report errors to the user" TODO item.
the last response was by email, right? (can't see the comment)
maybe just add a check so it's explicit?
if (!parsedResponse || !parsedResponse.cards) {
return;
}
var card = parsedResponse.cards[i];
if someone else comes along and edits the code, they might not notice the
check is implicitly handled with an exception a few lines later, and could
perform half an operation.
On Fri, Jan 18, 2013 at 3:24 PM, <vadimt@chromium.org> wrote:
> On 2013/01/18 23:21:24, Travis Skare wrote:
>
> https://codereview.chromium.**org/11967029/diff/16001/**
>
chrome/browser/resources/**google_now/background.js<https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/google_now/background.js>
>
>> File chrome/browser/resources/**google_now/background.js (right):
>>
>
>
> https://codereview.chromium.**org/11967029/diff/16001/**
>
chrome/browser/resources/**google_now/background.js#**newcode55<https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/google_now/background.js#newcode55>
>
>> chrome/browser/resources/**google_now/background.js:55:
>> ScheduleNextUpdate(**parsedResponse.expiration_**timestamp_seconds);
>> On 2013/01/18 23:05:12, vadimt wrote:
>> > On 2013/01/18 22:21:00, rgustafson wrote:
>> > > Also handle if expiration_timestamp_seconds isn't set (and don't try
>> this
>>
> if
>
>> > > parsedResponse is empty as Travis said above).
>> >
>> > The mentioned cases will cause an exception. So, I believe, this is
>> already
>> > covered.
>>
>
> yeah, that will work but I think explicit error handling is preferred to
>> depending on not catching exceptions for control flow
>>
>
> I believe this will be covered by the "Report errors to the user" TODO
> item.
>
>
https://codereview.chromium.**org/11967029/<https://codereview.chromium.org/1...
>
On 2013/01/18 23:57:53, skare wrote:
> the last response was by email, right? (can't see the comment)
>
> maybe just add a check so it's explicit?
> if (!parsedResponse || !parsedResponse.cards) {
> return;
> }
> var card = parsedResponse.cards[i];
>
> if someone else comes along and edits the code, they might not notice the
> check is implicitly handled with an exception a few lines later, and could
> perform half an operation.
>
>
>
> On Fri, Jan 18, 2013 at 3:24 PM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=vadimt@chromium.org> wrote:
>
> > On 2013/01/18 23:21:24, Travis Skare wrote:
> >
> > https://codereview.chromium.**org/11967029/diff/16001/**
> >
>
chrome/browser/resources/**google_now/background.js<https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/google_now/background.js>
> >
> >> File chrome/browser/resources/**google_now/background.js (right):
> >>
> >
> >
> > https://codereview.chromium.**org/11967029/diff/16001/**
> >
>
chrome/browser/resources/**google_now/background.js#**newcode55<https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/google_now/background.js#newcode55>
> >
> >> chrome/browser/resources/**google_now/background.js:55:
> >> ScheduleNextUpdate(**parsedResponse.expiration_**timestamp_seconds);
> >> On 2013/01/18 23:05:12, vadimt wrote:
> >> > On 2013/01/18 22:21:00, rgustafson wrote:
> >> > > Also handle if expiration_timestamp_seconds isn't set (and don't try
> >> this
> >>
> > if
> >
> >> > > parsedResponse is empty as Travis said above).
> >> >
> >> > The mentioned cases will cause an exception. So, I believe, this is
> >> already
> >> > covered.
> >>
> >
> > yeah, that will work but I think explicit error handling is preferred to
> >> depending on not catching exceptions for control flow
> >>
> >
> > I believe this will be covered by the "Report errors to the user" TODO
> > item.
> >
> >
>
https://codereview.chromium.**org/11967029/%3Chttps://codereview.chromium.org...>
> >
Done.
https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/16001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:55: ScheduleNextUpdate(parsedResponse.expiration_timestamp_seconds); On 2013/01/18 23:21:24, Travis Skare wrote: > On 2013/01/18 23:05:12, vadimt wrote: > > On 2013/01/18 22:21:00, rgustafson wrote: > > > Also handle if expiration_timestamp_seconds isn't set (and don't try this if > > > parsedResponse is empty as Travis said above). > > > > The mentioned cases will cause an exception. So, I believe, this is already > > covered. > > yeah, that will work but I think explicit error handling is preferred to > depending on not catching exceptions for control flow Done.
https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", I'm surprised this doesn't throw an error or something. Or does it? Can we make this a comment, and explain which urls these are? https://codereview.chromium.org/11967029/diff/11005/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/11005/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:62: if (typeof parsedResponse.expiration_timestamp_seconds != 'number') If the timestamp isn't present, it should still display the cards, but use the default time. Move this below the cards loop, so this only prevents resetting the alarm.
https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", On 2013/01/22 21:10:28, rgustafson wrote: > I'm surprised this doesn't throw an error or something. Or does it? Can we make > this a comment, and explain which urls these are? it's a magic token; Vadim, if you want to comment it you can reference http://developer.chrome.com/extensions/match_patterns.html
https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", Oh, that is magical. Are we keeping it like this when we know all the urls we need? On 2013/01/22 21:41:58, Travis Skare wrote: > On 2013/01/22 21:10:28, rgustafson wrote: > > I'm surprised this doesn't throw an error or something. Or does it? Can we > make > > this a comment, and explain which urls these are? > > it's a magic token; Vadim, if you want to comment it you can reference > http://developer.chrome.com/extensions/match_patterns.html
https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", On 2013/01/22 21:10:28, rgustafson wrote: > I'm surprised this doesn't throw an error or something. Or does it? Can we make > this a comment, and explain which urls these are? Done. https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", On 2013/01/22 21:41:58, Travis Skare wrote: > On 2013/01/22 21:10:28, rgustafson wrote: > > I'm surprised this doesn't throw an error or something. Or does it? Can we > make > > this a comment, and explain which urls these are? > > it's a magic token; Vadim, if you want to comment it you can reference > http://developer.chrome.com/extensions/match_patterns.html Done. https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/... chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", On 2013/01/22 21:50:25, rgustafson wrote: > Oh, that is magical. Are we keeping it like this when we know all the urls we > need? > On 2013/01/22 21:41:58, Travis Skare wrote: > > On 2013/01/22 21:10:28, rgustafson wrote: > > > I'm surprised this doesn't throw an error or something. Or does it? Can we > > make > > > this a comment, and explain which urls these are? > > > > it's a magic token; Vadim, if you want to comment it you can reference > > http://developer.chrome.com/extensions/match_patterns.html > Done. https://codereview.chromium.org/11967029/diff/11005/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/11005/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:62: if (typeof parsedResponse.expiration_timestamp_seconds != 'number') On 2013/01/22 21:10:28, rgustafson wrote: > If the timestamp isn't present, it should still display the cards, but use the > default time. Move this below the cards loop, so this only prevents resetting > the alarm. The fact that the timestamp is not present most likely means that the whole server response is malformed. Should we then show notifications that are part of such response?
lgtm https://codereview.chromium.org/11967029/diff/11005/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/11005/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:62: if (typeof parsedResponse.expiration_timestamp_seconds != 'number') I'd prefer to give the user everything we have, and assume we would catch errors in the actual cards when looping through. This whole scenario was imagined and shouldn't actually be happening from the server side though, so it should be fine how it is. On 2013/01/23 03:04:05, vadimt wrote: > On 2013/01/22 21:10:28, rgustafson wrote: > > If the timestamp isn't present, it should still display the cards, but use the > > default time. Move this below the cards loop, so this only prevents resetting > > the alarm. > > The fact that the timestamp is not present most likely means that the whole > server response is malformed. Should we then show notifications that are part of > such response?
lgtm
mpcomplete, sky, please provide owner's approvals. mpcomplete, please review component_loader.cc sky, please review the rest. Thanks!
I'm not a good reviewer for the JS changes. Maybe arv? Everything else LGTM
handful of nits. otherwise LGTM https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:50: function ParseAndShowNotificationCards(response) { JavaScript style is to use camelCase naming for functions: http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:138: chrome.alarms.create({ when: Date.now() + delaySeconds * 1000 }); nit: could also use: chrome.alarms.create({delayInMinutes: delaySeconds / 60}); (or just specify the delay in minutes everywhere. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:154: Initialize(); Do you need to update the cards immediately on startup? Alarms are persisted across browser restart. That means if the alarm has expired since you last closed chrome, it will dispatch the event when you start chrome. If it hasn't expired yet, the alarm will fire when it expires. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/manifest.json:20: } nit: indent
https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:4: 'use strict'; https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:65: for (var i = 0; i != cards.length; ++i) { cards.forEach? https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:69: return; why? Please add comments every time you silence an exception. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:82: var request = new XMLHttpRequest(); You might want to set the responseType to text so that no Document is created. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:83: request.onreadystatechange = function(event) { you can do onload instead
Thanks reviewers! All changes except for the extension code are due to rebase. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:4: On 2013/01/25 19:29:06, arv wrote: > 'use strict'; Done. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:50: function ParseAndShowNotificationCards(response) { On 2013/01/24 18:34:38, Matt Perry wrote: > JavaScript style is to use camelCase naming for functions: > http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone... Done. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:65: for (var i = 0; i != cards.length; ++i) { On 2013/01/25 19:29:06, arv wrote: > cards.forEach? With forEach, the error processing code for notification.show() will be less clear. Here, I simply call 'return' from try-catch. With forEach, the code will be more tricky (bContinue flag etc.), or I'll have to place try-catch around whole loop, making code less readable (not clear who throws an exception that we are trying to catch). https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:69: return; On 2013/01/25 19:29:06, arv wrote: > why? Please add comments every time you silence an exception. Done. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:82: var request = new XMLHttpRequest(); On 2013/01/25 19:29:06, arv wrote: > You might want to set the responseType to text so that no Document is created. Done. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:83: request.onreadystatechange = function(event) { On 2013/01/25 19:29:06, arv wrote: > you can do onload instead Done. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:138: chrome.alarms.create({ when: Date.now() + delaySeconds * 1000 }); On 2013/01/24 18:34:38, Matt Perry wrote: > nit: could also use: chrome.alarms.create({delayInMinutes: delaySeconds / 60}); > > (or just specify the delay in minutes everywhere. Yes, but chrome.alarms sometimes rounds delay to a minute, and I used to see it rounding to 5 mins IIRC. By specifying absolute time, I avoid the issue. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:154: Initialize(); On 2013/01/24 18:34:38, Matt Perry wrote: > Do you need to update the cards immediately on startup? Alarms are persisted > across browser restart. That means if the alarm has expired since you last > closed chrome, it will dispatch the event when you start chrome. If it hasn't > expired yet, the alarm will fire when it expires. Correct. However, we specifically want to update the cards on the browser's startup. This is a product decision. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/manifest.json:20: } On 2013/01/24 18:34:38, Matt Perry wrote: > nit: indent Done.
https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:138: chrome.alarms.create({ when: Date.now() + delaySeconds * 1000 }); On 2013/01/25 21:48:28, vadimt wrote: > On 2013/01/24 18:34:38, Matt Perry wrote: > > nit: could also use: chrome.alarms.create({delayInMinutes: delaySeconds / > 60}); > > > > (or just specify the delay in minutes everywhere. > > Yes, but chrome.alarms sometimes rounds delay to a minute, and I used to see it > rounding to 5 mins IIRC. By specifying absolute time, I avoid the issue. That's not true. delayInMinutes is a double, so you can specify fractional values. There is still a minimum delay of 1 minute, but that applies when using "when" as well.
https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:138: chrome.alarms.create({ when: Date.now() + delaySeconds * 1000 }); On 2013/01/25 22:05:34, Matt Perry wrote: > On 2013/01/25 21:48:28, vadimt wrote: > > On 2013/01/24 18:34:38, Matt Perry wrote: > > > nit: could also use: chrome.alarms.create({delayInMinutes: delaySeconds / > > 60}); > > > > > > (or just specify the delay in minutes everywhere. > > > > Yes, but chrome.alarms sometimes rounds delay to a minute, and I used to see > it > > rounding to 5 mins IIRC. By specifying absolute time, I avoid the issue. > > That's not true. delayInMinutes is a double, so you can specify fractional > values. There is still a minimum delay of 1 minute, but that applies when using > "when" as well. You are right! Changed to delayInMinutes, but still want to have seconds across the code, since Google Now server sends seconds.
LGTM https://codereview.chromium.org/11967029/diff/42002/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/42002/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:149: chrome.alarms.create({ delayInMinutes: delaySeconds / 60 }); no space after { nor before } in single line object literals
Thanks reviewers! https://codereview.chromium.org/11967029/diff/42002/chrome/browser/resources/... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/42002/chrome/browser/resources/... chrome/browser/resources/google_now/background.js:149: chrome.alarms.create({ delayInMinutes: delaySeconds / 60 }); On 2013/01/29 19:28:46, arv wrote: > no space after { nor before } in single line object literals Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11967029/44001
Presubmit check for 11967029-44001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** See the JavaScript style guide at http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript and if you have any feedback about the JavaScript PRESUBMIT check, contact tbreisacher@chromium.org or dbeam@chromium.org ** Presubmit Warnings ** Found JavaScript style violations in chrome/browser/resources/google_now/background.js: line 1: E0010: Missing semicolon at end of line // Copyright (c) 2013 The Chromium Authors. All rights reserved. ^^^ Presubmit checks took 1.6s to calculate.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11967029/36002
Message was sent while issue was closed.
Change committed as 179497 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
