Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(596)

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
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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+205 lines, -0 lines) Patch
M build/common.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/browser_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/extensions/component_loader.cc View 1 chunk +6 lines, -0 lines 0 comments Download
M chrome/browser/resources/component_extension_resources.grd View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
A chrome/browser/resources/google_now/background.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +170 lines, -0 lines 0 comments Download
A chrome/browser/resources/google_now/manifest.json View 1 2 3 4 5 6 7 8 9 1 chunk +20 lines, -0 lines 0 comments Download

Messages

Total messages: 29 (0 generated)
vadimt
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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. ...
7 years, 11 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: > ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 months ago (2013-01-23 23:18:56 UTC) #15
skare_
lgtm
7 years, 11 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. ...
7 years, 11 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
7 years, 11 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 ...
7 years, 11 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; ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 11 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 ...
7 years, 10 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 ...
7 years, 10 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
7 years, 10 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 ...
7 years, 10 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
7 years, 10 months ago (2013-01-29 21:24:43 UTC) #28
commit-bot: I haz the power
7 years, 10 months ago (2013-01-30 01:32:17 UTC) #29
Message was sent while issue was closed.
Change committed as 179497

Powered by Google App Engine
This is Rietveld 408576698