1463790 - Provide a Kotlin-ified GV API

Status: NEW

(GeckoView :: General, enhancement, P3)

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think the way we'd do this is to provide a Kotlin extension module. GeckoView KTX.

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1463790 - Introduce a Kotlin extension module for GeckoView

Review commit: https://reviewboard.mozilla.org/r/255594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255594/

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8990527 [details]
Bug 1463790 - Introduce a Kotlin extension module for GeckoView

This is a first crack at GeckoView KTX. Currently it simply adds GeckoResult.await() for coroutine integration.

I cargo culted a ton of build.gradle stuff, but it's unclear how much we really need. There are no docs, but apparently we can use Dokka[1] for this.

[1] https://github.com/Kotlin/dokka

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8990527 [details]
Bug 1463790 - Introduce a Kotlin extension module for GeckoView

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255594/diff/1-2/

Comment 5

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8990527 [details]
Bug 1463790 - Introduce a Kotlin extension module for GeckoView

I'm not really familiar enough with gradle to be a judge on that, but the Kotlin side of this patch looks good to me.

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8990527 [details]
Bug 1463790 - Introduce a Kotlin extension module for GeckoView

https://reviewboard.mozilla.org/r/255594/#review262536

::: gradle.properties:4
(Diff revision 1)
>  org.gradle.parallel=true
>  org.gradle.daemon=true
>  org.gradle.jvmargs=-Xmx2560M
> +kotlin.coroutines=enable

Interesting. I didn't know that you can do that in gradle.properties. So far I have only seen the snippet that enables coroutines in build.gradle.

::: gradle.properties:4
(Diff revision 1)
>  org.gradle.parallel=true
>  org.gradle.daemon=true
>  org.gradle.jvmargs=-Xmx2560M
> +kotlin.coroutines=enable

Interesting. I didn't know that you can do that in gradle.properties. So far I have only seen the snippet that enables coroutines in build.gradle.

::: mobile/android/geckoview_ktx/build.gradle:36
(Diff revision 1)
> +
> +uploadArchives {
> +    repositories.mavenDeployer {
> +        pom.groupId = 'org.mozilla'
> +        pom.artifactId = "geckoview-ktx"
> +        pom.version = "${geckoview_ktx_versionName}"

Does this build three different geckoview*-ktx libraries for each architecture?

If yes: Do we want this?

For our browser-engine-gecko component we avoid this by using "compileOnly" instead of "implementation" for adding the GeckoView dependency. This leaves it to the app to add the architecture(s) it wants. This works assuming that all architectures have the same public API.

If we want to have three different ktx artifacts (which is okay too) then we could consider switching the GeckoView dependency from "implementation" to "api". With that every app using GeckoView*-ktx gets access to the GeckoView classes without having to declare the GeckoView dependency with *the same* version in the app project again.

::: mobile/android/geckoview_ktx/src/androidTest/java/org/mozilla/geckoview/ktx/GeckoResultTests.kt:15
(Diff revision 1)
> +
> +import org.junit.Assert.*
> +import org.mozilla.geckoview.GeckoResult
> +
> +@RunWith(AndroidJUnit4::class)
> +class GeckoResultTests {

In theory this could be in "test" (run on machine) instead of "androidTest" (run on device). But I am not sure what our automation situation is here?

::: mobile/android/geckoview_ktx/src/main/java/org/mozilla/geckoview/ktx/GeckoResult.kt:1
(Diff revision 1)
> +package org.mozilla.geckoview.ktx

nit: license header here and in the other files?

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8990527 [details]
Bug 1463790 - Introduce a Kotlin extension module for GeckoView

https://reviewboard.mozilla.org/r/255594/#review262536

> Does this build three different geckoview*-ktx libraries for each architecture?
> 
> If yes: Do we want this?
> 
> For our browser-engine-gecko component we avoid this by using "compileOnly" instead of "implementation" for adding the GeckoView dependency. This leaves it to the app to add the architecture(s) it wants. This works assuming that all architectures have the same public API.
> 
> If we want to have three different ktx artifacts (which is okay too) then we could consider switching the GeckoView dependency from "implementation" to "api". With that every app using GeckoView*-ktx gets access to the GeckoView classes without having to declare the GeckoView dependency with *the same* version in the app project again.

It does not, because there is no native code here, but it does have the same "withGeckoBinariesNoMinApi" flavor/dimension stuff as geckoview. That doesn't leak out into the Maven repo, though.

It seems like compileOnly is probably what I we want here, so I'll look into that.

> In theory this could be in "test" (run on machine) instead of "androidTest" (run on device). But I am not sure what our automation situation is here?

GeckoResult doesn't run in Roboelectric becauses it uses Handler/Looper, which is (surprisingly) unsupported.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

One thing I'm wondering about is whether we need to version "geckoview-ktx" the same as "geckoview". I would expect the ktx module to change far less often and be usable over a variety of geckoview versions.

Comment 9

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8990527 [details]
Bug 1463790 - Introduce a Kotlin extension module for GeckoView

https://reviewboard.mozilla.org/r/255594/#review262598

This looks more or less as I'd expect.

::: gradle.properties:4
(Diff revision 2)
>  org.gradle.parallel=true
>  org.gradle.daemon=true
>  org.gradle.jvmargs=-Xmx2560M
> +kotlin.coroutines=enable

This isn't very flexible.  Can we avoid it by pushing this flag into `build.gradle`, which Sebastian suggested is possible?

::: mobile/android/geckoview_ktx/build.gradle:1
(Diff revision 2)
> +buildDir "${topobjdir}/gradle/build/mobile/android/geckoview_ktx"

license block?

::: mobile/android/geckoview_ktx/build.gradle:52
(Diff revision 2)
> +        repository(url: "file://${project.buildDir}/maven")
> +    }
> +}
> +
> +
> +// This is all related to the withGeckoBinaries approach; see

Hurgh, this hasn't generalized well to multiple products.

But reworking the interleaving between |mach build|, JNI wrapper generation, linking libxul.so, and |mach package|, is well out of scope for this ticket.

So fine, if this works for you.  But I would much rather you simplify this if you can, so that `debug` includes binaries, perhaps just by including `geckoview:debug`.  I.e., just have `debug` and `release` at this level, rather than the madness with all of the flavour dimensions.  (Those are really Fennec specific things, after the `{with,without}GradleBinaries` bits.)

::: mobile/android/geckoview_ktx/build.gradle:77
(Diff revision 2)
> +        archives bundleOfficialWithGeckoBinariesNoMinApiRelease
> +    }
> +}
> +
> +dependencies {
> +    testImplementation 'junit:junit:4.12'

Please group related things.

::: mobile/android/geckoview_ktx/build.gradle:86
(Diff revision 2)
> +
> +    androidTestImplementation 'com.android.support.test:runner:1.0.2'
> +    implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk7:$kotlin_version"
> +    implementation "org.jetbrains.kotlinx:kotlinx-coroutines-android:0.23.4"
> +
> +    // Not defining this library again results in test-app assuming 23.1.1, and the following errors:

https://bugzilla.mozilla.org/show_bug.cgi?id=1385464 should be changing this, so please be sure to update if that lands first.

::: mobile/android/geckoview_ktx/build.gradle:94
(Diff revision 2)
> +
> +    implementation project(path: ':geckoview')
> +}
> +
> +repositories {
> +    mavenCentral()

Eh?  We have special code to support repositories to fetch from in automation (see https://searchfox.org/mozilla-central/source/mobile/android/config/mozconfigs/android-api-16-gradle-dependencies/nightly#18) that would need to roll forward for `mavenCentral`.  In general, you shouldn't need it, since `jcenter` mirrors `mavenCentral`, IIUC.  And then you shouldn't need a per-project `repositories {}` block at all; which is good, because `android-gradle-dependencies` is global, not per-project.

I expect you need to bump `gradle.configure` to have dependency fetching work for these new dependencies, too.

::: mobile/android/geckoview_ktx/src/androidTest/java/org/mozilla/geckoview/ktx/GeckoResultTests.kt:1
(Diff revision 2)
> +package org.mozilla.geckoview.ktx

LICENSE BLOCK THROUGHOUT

::: mobile/android/geckoview_ktx/src/main/java/org/mozilla/geckoview/ktx/GeckoResult.kt:7
(Diff revision 2)
> +
> +import org.mozilla.geckoview.GeckoResult
> +import kotlin.coroutines.experimental.suspendCoroutine
> +
> +/**
> + * Waits for a GeckoResult to complete without blocking the current thread. Runs in

Can you linkify `GeckoResult`?

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8990527 [details]
Bug 1463790 - Introduce a Kotlin extension module for GeckoView

https://reviewboard.mozilla.org/r/255594/#review262598

> This isn't very flexible.  Can we avoid it by pushing this flag into `build.gradle`, which Sebastian suggested is possible?

Yeah. All of the Kotlin docs use gradle.properties, so I was following that.

> Hurgh, this hasn't generalized well to multiple products.
> 
> But reworking the interleaving between |mach build|, JNI wrapper generation, linking libxul.so, and |mach package|, is well out of scope for this ticket.
> 
> So fine, if this works for you.  But I would much rather you simplify this if you can, so that `debug` includes binaries, perhaps just by including `geckoview:debug`.  I.e., just have `debug` and `release` at this level, rather than the madness with all of the flavour dimensions.  (Those are really Fennec specific things, after the `{with,without}GradleBinaries` bits.)

Would you mind elaborating on this a bit more? I would love to get rid of the withGeckoBinaries flavors with GV, if that's what you're implying.

> Eh?  We have special code to support repositories to fetch from in automation (see https://searchfox.org/mozilla-central/source/mobile/android/config/mozconfigs/android-api-16-gradle-dependencies/nightly#18) that would need to roll forward for `mavenCentral`.  In general, you shouldn't need it, since `jcenter` mirrors `mavenCentral`, IIUC.  And then you shouldn't need a per-project `repositories {}` block at all; which is good, because `android-gradle-dependencies` is global, not per-project.
> 
> I expect you need to bump `gradle.configure` to have dependency fetching work for these new dependencies, too.

I think this was just accidentally included from when Studio created the module.

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8990527 [details]
Bug 1463790 - Introduce a Kotlin extension module for GeckoView

https://reviewboard.mozilla.org/r/255594/#review262598

> Would you mind elaborating on this a bit more? I would love to get rid of the withGeckoBinaries flavors with GV, if that's what you're implying.

I can try.  We have, at https://searchfox.org/mozilla-central/rev/28daa2806c89684b3dfa4f0b551db1d099dda7c2/mobile/android/gradle/product_flavors.gradle#7, 3 flavor dimensions:

`audience`, `minApi`, `geckoBinaries`

In addition, we have the `debug` configuration type.  These are isomorphic except when they aren't; you can't really add new configuration types that really behave like `debug`.

`geckoBinaries` is incredibly awkward but has to stay; it's the thing that makes us all not cry as local developers.  (Without it, we'd never get Fennec or GV builds that could actually launch on a device.)

`audience` is also Fennec specific.  For historical reasons related to Proguard, Fennec's _production_ APK is built as a `debug` type, _not_ as a `release` type.  So `audience` fills the gap between `debug` vs `release` and `developer` vs `production` (essentially, `MOZILLA_OFFICIAL`).  None of that makes sense for GV.  You don't get to choose your configuration type (that's up to the consuming App) and we shouldn't be exposing `developer` things to anything but `debug` builds.  (And `MOZILLA_OFFICIAL` makes no sense for library consumers either.)

`minApi` is very Fennec specific -- GV should just set a single target API and live with it.  It doesn't make sense for libraries to target more than one API level.  (In fact, Fennec only targets multiple APIs to support developer features, most of which don't make sense for GV anyway.)

So if we could hammer down GV (and this little wrapper thing) to just have `{with,without}GeckoBinaries` and either `debug` or `release` configuration type, that would be good.

You might find that all the `withGeckoBinaries` stuff Just Works without the `minApi`/`audience` noise... that would be pleasant.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8990527 [details]
Bug 1463790 - Introduce a Kotlin extension module for GeckoView

Note that it's advised to add ".experimental" to the package name [1] of libraries exposing coroutine-based APIs, so I think we should do that.

[1] https://kotlinlang.org/docs/reference/coroutines.html#experimental-status-of-coroutines

Comment 14

[Chris Peterson [:cpeterson]]

Tasks and enhancements should have severity N/A.