Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2576633002:
Add histograms for location timings for Android omnibox queries. (Closed)
DescriptionAdd histograms for location timings for Android omnibox queries.
Add histograms for the duration between the obtaining of location and the use of
location for omnibox queries. One set of histograms records the time since the
location was first obtained. There is a histogram for each combination of
Location Source and whether a location was sent. A second set of histograms
records the time since the location was most recently obtained. There is a
histogram for each of the Location Sources.
BUG=668923
Committed: https://crrev.com/5b6572e3c0aba3d558e173440bd644b2e4b9bd04
Cr-Commit-Position: refs/heads/master@{#441323}
Patch Set 1 #Patch Set 2 : Fix string comparison. #
Total comments: 21
Patch Set 3 : Address comments. Use histogram_suffixes. #
Total comments: 11
Patch Set 4 : Address comments. Log seconds instead of ms. Use nested histogram_suffixes. #
Total comments: 10
Patch Set 5 : change clock #Patch Set 6 : Suppress nullcheck warning, #
Messages
Total messages: 47 (29 generated)
pdyson@chromium.org changed reviewers: + dominickn@chromium.org, isherman@chromium.org, kcarattini@chromium.org
The CQ bit was checked by pdyson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by pdyson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
This is now ready for each of you to take a look at this. Thanks.
https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:160: * Using 24 days as that is the highest number of days that does not overflow int. Maybe this histogram doesn't make sense to measure in millis, if you're worried about int overflow as a constraining factor? https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:580: default: Should there be something like a NOTREACHED() assertion here? (That's a C++-ism -- I'm not familiar enough with Java to know what the appropriate equivalent is.) https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:583: } nit: Please leave a blank line after this one. https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:589: duration = duration >= 0 ? duration : Long.MAX_VALUE; Why might the duration be less than 0? https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:604: return; Ditto https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:606: duration = duration >= 0 ? duration : Long.MAX_VALUE; Ditto https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19359: +<histogram name="Geolocation.Header.LocationAgeBatterySaving"> Please add a units attribute, here and for the other histograms as well. https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19359: +<histogram name="Geolocation.Header.LocationAgeBatterySaving"> Please use histogram_suffixes element(s) to reduce the amount of metadata repetition in this file. https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19359: +<histogram name="Geolocation.Header.LocationAgeBatterySaving"> nit: I'd recommend having a dot between "LocationAge" and "BatterySaving" https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19365: + Battery Saving. Please document what is recorded when the location has not yet been obtained, or that this case is not possible. https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19417: + Saving when the location was not attached to the request. Why might the location not be attached, if it was acquired?
Dominick, I have a question in one of the comments for you. https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:160: * Using 24 days as that is the highest number of days that does not overflow int. On 2016/12/15 at 01:45:12, Ilya Sherman wrote: > Maybe this histogram doesn't make sense to measure in millis, if you're worried about int overflow as a constraining factor? Do you know how to change this? I tried using seconds instead, but recordCustomTimesHistogram() then just converts the values into milliseconds and overflows the int. https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:580: default: On 2016/12/15 at 01:45:13, Ilya Sherman wrote: > Should there be something like a NOTREACHED() assertion here? (That's a C++-ism -- I'm not familiar enough with Java to know what the appropriate equivalent is.) I'm new to Java. Dominick, should I have something like: throw new IllegalStateException("should not execute"); https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:583: } On 2016/12/15 at 01:45:13, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:589: duration = duration >= 0 ? duration : Long.MAX_VALUE; On 2016/12/15 at 01:45:12, Ilya Sherman wrote: > Why might the duration be less than 0? I've removed this test. It should not be less than zero. This was useful when I was developing this code as it caught the case where I did arithmetic involving Long.MAX_VALUE, but I now explicitly test for that above, so I can remove this here. https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:606: duration = duration >= 0 ? duration : Long.MAX_VALUE; On 2016/12/15 at 01:45:13, Ilya Sherman wrote: > Ditto I'm removed this line. The method GeolocationTracker.getLocationAge(), which provides the location age, already has this test. https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19359: +<histogram name="Geolocation.Header.LocationAgeBatterySaving"> On 2016/12/15 at 01:45:13, Ilya Sherman wrote: > Please add a units attribute, here and for the other histograms as well. Done units. Added dots after "LocationAge" here and "TimeListening" below. Added histogram_suffixes for LocationAge and TimeListening. TimeListening is still repetitive. Can two sets of suffixes be applied to TimeListening? https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19365: + Battery Saving. On 2016/12/15 at 01:45:13, Ilya Sherman wrote: > Please document what is recorded when the location has not yet been obtained, or that this case is not possible. Done. https://codereview.chromium.org/2576633002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19417: + Saving when the location was not attached to the request. On 2016/12/15 at 01:45:13, Ilya Sherman wrote: > Why might the location not be attached, if it was acquired? I've added in the reasons.
The CQ bit was checked by pdyson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:160: * Using 24 days as that is the highest number of days that does not overflow int. On 2016/12/15 05:31:00, pdyson wrote: > On 2016/12/15 at 01:45:12, Ilya Sherman wrote: > > Maybe this histogram doesn't make sense to measure in millis, if you're > worried about int overflow as a constraining factor? > > Do you know how to change this? I tried using seconds instead, but > recordCustomTimesHistogram() then just converts the values > into milliseconds and overflows the int. You can use recordCustomCountHistogram(). It's pretty much the same as recordCustomTimesHistogram(), except that it won't try to convert the recorded values into milliseconds =) https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19359: +<histogram name="Geolocation.Header.LocationAge" units="ms"> nit: Please add the base="true" attribute, which just means that there's no histogram named "Geolocation.Header.LocationAge" -- it's just a shared prefix. (And, if you take my advice about histogram_suffixes below, please do the same for the intermediary suffixes. I forget where exactly the base attribute goes for those, but there should be some examples in this file.) https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19364: + location was most recently acquired. What's recorded here if no location has been acquired? Is it the same as for Geolocation.Header.TimeListening? https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19384: + location was first acquired. If no location has been aquired, the maximum nit: s/aquired/acquired https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:110450: +<histogram_suffixes name="LocationMode"> nit: I think you need to specify separator=".", as the default IIRC is an underscore. https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:110480: + attached if it is too old or if the permissions do not allow it."/> You can use two levels of histogram_suffixes for the Attached/NotAttached variants, i.e. <histogram_suffixes ...> <suffix name="Attached" ...> <suffix name="NotAttached" ...> <affected-histogram name="Geolocation.Header.TimeListening.BatterySaving"/> <affected-histogram name="Geolocation.Header.TimeListening.GpsOnly"/> <affected-histogram name="Geolocation.Header.TimeListening.HighAccuracy"/> </histogram_suffixes>
Will wait till you resolve isherman's comments before a full review. https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2576633002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:580: default: There's no single pattern. assert false is probably the most common (but assertions only work in pre L I believe). Exceptions are sometimes called, but they are very destructive so I disprefer them. The other thing you could do is Log.e that an error occurred.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks. I've addressed the comments. https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19359: +<histogram name="Geolocation.Header.LocationAge" units="ms"> On 2016/12/15 at 23:06:19, Ilya Sherman wrote: > nit: Please add the base="true" attribute, which just means that there's no histogram named "Geolocation.Header.LocationAge" -- it's just a shared prefix. (And, if you take my advice about histogram_suffixes below, please do the same for the intermediary suffixes. I forget where exactly the base attribute goes for those, but there should be some examples in this file.) Done. See below for issue with base="true" in histogram_suffixes. https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19364: + location was most recently acquired. On 2016/12/15 at 23:06:19, Ilya Sherman wrote: > What's recorded here if no location has been acquired? Is it the same as for Geolocation.Header.TimeListening? Done. https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19384: + location was first acquired. If no location has been aquired, the maximum On 2016/12/15 at 23:06:19, Ilya Sherman wrote: > nit: s/aquired/acquired Done. https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:110450: +<histogram_suffixes name="LocationMode"> On 2016/12/15 at 23:06:19, Ilya Sherman wrote: > nit: I think you need to specify separator=".", as the default IIRC is an underscore. Done. https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:110480: + attached if it is too old or if the permissions do not allow it."/> On 2016/12/15 at 23:06:19, Ilya Sherman wrote: > You can use two levels of histogram_suffixes for the Attached/NotAttached variants, i.e. > > <histogram_suffixes ...> > <suffix name="Attached" ...> > <suffix name="NotAttached" ...> > <affected-histogram name="Geolocation.Header.TimeListening.BatterySaving"/> > <affected-histogram name="Geolocation.Header.TimeListening.GpsOnly"/> > <affected-histogram name="Geolocation.Header.TimeListening.HighAccuracy"/> > </histogram_suffixes> Done. In histogram_suffixes the base="true" attribute goes on the <suffix>. However, for LocationMode those suffixes should be base="true" for one affected-histogram but not for the other, so I have left it out.
Metrics LGTM, thanks Paul. https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2576633002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:110480: + attached if it is too old or if the permissions do not allow it."/> On 2016/12/16 05:03:31, pdyson wrote: > On 2016/12/15 at 23:06:19, Ilya Sherman wrote: > > You can use two levels of histogram_suffixes for the Attached/NotAttached > variants, i.e. > > > > <histogram_suffixes ...> > > <suffix name="Attached" ...> > > <suffix name="NotAttached" ...> > > <affected-histogram name="Geolocation.Header.TimeListening.BatterySaving"/> > > <affected-histogram name="Geolocation.Header.TimeListening.GpsOnly"/> > > <affected-histogram name="Geolocation.Header.TimeListening.HighAccuracy"/> > > </histogram_suffixes> > > Done. > > In histogram_suffixes the base="true" attribute goes on the <suffix>. However, > for LocationMode those suffixes should be base="true" for one affected-histogram > but not for the other, so I have left it out. > Yep, that's fine -- the base="true" attribute just keeps the histograms from popping up in the list of histograms on the dashboard, so it's not a big deal if it's omitted in some cases.
Description was changed from ========== Add histograms for location timings for Android omnibox queries. Add histograms for the duration between the obtaining of location and the use of location for omnibox queries. One set of histograms records the time since the location was first obtained. There is a histogram for each combination of Location Source and whether a location was sent. A second set of histograms records the time since the location was most recently obtained. There is a histogram for each of the Location Sources. BUG=668923 ========== to ========== Add histograms for location timings for Android omnibox queries. Add histograms for the duration between the obtaining of location and the use of location for omnibox queries. One set of histograms records the time since the location was first obtained. There is a histogram for each combination of Location Source and whether a location was sent. A second set of histograms records the time since the location was most recently obtained. There is a histogram for each of the Location Sources. BUG=668923 ==========
Description was changed from ========== Add histograms for location timings for Android omnibox queries. Add histograms for the duration between the obtaining of location and the use of location for omnibox queries. One set of histograms records the time since the location was first obtained. There is a histogram for each combination of Location Source and whether a location was sent. A second set of histograms records the time since the location was most recently obtained. There is a histogram for each of the Location Sources. BUG=668923 ========== to ========== Add histograms for location timings for Android omnibox queries. Add histograms for the duration between the obtaining of location and the use of location for omnibox queries. One set of histograms records the time since the location was first obtained. There is a histogram for each combination of Location Source and whether a location was sent. A second set of histograms records the time since the location was most recently obtained. There is a histogram for each of the Location Sources. BUG=668923 ==========
lgtm
lgtm, thanks Paul!
pdyson@chromium.org changed reviewers: + tedchoc@chromium.org
Ted - I've added you as an owner reviewer after getting approval form the other reviewers. Please take a look. Thanks.
lgtm w/ small comments https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:185: sFirstLocationTime = System.currentTimeMillis(); Looks like you are using this value for interval tracking. If so, you probably should use one of the SystemClock methods (the top class level description is better here): https://developer.android.com/reference/android/os/SystemClock.html I would probably go with: https://developer.android.com/reference/android/os/SystemClock.html#elapsedRe... https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:562: /** Determines the name for a Time Listening Histogram. Returns empty string if the location Nit: For multi-line javadoc, put the first line after the /** line (to keep the lines indented the same) https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:582: return ""; In addition to the Log, maybe it would be worth adding: assert false : "Unexpected locationSource: " + locationSource; I think returning null would also be better there IMO. https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:590: if (name.equals("")) return; == null if you go with null above https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:606: return; same thing about asserts
The CQ bit was checked by pdyson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Address comments. I have the required approvals so will submit tomorrow. https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java (right): https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:185: sFirstLocationTime = System.currentTimeMillis(); On 2016/12/28 at 18:14:52, Ted C wrote: > Looks like you are using this value for interval tracking. If so, you probably should use one of the SystemClock methods (the top class level description is better here): > > https://developer.android.com/reference/android/os/SystemClock.html > > I would probably go with: > https://developer.android.com/reference/android/os/SystemClock.html#elapsedRe... Done. Thanks. Yes, that is a more suitable clock to use. https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:562: /** Determines the name for a Time Listening Histogram. Returns empty string if the location On 2016/12/28 at 18:14:52, Ted C wrote: > Nit: For multi-line javadoc, put the first line after the /** line (to keep the lines indented the same) Done. https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:582: return ""; On 2016/12/28 at 18:14:52, Ted C wrote: > In addition to the Log, maybe it would be worth adding: > > assert false : "Unexpected locationSource: " + locationSource; > > I think returning null would also be better there IMO. Done. dominickn@ suggested "assert false" and/or Log.e here. I originally added just Log.e, but I'll add "assert false" now that you have suggested it as well. https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:590: if (name.equals("")) return; On 2016/12/28 at 18:14:52, Ted C wrote: > == null if you go with null above Done. https://codereview.chromium.org/2576633002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/omnibox/geo/GeolocationHeader.java:606: return; On 2016/12/28 at 18:14:52, Ted C wrote: > same thing about asserts Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
I changed getTimeListeningHistogramEnum to return null when locationSource was not one of the three expected options (i.e. when it is LOCATION_SOURCE_MASTER_OFF) and changed the if test to if (name == null), but it is telling me that null can't be returned. It can't be returned by the code as it is currently written because I have a test for locationSource before I call recordTimeListeningHistogram which then calls getTimeListeningHistogramEnum. Is the redundant null check that clever? I think it is safer to have the code deal with this case in case someone changes the code later on. Is there a way to keep this? It's failing android_clang_dbg_recipe: RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE: Redundant nullcheck of value known to be non-null In class org.chromium.chrome.browser.omnibox.geo.GeolocationHeader In method org.chromium.chrome.browser.omnibox.geo.GeolocationHeader.recordTimeListeningHistogram(int, boolean, long) Return value of org.chromium.chrome.browser.omnibox.geo.GeolocationHeader.getTimeListeningHistogramEnum(int, boolean) of type String Redundant null check at GeolocationHeader.java:[line 593]
On 2017/01/03 06:20:06, pdyson wrote:
> I changed getTimeListeningHistogramEnum to return null when locationSource was
> not one of the three expected options (i.e. when it is
> LOCATION_SOURCE_MASTER_OFF) and changed the if test to if (name == null), but
it
> is telling me that null can't be returned. It can't be returned by the code as
> it is currently written because I have a test for locationSource before I call
> recordTimeListeningHistogram which then calls getTimeListeningHistogramEnum.
Is
> the redundant null check that clever?
>
> I think it is safer to have the code deal with this case in case someone
changes
> the code later on. Is there a way to keep this?
>
>
> It's failing android_clang_dbg_recipe:
>
> RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE: Redundant nullcheck of value known
to
> be non-null
> In class org.chromium.chrome.browser.omnibox.geo.GeolocationHeader
> In method
>
org.chromium.chrome.browser.omnibox.geo.GeolocationHeader.recordTimeListeningHistogram(int,
> boolean, long)
> Return value of
>
org.chromium.chrome.browser.omnibox.geo.GeolocationHeader.getTimeListeningHistogramEnum(int,
> boolean) of type String
> Redundant null check at GeolocationHeader.java:[line 593]
I wouldn't be surprised if the assert was making it think the return null
was never possible in all instances (but those are stripped out in release
builds).
You can likely add this to the recordTimeListeningHistogram:
@SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NONNULL_VALUE")
The CQ bit was checked by pdyson@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Added the @SuppressFBWarnings. Thanks.
The CQ bit was checked by pdyson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, kcarattini@chromium.org, tedchoc@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2576633002/#ps100001 (title: "Suppress nullcheck warning,")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483506491697020,
"parent_rev": "6a606e04da713860eace398b9789e6c0387fede9", "commit_rev":
"48fee98da79cfae4c49636d197cbf4492ecfecab"}
Message was sent while issue was closed.
Description was changed from ========== Add histograms for location timings for Android omnibox queries. Add histograms for the duration between the obtaining of location and the use of location for omnibox queries. One set of histograms records the time since the location was first obtained. There is a histogram for each combination of Location Source and whether a location was sent. A second set of histograms records the time since the location was most recently obtained. There is a histogram for each of the Location Sources. BUG=668923 ========== to ========== Add histograms for location timings for Android omnibox queries. Add histograms for the duration between the obtaining of location and the use of location for omnibox queries. One set of histograms records the time since the location was first obtained. There is a histogram for each combination of Location Source and whether a location was sent. A second set of histograms records the time since the location was most recently obtained. There is a histogram for each of the Location Sources. BUG=668923 Review-Url: https://codereview.chromium.org/2576633002 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Add histograms for location timings for Android omnibox queries. Add histograms for the duration between the obtaining of location and the use of location for omnibox queries. One set of histograms records the time since the location was first obtained. There is a histogram for each combination of Location Source and whether a location was sent. A second set of histograms records the time since the location was most recently obtained. There is a histogram for each of the Location Sources. BUG=668923 Review-Url: https://codereview.chromium.org/2576633002 ========== to ========== Add histograms for location timings for Android omnibox queries. Add histograms for the duration between the obtaining of location and the use of location for omnibox queries. One set of histograms records the time since the location was first obtained. There is a histogram for each combination of Location Source and whether a location was sent. A second set of histograms records the time since the location was most recently obtained. There is a histogram for each of the Location Sources. BUG=668923 Committed: https://crrev.com/5b6572e3c0aba3d558e173440bd644b2e4b9bd04 Cr-Commit-Position: refs/heads/master@{#441323} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/5b6572e3c0aba3d558e173440bd644b2e4b9bd04 Cr-Commit-Position: refs/heads/master@{#441323} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
