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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 4 months ago by vadimt
Modified:
1 year, 4 months ago
CC:
chromium-reviews_chromium.org
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) Lint Patch
M build/common.gypi View 1 2 3 4 5 6 7 5 chunks +9 lines, -0 lines 0 comments ? errors 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 1 errors 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 1 errors 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 2 errors 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 1 errors 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 1 errors 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 ? errors Download
M chrome/common/chrome_switches.h View 1 chunk +1 line, -0 lines 0 comments ? errors Download
M chrome/common/chrome_switches.cc View 1 1 chunk +3 lines, -0 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 17
vadimt
1 year, 4 months ago #1
Travis 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 ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #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. ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #7
Travis Skare
lgtm
1 year, 4 months ago #8
vadimt
1 year, 4 months ago #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 ...
1 year, 4 months ago #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: ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #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 ...
1 year, 4 months ago #14
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11412291/27001
1 year, 4 months ago #15
I haz the power (commit-bot)
Change committed as 171868
1 year, 4 months ago #16
vadimt
1 year, 4 months ago #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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1275:d14800f88434