Creating a skeleton for Google Now for Chrome implementation.

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

[skare_, 2012-12-01 03:00:14]

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
File chrome/browser/ui/google_now/google_now_service.cc (right):

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:15: const int
kDefaultPollingPeriodMs = 300000;
(optional) add comment
... = 300000;  // 5 minutes

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:29: // If Google Now
integration is enabled for the profile, start the first cards
nit: code sort of explains itself here but feel free to keep the comment.

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:104: // default delay.
You may want exponential backoff with randomized jitter in the error case (or a
TODO for the functionality), so that if the server gets DDOSed you don't have
that same collection of clients hitting the server simultaneously one interval
later.

Chrome may have reusable code to facilitate this (not sure; Toolbar does, for
example).

This is assuming ServerResponse won't handle all network-related issues itself.

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:112:
server_response->next_request_delay_ms = 10000;// TODO 300000;
I know this will come from the response, but maybe use kDefaultPollingPeriodMs
as the constant (and remove the TODO) for now?

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:118: // TODO(vadimt):
Implement via using Chrome Notifications.
nit: just one (either) of "via using"

[vadimt, 2012-12-03 18:48:07]

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
File chrome/browser/ui/google_now/google_now_service.cc (right):

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:15: const int
kDefaultPollingPeriodMs = 300000;
On 2012/12/01 03:00:14, Travis Skare wrote:
> (optional) add comment
> ... = 300000;  // 5 minutes
> 

Done.

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:29: // If Google Now
integration is enabled for the profile, start the first cards
On 2012/12/01 03:00:14, Travis Skare wrote:
> nit: code sort of explains itself here but feel free to keep the comment.
Thanks! I'll keep the comment, more info doesn't hurt :)

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:104: // default delay.
On 2012/12/01 03:00:14, Travis Skare wrote:
> You may want exponential backoff with randomized jitter in the error case (or
a
> TODO for the functionality), so that if the server gets DDOSed you don't have
> that same collection of clients hitting the server simultaneously one interval
> later.
> 
> Chrome may have reusable code to facilitate this (not sure; Toolbar does, for
> example).
> 
> This is assuming ServerResponse won't handle all network-related issues
itself.

Done.

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:112:
server_response->next_request_delay_ms = 10000;// TODO 300000;
On 2012/12/01 03:00:14, Travis Skare wrote:
> I know this will come from the response, but maybe use kDefaultPollingPeriodMs
> as the constant (and remove the TODO) for now?

Done.

https://codereview.chromium.org/11412291/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:118: // TODO(vadimt):
Implement via using Chrome Notifications.
On 2012/12/01 03:00:14, Travis Skare wrote:
> nit: just one (either) of "via using"

Done.

[rgustafson, 2012-12-03 19:28:59]

https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_n...
File chrome/browser/ui/google_now/google_now_service.cc (right):

https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:80:
StartWaitingForNextUpdate(kDefaultPollingPeriodMs);
Hopefully, we can do better than showing nothing even with a bad location. There
is support for this on the other end to send some cards without a location.

https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:84: void
GoogleNowService::StartServerRequest(
If we support non-location specific cards when we have a bad location, this
needs to also support having no position.

https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1410: // Create the instance of the Google
Now service.
Just to clarify, this class (chrome_browser_main) does get created just once per
profile regardless of windows, etc etc? This isn't immediately obvious. Can you
point me to where you figured this out?

[rgustafson, 2012-12-03 19:38:03]

https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1410: // Create the instance of the Google
Now service.
Commenting on my own comment... The singleton properties are brought by the
factory class, but I'm still wondering when this guy is created.

On 2012/12/03 19:28:59, rgustafson wrote:
> Just to clarify, this class (chrome_browser_main) does get created just once
per
> profile regardless of windows, etc etc? This isn't immediately obvious. Can
you
> point me to where you figured this out?

[vadimt, 2012-12-03 20:48:56]

https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_n...
File chrome/browser/ui/google_now/google_now_service.cc (right):

https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:80:
StartWaitingForNextUpdate(kDefaultPollingPeriodMs);
On 2012/12/03 19:28:59, rgustafson wrote:
> Hopefully, we can do better than showing nothing even with a bad location.
There
> is support for this on the other end to send some cards without a location.

That's a cool observation! Done.

https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:84: void
GoogleNowService::StartServerRequest(
On 2012/12/03 19:28:59, rgustafson wrote:
> If we support non-location specific cards when we have a bad location, this
> needs to also support having no position.

Correct; but I assume, we still need to try getting a location?

https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1410: // Create the instance of the Google
Now service.
On 2012/12/03 19:28:59, rgustafson wrote:
> Just to clarify, this class (chrome_browser_main) does get created just once
per
> profile regardless of windows, etc etc? This isn't immediately obvious. Can
you
> point me to where you figured this out?

Done.

https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1410: // Create the instance of the Google
Now service.
On 2012/12/03 19:38:03, rgustafson wrote:
> Commenting on my own comment... The singleton properties are brought by the
> factory class, but I'm still wondering when this guy is created.
> 
> On 2012/12/03 19:28:59, rgustafson wrote:
> > Just to clarify, this class (chrome_browser_main) does get created just once
> per
> > profile regardless of windows, etc etc? This isn't immediately obvious. Can
> you
> > point me to where you figured this out?
> 

That's an incredibly good question! You've actually found a bug!
This place is called only once, at the browser's startup. This is how the
factory and the first instance of the service is created.

Now, during (too aggressive) cleanup I've removed
GoogleNowServiceFactory::ServiceIsCreatedWithProfile method. Thanks to it,
additional instances of the service will be automatically created when the user
opens other profiles. I've re-added the method; thanks!

[rgustafson, 2012-12-04 00:38:01]

lgtm

https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_n...
File chrome/browser/ui/google_now/google_now_service.cc (right):

https://codereview.chromium.org/11412291/diff/7001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:84: void
GoogleNowService::StartServerRequest(
Yes, but its nice to get as much as you can with the failure.
On 2012/12/03 20:48:56, vadimt wrote:
> On 2012/12/03 19:28:59, rgustafson wrote:
> > If we support non-location specific cards when we have a bad location, this
> > needs to also support having no position.
> 
> Correct; but I assume, we still need to try getting a location?

https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11412291/diff/5002/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1410: // Create the instance of the Google
Now service.
Okay good, just wanted to make sure the multi-profile case was covered. LGTM.
On 2012/12/03 20:48:57, vadimt wrote:
> On 2012/12/03 19:38:03, rgustafson wrote:
> > Commenting on my own comment... The singleton properties are brought by the
> > factory class, but I'm still wondering when this guy is created.
> > 
> > On 2012/12/03 19:28:59, rgustafson wrote:
> > > Just to clarify, this class (chrome_browser_main) does get created just
once
> > per
> > > profile regardless of windows, etc etc? This isn't immediately obvious.
Can
> > you
> > > point me to where you figured this out?
> > 
> 
> That's an incredibly good question! You've actually found a bug!
> This place is called only once, at the browser's startup. This is how the
> factory and the first instance of the service is created.
> 
> Now, during (too aggressive) cleanup I've removed
> GoogleNowServiceFactory::ServiceIsCreatedWithProfile method. Thanks to it,
> additional instances of the service will be automatically created when the
user
> opens other profiles. I've re-added the method; thanks!

[skare_, 2012-12-04 21:19:24]

lgtm

[sky, 2012-12-06 16:40:54]

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1411: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
Would it be better to have a feature level ifdef, eg ENABLE_GOOGLE_NOW (similar
to ENABLE_LANGUAGE_DETECTION above).

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1412: if
(CommandLine::ForCurrentProcess()->HasSwitch(
Do we really need to do this right at startup? Or said differently, won't doing
this now impact startup time?

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
File chrome/browser/ui/google_now/google_now_service.cc (right):

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:6: #include
"content/public/common/geoposition.h"
nit: newline between 5 and 6.

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:25: };
next_request_delay_ms -> TimeDelta.

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:42: void
GoogleNowService::StartWaitingForNextUpdate(int64 delay_ms) {
Order of methods in .cc should match that of .h

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:107: bool
GoogleNowService::ParseServerResponse(const URLRequest* request,
If you can't fit everything on one line, then one param per line and either
align to ( on previous line, or start request on next line and indent 4.

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
File chrome/browser/ui/google_now/google_now_service.h (right):

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.h:57: static bool
ParseServerResponse(const net::URLRequest* request, int num_bytes,
nit: each param on its own line.

[vadimt, 2012-12-06 20:43:15]

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1411: #if !defined(OS_ANDROID) &&
!defined(OS_IOS)
On 2012/12/06 16:40:54, sky wrote:
> Would it be better to have a feature level ifdef, eg ENABLE_GOOGLE_NOW
(similar
> to ENABLE_LANGUAGE_DETECTION above).

Done.

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1412: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/12/06 16:40:54, sky wrote:
> Do we really need to do this right at startup? Or said differently, won't
doing
> this now impact startup time?

Well, it will instantiate several objects and post a task to start a geolocation
request. This is not too much, but probably, it's better to move this from the
startup. What place would you suggest?

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
File chrome/browser/ui/google_now/google_now_service.cc (right):

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:6: #include
"content/public/common/geoposition.h"
On 2012/12/06 16:40:54, sky wrote:
> nit: newline between 5 and 6.

Done.

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:25: };
On 2012/12/06 16:40:54, sky wrote:
> next_request_delay_ms -> TimeDelta.

Done.

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:42: void
GoogleNowService::StartWaitingForNextUpdate(int64 delay_ms) {
On 2012/12/06 16:40:54, sky wrote:
> Order of methods in .cc should match that of .h

Done.

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:107: bool
GoogleNowService::ParseServerResponse(const URLRequest* request,
On 2012/12/06 16:40:54, sky wrote:
> If you can't fit everything on one line, then one param per line and either
> align to ( on previous line, or start request on next line and indent 4.

Done.

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
File chrome/browser/ui/google_now/google_now_service.h (right):

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.h:57: static bool
ParseServerResponse(const net::URLRequest* request, int num_bytes,
On 2012/12/06 16:40:54, sky wrote:
> nit: each param on its own line.

Done.

[sky, 2012-12-06 21:07:06]

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1412: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/12/06 20:43:16, vadimt wrote:
> On 2012/12/06 16:40:54, sky wrote:
> > Do we really need to do this right at startup? Or said differently, won't
> doing
> > this now impact startup time?
> 
> Well, it will instantiate several objects and post a task to start a
geolocation
> request. This is not too much, but probably, it's better to move this from the
> startup. What place would you suggest?

I don't know enough about this feature to suggest a good place. I would at least
wait until we've created all the browsers we need to.

[vadimt, 2012-12-07 02:41:23]

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11412291/diff/8012/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1412: if
(CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/12/06 21:07:07, sky wrote:
> On 2012/12/06 20:43:16, vadimt wrote:
> > On 2012/12/06 16:40:54, sky wrote:
> > > Do we really need to do this right at startup? Or said differently, won't
> > doing
> > > this now impact startup time?
> > 
> > Well, it will instantiate several objects and post a task to start a
> geolocation
> > request. This is not too much, but probably, it's better to move this from
the
> > startup. What place would you suggest?
> 
> I don't know enough about this feature to suggest a good place. I would at
least
> wait until we've created all the browsers we need to.

Moved past starting browser.

[sky, 2012-12-07 16:39:05]

LGTM with a slight gyp change.

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', {
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.

[commit-bot: I haz the power, 2012-12-07 18:35:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vadimt@chromium.org/11412291/27001

[commit-bot: I haz the power, 2012-12-07 22:44:45]

Change committed as 171868

[vadimt, 2012-12-07 23:26:01]

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.