Chromium Code Reviews
Help | Chromium Project | Sign in
(37)

Issue 11967029: Initial implementation of Google Now Notifications integration (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 6 months ago by vadimt
Modified:
2 years, 6 months ago
CC:
chromium-reviews, Aaron Boodman, arv (Not doing code reviews), chromium-apps-reviews_chromium.org
Visibility:
Public.

Description

Initial 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)
vadimt
2 years, 6 months ago (2013-01-16 22:05:16 UTC) #1
skare_
thanks, here's some preliminaries. 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#newcode1 chrome/browser/resources/google_now/background.js:1: // Copyright (c) 2012 The ...
2 years, 6 months ago (2013-01-18 21:05:23 UTC) #2
vadimt
Thanks! 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#newcode1 chrome/browser/resources/google_now/background.js:1: // Copyright (c) 2012 The Chromium Authors. All ...
2 years, 6 months ago (2013-01-18 22:16:08 UTC) #3
rgustafson
First comments, not yet done. Sending these and I will continue on the new revision. ...
2 years, 6 months ago (2013-01-18 22:21:00 UTC) #4
vadimt
Many thanks! 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 chrome/browser/resources/google_now/background.js:55: ScheduleNextUpdate(parsedResponse.expiration_timestamp_seconds); On 2013/01/18 22:21:00, rgustafson wrote: > ...
2 years, 6 months ago (2013-01-18 23:05:11 UTC) #5
skare_
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 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 ...
2 years, 6 months ago (2013-01-18 23:21:24 UTC) #6
vadimt
On 2013/01/18 23:21:24, Travis Skare wrote: > 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 ...
2 years, 6 months ago (2013-01-18 23:24:44 UTC) #7
skare
the last response was by email, right? (can't see the comment) maybe just add a ...
2 years, 6 months ago (2013-01-18 23:57:53 UTC) #8
vadimt
On 2013/01/18 23:57:53, skare wrote: > the last response was by email, right? (can't see ...
2 years, 6 months ago (2013-01-19 00:47:13 UTC) #9
vadimt
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 chrome/browser/resources/google_now/background.js:55: ScheduleNextUpdate(parsedResponse.expiration_timestamp_seconds); On 2013/01/18 23:21:24, Travis Skare wrote: > On ...
2 years, 6 months ago (2013-01-19 00:51:21 UTC) #10
rgustafson
https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/google_now/manifest.json File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/google_now/manifest.json#newcode9 chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", I'm surprised this doesn't throw an error or ...
2 years, 6 months ago (2013-01-22 21:10:28 UTC) #11
skare_
https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/google_now/manifest.json File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/google_now/manifest.json#newcode9 chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", On 2013/01/22 21:10:28, rgustafson wrote: > I'm surprised ...
2 years, 6 months ago (2013-01-22 21:41:58 UTC) #12
rgustafson
https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/google_now/manifest.json File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/google_now/manifest.json#newcode9 chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", Oh, that is magical. Are we keeping it ...
2 years, 6 months ago (2013-01-22 21:50:24 UTC) #13
vadimt
https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/google_now/manifest.json File chrome/browser/resources/google_now/manifest.json (right): https://codereview.chromium.org/11967029/diff/21001/chrome/browser/resources/google_now/manifest.json#newcode9 chrome/browser/resources/google_now/manifest.json:9: "<all_urls>", On 2013/01/22 21:10:28, rgustafson wrote: > I'm surprised ...
2 years, 6 months ago (2013-01-23 03:04:04 UTC) #14
rgustafson
lgtm https://codereview.chromium.org/11967029/diff/11005/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/11005/chrome/browser/resources/google_now/background.js#newcode62 chrome/browser/resources/google_now/background.js:62: if (typeof parsedResponse.expiration_timestamp_seconds != 'number') I'd prefer to ...
2 years, 6 months ago (2013-01-23 23:18:56 UTC) #15
skare_
lgtm
2 years, 6 months ago (2013-01-24 01:30:24 UTC) #16
vadimt
mpcomplete, sky, please provide owner's approvals. mpcomplete, please review component_loader.cc sky, please review the rest. ...
2 years, 6 months ago (2013-01-24 01:51:18 UTC) #17
sky
I'm not a good reviewer for the JS changes. Maybe arv? Everything else LGTM
2 years, 6 months ago (2013-01-24 18:28:23 UTC) #18
Matt Perry
handful of nits. otherwise LGTM https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js#newcode50 chrome/browser/resources/google_now/background.js:50: function ParseAndShowNotificationCards(response) { JavaScript ...
2 years, 6 months ago (2013-01-24 18:34:38 UTC) #19
arv (Not doing code reviews)
https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js#newcode4 chrome/browser/resources/google_now/background.js:4: 'use strict'; https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js#newcode65 chrome/browser/resources/google_now/background.js:65: for (var i = 0; ...
2 years, 6 months ago (2013-01-25 19:29:05 UTC) #20
vadimt
Thanks reviewers! All changes except for the extension code are due to rebase. https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js File ...
2 years, 6 months ago (2013-01-25 21:48:28 UTC) #21
Matt Perry
https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js#newcode138 chrome/browser/resources/google_now/background.js:138: chrome.alarms.create({ when: Date.now() + delaySeconds * 1000 }); On ...
2 years, 6 months ago (2013-01-25 22:05:33 UTC) #22
vadimt
https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/29001/chrome/browser/resources/google_now/background.js#newcode138 chrome/browser/resources/google_now/background.js:138: chrome.alarms.create({ when: Date.now() + delaySeconds * 1000 }); On ...
2 years, 6 months ago (2013-01-25 23:01:19 UTC) #23
arv (Not doing code reviews)
LGTM https://codereview.chromium.org/11967029/diff/42002/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/42002/chrome/browser/resources/google_now/background.js#newcode149 chrome/browser/resources/google_now/background.js:149: chrome.alarms.create({ delayInMinutes: delaySeconds / 60 }); no space ...
2 years, 6 months ago (2013-01-29 19:28:46 UTC) #24
vadimt
Thanks reviewers! https://codereview.chromium.org/11967029/diff/42002/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/11967029/diff/42002/chrome/browser/resources/google_now/background.js#newcode149 chrome/browser/resources/google_now/background.js:149: chrome.alarms.create({ delayInMinutes: delaySeconds / 60 }); On ...
2 years, 6 months ago (2013-01-29 20:16:38 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11967029/44001
2 years, 6 months ago (2013-01-29 20:20:58 UTC) #26
commit-bot: I haz the power
Presubmit check for 11967029-44001 failed and returned exit status 1. Running presubmit commit checks ...
2 years, 6 months ago (2013-01-29 20:21:02 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11967029/36002
2 years, 6 months ago (2013-01-29 21:24:43 UTC) #28
commit-bot: I haz the power
2 years, 6 months ago (2013-01-30 01:32:17 UTC) #29
Message was sent while issue was closed.
Change committed as 179497
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 5fa3ca5