Closed Bug 1314974 Opened 5 years ago Closed 5 years ago

[geckoview] Ensure GeckoView doesn't reference ScreenManagerHelper

Categories

(GeckoView :: General, defect)

defect
Not set
normal

Tracking

(firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: snorp, Assigned: snorp)

References

Details

Attachments

(1 file)

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
Attachment #8807164 - Flags: review?(nchen) → feedback+
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.
Summary: Make GeckoView work again → [geckoview] Ensure GeckoView doesn't reference ScreenManagerHelper
(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 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.)
snorp: ping on this.  I'd really like to get some GVE test into the tree, and I can't work around this easily...
Flags: needinfo?(snorp)
(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).
Flags: needinfo?(snorp)
Pushed by jwillcox@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/71543633d754
Make GeckoView on Android work again r=jchen
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
https://hg.mozilla.org/mozilla-central/rev/71543633d754
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Assignee: nobody → snorp
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 53 → mozilla53
You need to log in before you can comment on or make changes to this bug.