Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11434116:
Geolocation request for Google Now (Closed)
|
Created:
8 years ago by vadimt Modified:
8 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionObtaining geolocation for Google Now notifications.
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=172192
Patch Set 1 #Patch Set 2 : Cosmetics. #
Total comments: 4
Patch Set 3 : rgustafson's comments #
Total comments: 8
Patch Set 4 : skare's comments. #Patch Set 5 : Rebasing #
Total comments: 4
Patch Set 6 : bulach and jknotten comments. #
Messages
Total messages: 14 (0 generated)
https://codereview.chromium.org/11434116/diff/2001/chrome/browser/ui/google_n... File chrome/browser/ui/google_now/google_now_service.cc (right): https://codereview.chromium.org/11434116/diff/2001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.cc:6: #include "content/public/browser/browser_thread.h" Is this used? https://codereview.chromium.org/11434116/diff/2001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.cc:94: DCHECK(geolocation_request_weak_factory_.HasWeakPtrs()); If the location is obtained, where are the ptrs invalidated?
https://codereview.chromium.org/11434116/diff/2001/chrome/browser/ui/google_n... File chrome/browser/ui/google_now/google_now_service.cc (right): https://codereview.chromium.org/11434116/diff/2001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.cc:6: #include "content/public/browser/browser_thread.h" On 2012/12/06 20:10:34, rgustafson wrote: > Is this used? Done. https://codereview.chromium.org/11434116/diff/2001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.cc:94: DCHECK(geolocation_request_weak_factory_.HasWeakPtrs()); On 2012/12/06 20:10:34, rgustafson wrote: > If the location is obtained, where are the ptrs invalidated? Another great catch! It's done by the framework after OnLocationObtained, but asynchronously, so in theory, this may not happen before OnServerRequestCompleted. I've made invalidation explicit.
lgtm
lgtm https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... File chrome/browser/ui/google_now/google_now_service.cc (right): https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.cc:24: const int kGeolocationRequestTimeoutMs = 60000; // 1 min nit here and above: 2 spaces between code and comments, 1 min->1 minute just for consistency https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.cc:112: // TODO(vadimt): Implement via making URLRequest to the server. should this be implemented in the same CL? or are reviewers ok with this granularity? https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... File chrome/browser/ui/google_now/google_now_service.h (right): https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.h:53: // Starts downloading cards from the server. If the position's Validate() call will callers need to care about existence or return value of position::Validate()? If not, maybe just something like "If geolocation is not able to be determined, does X." https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.h:72: base::WeakPtrFactory<GoogleNowService> geolocation_request_weak_factory_; tiny, tiny nit: some similar code uses comments like // Used to ensure geolocation request callback is not run after this object is destroyed.
https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... File chrome/browser/ui/google_now/google_now_service.cc (right): https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.cc:24: const int kGeolocationRequestTimeoutMs = 60000; // 1 min On 2012/12/07 23:39:00, Travis Skare wrote: > nit here and above: 2 spaces between code and comments, 1 min->1 minute just for > consistency Done. https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.cc:112: // TODO(vadimt): Implement via making URLRequest to the server. On 2012/12/07 23:39:00, Travis Skare wrote: > should this be implemented in the same CL? or are reviewers ok with this > granularity? It's recommended to make reviews small in Chrome. So, I'd say, we can implement geolocation request separately from the server request. https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... File chrome/browser/ui/google_now/google_now_service.h (right): https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.h:53: // Starts downloading cards from the server. If the position's Validate() call On 2012/12/07 23:39:00, Travis Skare wrote: > will callers need to care about existence or return value of > position::Validate()? > If not, maybe just something like > "If geolocation is not able to be determined, does X." This tells to the callers how exactly to specify that they couldn't determine a position. So, this is a part of the method's contract. I'd keep this comment, if you don't mind. https://codereview.chromium.org/11434116/diff/7001/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.h:72: base::WeakPtrFactory<GoogleNowService> geolocation_request_weak_factory_; On 2012/12/07 23:39:00, Travis Skare wrote: > tiny, tiny nit: some similar code uses comments like > > // Used to ensure geolocation request callback is not run after this object is > destroyed. Done.
sky@, please provide Owner's Approval. bulach@, you may want to look whether I correctly use RequestLocationUpdate; however your response is not required.
+jknotten lgtm, and apologies for the delay, I was travelling the last couple of days. one nit below. also, jknotten points out that RequestLocationUpdate is good for "one shot". I don't know how this is integrated further up on your stack, but you may be interested in keeping an Observer for GeolocationProvider, if there's any value in watching location changes.. thanks! https://codereview.chromium.org/11434116/diff/9002/chrome/browser/ui/google_n... File chrome/browser/ui/google_now/google_now_service.h (right): https://codereview.chromium.org/11434116/diff/9002/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.h:57: // Starts downloading cards from the server. If the position's Validate() call nit: not sure if the last bit add much in here :) I mean, for any content::Geoposition object, Validate() being false means it's not available...
lgtm https://codereview.chromium.org/11434116/diff/9002/chrome/browser/ui/google_n... File chrome/browser/ui/google_now/google_now_service.h (right): https://codereview.chromium.org/11434116/diff/9002/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.h:78: base::WeakPtrFactory<GoogleNowService> geolocation_request_weak_factory_; nit: I read this as "a factory that hands out weak pointers to geolocation requests". It might be clearer to name it just weak_factory_.
LGTM
Thanks for all approvals! At this point, RequestLocationUpdate seems the right thing since geolocation requests will be issues relatively rarely. https://codereview.chromium.org/11434116/diff/9002/chrome/browser/ui/google_n... File chrome/browser/ui/google_now/google_now_service.h (right): https://codereview.chromium.org/11434116/diff/9002/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.h:57: // Starts downloading cards from the server. If the position's Validate() call On 2012/12/10 14:25:46, bulach wrote: > nit: not sure if the last bit add much in here :) I mean, for any > content::Geoposition object, Validate() being false means it's not available... Done. https://codereview.chromium.org/11434116/diff/9002/chrome/browser/ui/google_n... chrome/browser/ui/google_now/google_now_service.h:78: base::WeakPtrFactory<GoogleNowService> geolocation_request_weak_factory_; On 2012/12/10 14:30:58, John Knottenbelt wrote: > nit: I read this as "a factory that hands out weak pointers to geolocation > requests". It might be clearer to name it just weak_factory_. Probably, we'll have other weak pointers here, for other kinds of asynchronous requests. So, if you don't mind, I'll leave it as is. Thanks for the comment!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11434116/21001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/11434116/21001
Message was sent while issue was closed.
Change committed as 172192 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
