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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 3 months ago by vadimt
Modified:
1 year, 2 months ago
CC:
chromium-reviews_chromium.org, Aaron Boodman, arv, 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) Lint 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 ? errors 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 ? errors Download
M chrome/browser/extensions/component_loader.cc View 1 chunk +6 lines, -0 lines 0 comments ? errors 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 ? errors 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 ? errors 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 ? errors Download
Commit:

Messages

Total messages: 29
vadimt
1 year, 3 months ago #1
Travis 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 ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #3
rgustafson
First comments, not yet done. Sending these and I will continue on the new revision. ...
1 year, 3 months ago #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: > ...
1 year, 3 months ago #5
Travis 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 ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #7
skare
the last response was by email, right? (can't see the comment) maybe just add a ...
1 year, 3 months ago #8
vadimt
On 2013/01/18 23:57:53, skare wrote: > the last response was by email, right? (can't see ...
1 year, 3 months ago #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 ...
1 year, 3 months ago #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 ...
1 year, 2 months ago #11
Travis 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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #15
Travis Skare
lgtm
1 year, 2 months ago #16
vadimt
mpcomplete, sky, please provide owner's approvals. mpcomplete, please review component_loader.cc sky, please review the rest. ...
1 year, 2 months ago #17
sky
I'm not a good reviewer for the JS changes. Maybe arv? Everything else LGTM
1 year, 2 months ago #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 ...
1 year, 2 months ago #19
arv
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; ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #23
arv
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 ...
1 year, 2 months ago #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 ...
1 year, 2 months ago #25
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11967029/44001
1 year, 2 months ago #26
I haz the power (commit-bot)
Presubmit check for 11967029-44001 failed and returned exit status 1. Running presubmit commit checks ...
1 year, 2 months ago #27
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11967029/36002
1 year, 2 months ago #28
I haz the power (commit-bot)
1 year, 2 months ago #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 1280:2d3e6564b7b6