Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
546633004:
Refactoring the code for location manager. (Closed)
|
Created:
6 years, 3 months ago by divya.bansal Modified:
6 years, 3 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRefactoring the code for location manager.
Code refactoring for location provider while registering for location updates.
While registering the location provider for location updates location updates are requested
redundantly for gps enabled or disabled code.
BUG=411182
Committed: https://crrev.com/a19de98a24d8d6eef2caa3862e12a7c2c8136570
Cr-Commit-Position: refs/heads/master@{#293766}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Changes according to review #
Total comments: 1
Patch Set 3 : Changes according to review #Messages
Total messages: 28 (11 generated)
divya.bansal@samsung.com changed reviewers: + jdduke@google.com, l.gombos@samsung.com
On 2014/09/05 12:41:43, divya.bansal wrote: > mailto:divya.bansal@samsung.com changed reviewers: > + mailto:jdduke@google.com, mailto:l.gombos@samsung.com PTAL
divya.bansal@samsung.com changed reviewers: + tedchoc@chromium.org
jdduke@chromium.org changed reviewers: + benm@chromium.org, jdduke@chromium.org
Seems reasonable. Adding benm@ (the original author? or at least the most recent modifier). Do we have any tests for this functionality? https://chromiumcodereview.appspot.com/546633004/diff/1/content/public/androi... File content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java (right): https://chromiumcodereview.appspot.com/546633004/diff/1/content/public/androi... content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:153: if (isGpsEnabled) { Nit: This branch can fit on a single line without braces.
lgtm
The CQ bit was checked by divya.bansal@samsung.com
The CQ bit was unchecked by divya.bansal@samsung.com
The CQ bit was checked by divya.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/divya.bansal@samsung.com/546633004/1
On 2014/09/06 05:50:03, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/divya.bansal@samsung.com/546633004/1 @jdduke its already in commit queue. Should I try to stop and put that nit change that you suggested.
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by divya.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/divya.bansal@samsung.com/546633004/1
The CQ bit was unchecked by jdduke@chromium.org
On 2014/09/06 05:56:20, divya.bansal wrote: > On 2014/09/06 05:50:03, I haz the power (commit-bot) wrote: > > CQ is trying da patch. Follow status at > > https://chromium-status.appspot.com/cq/divya.bansal@samsung.com/546633004/1 > > @jdduke its already in commit queue. Should I try to stop and put that nit > change that you suggested. Yes, you should, that's why we do code reviews.
On 2014/09/06 12:44:39, jdduke wrote: > On 2014/09/06 05:56:20, divya.bansal wrote: > > On 2014/09/06 05:50:03, I haz the power (commit-bot) wrote: > > > CQ is trying da patch. Follow status at > > > https://chromium-status.appspot.com/cq/divya.bansal@samsung.com/546633004/1 > > > > @jdduke its already in commit queue. Should I try to stop and put that nit > > change that you suggested. > > Yes, you should, that's why we do code reviews. @jdduke thanks for the review i will make the changes.
On 2014/09/06 16:28:39, divya.bansal wrote: > On 2014/09/06 12:44:39, jdduke wrote: > > On 2014/09/06 05:56:20, divya.bansal wrote: > > > On 2014/09/06 05:50:03, I haz the power (commit-bot) wrote: > > > > CQ is trying da patch. Follow status at > > > > > https://chromium-status.appspot.com/cq/divya.bansal@samsung.com/546633004/1 > > > > > > @jdduke its already in commit queue. Should I try to stop and put that nit > > > change that you suggested. > > > > Yes, you should, that's why we do code reviews. > > @jdduke thanks for the review i will make the changes. Please review the changes.
jdduke@chromium.org changed reviewers: - jdduke@google.com
https://codereview.chromium.org/546633004/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java (right): https://codereview.chromium.org/546633004/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:154: criteria.setAccuracy(Criteria.ACCURACY_FINE); The if body should be on the same line as the if condition (assuming the width is less than 100 characters).
On 2014/09/07 13:19:26, jdduke wrote: > https://codereview.chromium.org/546633004/diff/20001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java > (right): > > https://codereview.chromium.org/546633004/diff/20001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/LocationProviderFactory.java:154: > criteria.setAccuracy(Criteria.ACCURACY_FINE); > The if body should be on the same line as the if condition (assuming the width > is less than 100 characters). PTAL
On 2014/09/08 03:23:37, divya.bansal wrote: > PTAL lgtm
The CQ bit was checked by divya.bansal@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/divya.bansal@samsung.com/546633004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as f1821ba48feddbb1a2ed7d6f43469263c91f9cb6
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a19de98a24d8d6eef2caa3862e12a7c2c8136570 Cr-Commit-Position: refs/heads/master@{#293766} |
