1314974 - [geckoview] Ensure GeckoView doesn't reference ScreenManagerHelper

Status: RESOLVED FIXED

(GeckoView :: General, defect)

Description

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

Bug 1282003 busted it

Comment 1

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

Attachment: Make GeckoView on Android work again

Comment 2

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

Comment on attachment 8807164 [details] [diff] [review]
Make GeckoView on Android work again

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

::: mobile/android/base/moz.build
@@ +276,5 @@
>      'permissions/PermissionBlock.java',
>      'permissions/Permissions.java',
>      'permissions/PermissionsHelper.java',
>      'PrefsHelper.java',
> +    'ScreenManagerHelper.java',

Why do we need to move this? If it's to make [1] not crash, then we need to call `mozilla::jni::IsFennec()` in the line above, similar to [2].

[1] https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/widget/android/nsScreenManagerAndroid.cpp#179
[2] https://dxr.mozilla.org/mozilla-central/rev/ade8d4a63e57560410de106450f37b50ed71cca5/widget/android/nsAppShell.cpp#385

Comment 3

[Nick Alexander :nalexander [he/him]]

While we're fixing bustage from Bug 1282003, note that https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoView.java?q=path%3AGeckoView.java&redirect_type=single#52 needs to be fixed as well.

This ticket underscores the need to have a GeckoView example running in automation; we can't expect review to catch the wide scope of breaking changes possible in the current situation.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander (leave until January 2017) from comment #3)
> While we're fixing bustage from Bug 1282003, note that
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/
> main/java/org/mozilla/gecko/GeckoView.java?q=path%3AGeckoView.
> java&redirect_type=single#52 needs to be fixed as well.

Oh, I see your patch fixes this bustage.  Great!  snorp, I can review a fresh patch while Jim is on PTO.

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8807164 [details] [diff] [review]
Make GeckoView on Android work again

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

Can we land some IsFennec fix immediately to unbust this?  It's blocking me locally.  (This is not critical since this is just my side-project while on parental leave, but it's irritating me nonetheless.)

::: mobile/android/base/moz.build
@@ +281,1 @@
>      'sqlite/ByteBufferInputStream.java',

Isn't the question whether screen management should be part of GeckoView or part of Fennec?  Given that this is about nsScreen, it seems like it's a GV thing.  (I see comments about "adding and removing Android displays", which doesn't sound Fennec-specific.)

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aabb06ac90d4

Comment 7

[Nick Alexander :nalexander [he/him]]

snorp: ping on this.  I'd really like to get some GVE test into the tree, and I can't work around this easily...

Comment 8

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

(In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> 
> Why do we need to move this? If it's to make [1] not crash, then we need to
> call `mozilla::jni::IsFennec()` in the line above, similar to [2].
> 

Whoops, missed this comment. Indeed as Nick says, nsScreenManager is not really Fennec-specific, though the reason it was added is for Fennec. I think the IsFennec() stuff was added in order to pass xpcshell tests (where we have no Java env at all).

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71543633d754
Make GeckoView on Android work again r=jchen

Comment 10

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #2)
> > 
> > Why do we need to move this? If it's to make [1] not crash, then we need to
> > call `mozilla::jni::IsFennec()` in the line above, similar to [2].
> > 
> 
> Whoops, missed this comment. Indeed as Nick says, nsScreenManager is not
> really Fennec-specific, though the reason it was added is for Fennec. I
> think the IsFennec() stuff was added in order to pass xpcshell tests (where
> we have no Java env at all).

That's jni::IsAvailable(), which makes xpcshell work without Java. jni::IsFennec() is for cases where we have JNI code that's specific to Fennec classes.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71543633d754