Closed Bug 1476689 Opened Last year Closed Last year

Use GeckoResult in RuntimeTelemetry

Categories

(GeckoView :: General, enhancement, P3)

51 Branch
All
Android
enhancement

Tracking

(firefox62 fixed, firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox62 --- fixed
firefox63 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

Attachments

(2 files, 3 obsolete files)

RuntimeTelemetry.getSnapshots is still based on GeckoResponse, we should transition it to use GeckoResult instead.
Assignee: nobody → esawin
Attachment #8993044 - Flags: review?(nchen)
Attachment #8993044 - Flags: review?(droeh)
Missed updating the docs.
Attachment #8993044 - Attachment is obsolete: true
Attachment #8993044 - Flags: review?(nchen)
Attachment #8993044 - Flags: review?(droeh)
Attachment #8993047 - Flags: review?(nchen)
Attachment #8993047 - Flags: review?(droeh)
Comment on attachment 8993047 [details] [diff] [review]
0001-Bug-1476689-1.0-Use-GeckoResult-in-runtime-telemetry.patch

Review of attachment 8993047 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/RuntimeTelemetry.java
@@ +50,2 @@
>          mEventDispatcher.dispatch("GeckoView:TelemetrySnapshots", msg,
>              new EventCallback() {

Use `GeckoSession.CallbackResult<GeckoBundle>` here
Attachment #8993047 - Flags: review?(nchen) → review+
Comment on attachment 8993045 [details] [diff] [review]
0002-Bug-1476689-2.0-Add-basic-runtime-telemetry-test.-r-.patch

Review of attachment 8993045 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ +124,5 @@
> +                return null
> +            }
> +        })
> +
> +        sessionRule.waitForResult(result)

You should do

> val result = sessionRule.waitForResult(telemetry.getSnapshots(true))

and then assert on `result` directly, i.e.

> assertThat("Histograms should not be null", result?.get("histograms"), notNullValue())
Attachment #8993045 - Flags: review?(nchen) → review-
Attachment #8993047 - Attachment is obsolete: true
Attachment #8993047 - Flags: review?(droeh)
Attachment #8993625 - Flags: review?(droeh)
Attachment #8993045 - Attachment is obsolete: true
Attachment #8993626 - Flags: review?(nchen)
Comment on attachment 8993625 [details] [diff] [review]
0001-Bug-1476689-1.1-Use-GeckoResult-in-runtime-telemetry.patch

Review of attachment 8993625 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM
Attachment #8993625 - Flags: review?(droeh) → review+
Comment on attachment 8993626 [details] [diff] [review]
0002-Bug-1476689-2.1-Add-basic-runtime-telemetry-test.-r-.patch

Review of attachment 8993626 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt
@@ +4,5 @@
>  
>  package org.mozilla.geckoview.test
>  
>  import org.mozilla.geckoview.GeckoResult
> +import org.mozilla.gecko.util.GeckoBundle

Move to above `import org.mozilla.geckoview.GeckoResult`
Attachment #8993626 - Flags: review?(nchen) → review+
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0048726ae54
[1.1] Use GeckoResult in runtime telemetry API. r=droeh,jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/93a71fc8a13d
[2.2] Add basic runtime telemetry test. r=jchen
https://hg.mozilla.org/mozilla-central/rev/d0048726ae54
https://hg.mozilla.org/mozilla-central/rev/93a71fc8a13d
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
This is blocking the bug 1479026 uplift.
Can you request uplift here? Thanks.
Flags: needinfo?(esawin)
Comment on attachment 8993625 [details] [diff] [review]
0001-Bug-1476689-1.1-Use-GeckoResult-in-runtime-telemetry.patch

Approval Request Comment
[Feature/Bug causing the regression]: N/A.
[User impact if declined]: Blocks bug 1479026.
[Is this code covered by automated tests?]: Yes, patch 2 adds a test.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No. 
[List of other uplifts needed for the feature/fix]: Both patches in this bug.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a simple refactoring and a test patch.
[String changes made/needed]: None.
Flags: needinfo?(esawin)
Attachment #8993625 - Flags: approval-mozilla-beta?
Comment on attachment 8993625 [details] [diff] [review]
0001-Bug-1476689-1.1-Use-GeckoResult-in-runtime-telemetry.patch

Geckoview telemetry fix, includes new tests. Should uplift for beta 16.
Attachment #8993625 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 63 → mozilla63
You need to log in before you can comment on or make changes to this bug.