Fetching Google Now Notifications data from the server

Fetching Google Now Notifications data from the server.

BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173005

[rgustafson, 2012-12-12 22:26:52]

lgtm

Technically, things look fine to me, but I just want to point out that the way
we contact the server may change. It isn't guaranteed to be a URL fetch. Don't
want to let you get too comfortable in believing things will stay the same. :)

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

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:150: // sure we never get
responses like 'Select an account' page.
The final server won't have a login page like it does currently. It can send a
friendlier response. Just an FYI for later work.

[skare_, 2012-12-12 22:44:35]

lgtm

likewise, technically looks ok. Here are a couple minor readability things and a
couple FYIs.

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

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:126:
switches::kEnableGoogleNowIntegration);
is 
somethingThatSuggestsBoolean = http://...
confusing? Even though this is an experiment.

For instance, all the switch::kEnableX's in about_flags (I know this one isn't)
are single value type.

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:132: parameters << "?q="
fyi: google c++ style is to avoid streams, but plenty of places in /browser use
std::stringstream and I'm not an expert here, so ok.

ref: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Streams

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:178: DCHECK(fetcher_.get() !=
NULL);
nit: human-readability-wise would this make more sense by the fetcher_.get()
check? And perhaps IsGoogleNowEnabled() first?

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:180: // TODO(vadimt):
Consider exponential backoff with randomized jitter.
fyi: having a TODO is ok but I think backoff is more of a 'really should do this
on requests that automatically retry' more than 'consider' :)

Mostly for HTTP 5xx.

[vadimt, 2012-12-12 23:02:31]

sky@, please provide owner's approval. Thanks!

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

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:126:
switches::kEnableGoogleNowIntegration);
On 2012/12/12 22:44:35, Travis Skare wrote:
> is 
> somethingThatSuggestsBoolean = http://...
> confusing? Even though this is an experiment.
> 
> For instance, all the switch::kEnableX's in about_flags (I know this one
isn't)
> are single value type.

As discussed offline, leaving as is. Thanks Travis!

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:132: parameters << "?q="
On 2012/12/12 22:44:35, Travis Skare wrote:
> fyi: google c++ style is to avoid streams, but plenty of places in /browser
use
> std::stringstream and I'm not an expert here, so ok.
> 
> ref: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Streams

Thanks!

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:150: // sure we never get
responses like 'Select an account' page.
On 2012/12/12 22:26:52, rgustafson wrote:
> The final server won't have a login page like it does currently. It can send a
> friendlier response. Just an FYI for later work.

Nice to know; thanks!

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:178: DCHECK(fetcher_.get() !=
NULL);
On 2012/12/12 22:44:35, Travis Skare wrote:
> nit: human-readability-wise would this make more sense by the fetcher_.get()
> check? And perhaps IsGoogleNowEnabled() first?

Done.

https://codereview.chromium.org/11471046/diff/2001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:180: // TODO(vadimt):
Consider exponential backoff with randomized jitter.
On 2012/12/12 22:44:35, Travis Skare wrote:
> fyi: having a TODO is ok but I think backoff is more of a 'really should do
this
> on requests that automatically retry' more than 'consider' :)
> 
> Mostly for HTTP 5xx.

Done.

[sky, 2012-12-13 00:38:46]

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

https://codereview.chromium.org/11471046/diff/8001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:127:
DCHECK(server_path.length() > 0);
!empty

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

https://codereview.chromium.org/11471046/diff/8001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.h:30: private
net::URLFetcherDelegate {
style guide says no private inheritance.

https://codereview.chromium.org/11471046/diff/8001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.h:57: // Starts downloading
cards from the server.
nit: newline between 56/57.

[vadimt, 2012-12-13 00:55:10]

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

https://codereview.chromium.org/11471046/diff/8001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.cc:127:
DCHECK(server_path.length() > 0);
On 2012/12/13 00:38:46, sky wrote:
> !empty

Done.

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

https://codereview.chromium.org/11471046/diff/8001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.h:30: private
net::URLFetcherDelegate {
On 2012/12/13 00:38:46, sky wrote:
> style guide says no private inheritance.

Done.

https://codereview.chromium.org/11471046/diff/8001/chrome/browser/ui/google_n...
chrome/browser/ui/google_now/google_now_service.h:57: // Starts downloading
cards from the server.
On 2012/12/13 00:38:46, sky wrote:
> nit: newline between 56/57.

Done.

[sky, 2012-12-13 17:23:23]

LGTM

[commit-bot: I haz the power, 2012-12-13 18:18:22]

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

[commit-bot: I haz the power, 2012-12-13 18:45:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-13 19:17:36]

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

[commit-bot: I haz the power, 2012-12-13 23:46:30]

Change committed as 173005