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

Issue 11412291: Creating a skeleton for Google Now for Chrome implementation. (Closed)

Created:
8 years ago by vadimt
Modified:
8 years ago
Reviewers:
sky, rgustafson, skare_, stromme
CC:
chromium-reviews
Visibility:
Public.

Description

Creating a skeleton for Google Now for Chrome implementation. The CL creates the top-level structure for showing Google Now cards in Chrome via Chrome Notifications. The implementation lives behind -enable-google-now-integration flag. BUG=164227 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171868

Patch Set 1 #

Patch Set 2 : Cosmetics #

Total comments: 10

Patch Set 3 : #

Total comments: 5

Patch Set 4 : Travis' CR #

Total comments: 5

Patch Set 5 : rgustafson's comments #

Patch Set 6 : Fixing multi-platform compile errors. #

Patch Set 7 : Fixing a typo. #

Total comments: 16

Patch Set 8 : sky's comments #

Patch Set 9 : More sky's notes. #

Total comments: 2

Patch Set 10 : Final sky's comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+302 lines, -0 lines) Patch
M build/common.gypi View 1 2 3 4 5 6 7 5 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/chrome_browser_main.cc View 1 2 3 4 5 6 7 8 2 chunks +12 lines, -0 lines 0 comments Download
A chrome/browser/ui/google_now/google_now_service.h View 1 2 3 4 5 6 7 1 chunk +76 lines, -0 lines 0 comments Download
A chrome/browser/ui/google_now/google_now_service.cc View 1 2 3 4 5 6 7 1 chunk +121 lines, -0 lines 0 comments Download
A chrome/browser/ui/google_now/google_now_service_factory.h View 1 2 3 4 5 1 chunk +32 lines, -0 lines 0 comments Download
A chrome/browser/ui/google_now/google_now_service_factory.cc View 1 2 3 4 5 1 chunk +36 lines, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 2 chunks +12 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 1 chunk +3 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
vadimt
8 years ago (2012-12-01 02:36:02 UTC) #1
skare_
https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_now/google_now_service.cc File chrome/browser/ui/google_now/google_now_service.cc (right): https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_now/google_now_service.cc#newcode15 chrome/browser/ui/google_now/google_now_service.cc:15: const int kDefaultPollingPeriodMs = 300000; (optional) add comment ...
8 years ago (2012-12-01 03:00:14 UTC) #2
vadimt
https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_now/google_now_service.cc File chrome/browser/ui/google_now/google_now_service.cc (right): https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_now/google_now_service.cc#newcode15 chrome/browser/ui/google_now/google_now_service.cc:15: const int kDefaultPollingPeriodMs = 300000; On 2012/12/01 03:00:14, Travis ...
8 years ago (2012-12-03 18:48:07 UTC) #3
rgustafson
https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_now/google_now_service.cc File chrome/browser/ui/google_now/google_now_service.cc (right): https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_now/google_now_service.cc#newcode80 chrome/browser/ui/google_now/google_now_service.cc:80: StartWaitingForNextUpdate(kDefaultPollingPeriodMs); Hopefully, we can do better than showing nothing ...
8 years ago (2012-12-03 19:28:59 UTC) #4
rgustafson
https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_browser_main.cc#newcode1410 chrome/browser/chrome_browser_main.cc:1410: // Create the instance of the Google Now service. ...
8 years ago (2012-12-03 19:38:03 UTC) #5
vadimt
https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_now/google_now_service.cc File chrome/browser/ui/google_now/google_now_service.cc (right): https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_now/google_now_service.cc#newcode80 chrome/browser/ui/google_now/google_now_service.cc:80: StartWaitingForNextUpdate(kDefaultPollingPeriodMs); On 2012/12/03 19:28:59, rgustafson wrote: > Hopefully, we ...
8 years ago (2012-12-03 20:48:56 UTC) #6
rgustafson
lgtm https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_now/google_now_service.cc File chrome/browser/ui/google_now/google_now_service.cc (right): https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_now/google_now_service.cc#newcode84 chrome/browser/ui/google_now/google_now_service.cc:84: void GoogleNowService::StartServerRequest( Yes, but its nice to get ...
8 years ago (2012-12-04 00:38:01 UTC) #7
skare_
lgtm
8 years ago (2012-12-04 21:19:24 UTC) #8
vadimt
8 years ago (2012-12-04 21:34:20 UTC) #9
sky
https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_browser_main.cc#newcode1411 chrome/browser/chrome_browser_main.cc:1411: #if !defined(OS_ANDROID) && !defined(OS_IOS) Would it be better to ...
8 years ago (2012-12-06 16:40:54 UTC) #10
vadimt
https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_browser_main.cc#newcode1411 chrome/browser/chrome_browser_main.cc:1411: #if !defined(OS_ANDROID) && !defined(OS_IOS) On 2012/12/06 16:40:54, sky wrote: ...
8 years ago (2012-12-06 20:43:15 UTC) #11
sky
https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_browser_main.cc#newcode1412 chrome/browser/chrome_browser_main.cc:1412: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2012/12/06 20:43:16, vadimt wrote: > On ...
8 years ago (2012-12-06 21:07:06 UTC) #12
vadimt
https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_browser_main.cc#newcode1412 chrome/browser/chrome_browser_main.cc:1412: if (CommandLine::ForCurrentProcess()->HasSwitch( On 2012/12/06 21:07:07, sky wrote: > On ...
8 years ago (2012-12-07 02:41:23 UTC) #13
sky
LGTM with a slight gyp change. https://codereview.chromium.org/11412291/diff/17003/chrome/chrome_browser_ui.gypi File chrome/chrome_browser_ui.gypi (right): https://codereview.chromium.org/11412291/diff/17003/chrome/chrome_browser_ui.gypi#newcode2178 chrome/chrome_browser_ui.gypi:2178: ['enable_google_now==1', { See ...
8 years ago (2012-12-07 16:39:05 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11412291/27001
8 years ago (2012-12-07 18:35:20 UTC) #15
commit-bot: I haz the power
Change committed as 171868
8 years ago (2012-12-07 22:44:45 UTC) #16
vadimt
8 years ago (2012-12-07 23:26:01 UTC) #17
Message was sent while issue was closed.
https://codereview.chromium.org/11412291/diff/17003/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/11412291/diff/17003/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:2178: ['enable_google_now==1', {
On 2012/12/07 16:39:06, sky wrote:
> See comment on line 67. In general we list all files above and exclude where
not
> needed. So, put the google_now files above and change this to be
> enable_google_now==0 and exclude the files.

Done.

Powered by Google App Engine
This is Rietveld 408576698