Screen orientation change in GeckoView doesn't reset cached screen size in GeckoAppShell

RESOLVED FIXED in Firefox 63

Status

enhancement
P2
normal
RESOLVED FIXED
11 months ago
6 months ago

People

(Reporter: JanH, Assigned: JanH)

Tracking

Trunk
mozilla63
All
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 wontfix, firefox63 fixed)

Details

(Whiteboard: [geckoview:klar:p2])

Attachments

(6 attachments)

GeckoApp Fennec calls GeckoScreenOrientation.getInstance().update [1], which in turn calls GeckoAppShell.resetScreenSize() [2] to reset the screen size cached by GeckoAppShell for responding to calls to getScreenSize().

A pure GeckoView implementation on the other hand (e.g. the GeckoView example app) does nothing of the sort, so the cached screen size is never updated after an orientation change.

Bug 1475875 might ultimately result in moving the screen size cache into Gecko itself in order to avoid repeated calls along the lines of child process -> parent -> process -> JNI -> GeckoAppShell, but even then we'd still have to reset that cached value after an orientation change.

[1] https://hg.mozilla.org/mozilla-central/annotate/085cdfb90903d4985f0de1dc7786522d9fb45596/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#l2183
[2] https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoScreenOrientation.java#154
Priority: -- → P3
I'm clearing priority on this so we talk about it in triage again. With this bug, media queries will be wrong for orientation. I think it's at least a P2 because of this.
Priority: P3 → --
Whiteboard: [geckoview:klar]
After some discussion, the preferred solution so far would probably mean moving the calls to GeckoScreenOrientation.update() from GeckoApp into GeckoView, so that not just the screen size information, but also the orientation gets updated correctly and automatically for *all* apps embedding a GeckoView.
Assignee: nobody → jh+bugzilla
OS: Unspecified → Android
Hardware: Unspecified → All
> 21:19:26 - snorp: JanH: we shouldn't have GeckoView call GeckoScreenOrientation directly, IMHO
> 21:19:32 - snorp: JanH: it should be via some new GeckoSession API
> 21:19:38 - snorp: (setOrientation? updateOrientation?)
> 21:19:45 - JanH: okay
> 21:19:49 - snorp: if setOrientation, we'd pass in the changed value if we have it
> 21:20:01 - snorp: we won't have it in construction though
> 21:20:02 - snorp: so meh
> 21:20:09 - snorp: maybe just orientationChanged()
Assignee

Updated

11 months ago
Depends on: 1475875
See Also: 1475875
Priority: -- → P2
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8997541 [details]
Bug 1476106 - Part 1 - Make it possible to notify Java listeners from GeckoScreenOrientation, too.

https://reviewboard.mozilla.org/r/261084/#review268364
Attachment #8997541 - Flags: review?(snorp) → review+
Comment on attachment 8997542 [details]
Bug 1476106 - Part 3 - Move GeckoScreenOrientation updates into GeckoView.

https://reviewboard.mozilla.org/r/261086/#review268368

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java:1395
(Diff revision 1)
>      }
>  
>      /**
> +     * Notify Gecko that the screen orientation has changed.
> +     */
> +    public void orientationChanged() {

I think I said GeckoSession when we talked about this earlier, but since this is a global thing it should probably go in GeckoRuntime.
Attachment #8997542 - Flags: review?(snorp) → review+
Comment on attachment 8997543 [details]
Bug 1476106 - Part 4 - Refresh ScreenManager data when detecting orientation changes.

https://reviewboard.mozilla.org/r/261088/#review268370
Attachment #8997543 - Flags: review?(snorp) → review+
Comment on attachment 8997544 [details]
Bug 1476106 - Part 5 - Subscribe PromptService to OrientationChangeListener, too.

https://reviewboard.mozilla.org/r/261090/#review268374
Attachment #8997544 - Flags: review?(snorp) → review+
Assignee

Comment 13

11 months ago
mozreview-review-reply
Comment on attachment 8997542 [details]
Bug 1476106 - Part 3 - Move GeckoScreenOrientation updates into GeckoView.

https://reviewboard.mozilla.org/r/261086/#review268368

> I think I said GeckoSession when we talked about this earlier, but since this is a global thing it should probably go in GeckoRuntime.

You did say that, but I've moved it and also discovered another small bug along the way.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 20

11 months ago
mozreview-review
Comment on attachment 8997540 [details]
Bug 1476106 - Part 0 - Cleanup imports.

https://reviewboard.mozilla.org/r/261082/#review268436
Attachment #8997540 - Flags: review+
Comment on attachment 8997655 [details]
Bug 1476106 - Part 2 - Fix setting of mRuntime when restoring GeckoView from savedInstanceState.

https://reviewboard.mozilla.org/r/261344/#review268452
Attachment #8997655 - Flags: review?(snorp) → review+
Do we need to uplift this fix to GV 62 Beta?
Once this bug is fixed, we should verify whether the following Focus issue is fixed:

https://github.com/mozilla-mobile/focus-android/issues/3042

Comment 24

11 months ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/6d885f08136b
Part 0 - Cleanup imports. r=JanH
https://hg.mozilla.org/integration/autoland/rev/3b1084efc943
Part 1 - Make it possible to notify Java listeners from GeckoScreenOrientation, too. r=snorp
https://hg.mozilla.org/integration/autoland/rev/deb22f602d59
Part 2 - Fix setting of mRuntime when restoring GeckoView from savedInstanceState. r=snorp
https://hg.mozilla.org/integration/autoland/rev/b1a3ff0760bf
Part 3 - Move GeckoScreenOrientation updates into GeckoView. r=snorp
https://hg.mozilla.org/integration/autoland/rev/ab38b11f85dd
Part 4 - Refresh ScreenManager data when detecting orientation changes. r=snorp
https://hg.mozilla.org/integration/autoland/rev/bf1f28e2b46d
Part 5 - Subscribe PromptService to OrientationChangeListener, too. r=snorp
Backed out 6 changesets (Bug 1476106) for failure at bf1f28e2b46d for failure at testing/marionette/harness/marionette_harness/tests/unit/test_screen_orientation.py

Backout:
https://hg.mozilla.org/integration/autoland/rev/b7cd9638d4a4c7d2f9df0cf7dbcb2a85bbafa7b7

Push link:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bf1f28e2b46d1580b889c90511d42a887ead5f2c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=192850184&repo=autoland&lineNumber=1864

[task 2018-08-08T19:43:52.761Z] 19:43:52     INFO -  mozcrash Copy/paste: /usr/local/bin/linux64-minidump_stackwalk /tmp/tmpPKKbY_/1206c063-5660-a1bf-1279-9a59441db44c.dmp /builds/worker/workspace/build/symbols
[task 2018-08-08T19:44:03.614Z] 19:44:03     INFO -  mozcrash Saved minidump as /builds/worker/workspace/build/blobber_upload_dir/1206c063-5660-a1bf-1279-9a59441db44c.dmp
[task 2018-08-08T19:44:03.615Z] 19:44:03     INFO -  mozcrash Saved app info as /builds/worker/workspace/build/blobber_upload_dir/1206c063-5660-a1bf-1279-9a59441db44c.extra
[task 2018-08-08T19:44:03.619Z] 19:44:03  WARNING -  PROCESS-CRASH | testing/marionette/harness/marionette_harness/tests/unit/test_screen_orientation.py TestScreenOrientation.test_set_orientation_to_landscape_primary | application crashed [@ mozilla::jni::Accessor::EndAccess<mozilla::java::AndroidGamepadManager::OnGamepadAdded_t>(mozilla::java::AndroidGamepadManager::OnGamepadAdded_t::Owner::Context const&, nsresult*) + 0x16]
[task 2018-08-08T19:44:03.619Z] 19:44:03     INFO -  Crash dump filename: /tmp/tmpPKKbY_/1206c063-5660-a1bf-1279-9a59441db44c.dmp
[task 2018-08-08T19:44:03.620Z] 19:44:03     INFO -  Operating system: Android
[task 2018-08-08T19:44:03.620Z] 19:44:03     INFO -                    0.0.0 Linux 2.6.29-gea477bb #1 Wed Sep 26 11:04:45 PDT 2012 armv7l
[task 2018-08-08T19:44:03.621Z] 19:44:03     INFO -  CPU: arm
[task 2018-08-08T19:44:03.621Z] 19:44:03     INFO -       ARMv7 ARM Cortex-A8 features: swp,half,thumb,fastmult,vfpv2,edsp,neon,vfpv3
[task 2018-08-08T19:44:03.621Z] 19:44:03     INFO -       1 CPU
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -  GPU: UNKNOWN
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -  Crash reason:  SIGSEGV
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -  Crash address: 0x0
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -  Process uptime: not available
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -  Thread 12 (crashed)
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -   0  libxul.so!void mozilla::jni::Accessor::EndAccess<mozilla::java::AndroidGamepadManager::OnGamepadAdded_t>(mozilla::java::AndroidGamepadManager::OnGamepadAdded_t::Owner::Context const&, nsresult*) + 0x16
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -       r0 = 0x00000000    r1 = 0x0000003b    r2 = 0x600c899d    r3 = 0x0000003b
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -       r4 = 0x4087aa4d    r5 = 0x2a1f9bf8    r6 = 0x52ffbe84    r7 = 0x52ffbe58
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -       r8 = 0x00000000    r9 = 0x00000000   r10 = 0x55a10800   r12 = 0x00000003
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -       fp = 0x52ffc058    sp = 0x52ffbe58    lr = 0x5de5910f    pc = 0x5de590e6
[task 2018-08-08T19:44:03.628Z] 19:44:03     INFO -      Found by: given as instruction pointer in context
Flags: needinfo?(jh+bugzilla)
Er, it's been backed out, wasn't it?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 63 → ---
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

10 months ago
Flags: needinfo?(jh+bugzilla)
Attachment #8997542 - Flags: review+ → review?(snorp)
Assignee

Updated

10 months ago
Attachment #8997541 - Flags: review+ → review?(snorp)
Attachment #8997542 - Flags: review?(snorp) → review+
Attachment #8997541 - Flags: review?(snorp) → review+

Comment 33

10 months ago
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/20b991052ad7
Part 0 - Cleanup imports. r=JanH
https://hg.mozilla.org/integration/autoland/rev/1484cdde97b8
Part 1 - Make it possible to notify Java listeners from GeckoScreenOrientation, too. r=snorp
https://hg.mozilla.org/integration/autoland/rev/70bfe613b325
Part 2 - Fix setting of mRuntime when restoring GeckoView from savedInstanceState. r=snorp
https://hg.mozilla.org/integration/autoland/rev/e936ef6b4b50
Part 3 - Move GeckoScreenOrientation updates into GeckoView. r=snorp
https://hg.mozilla.org/integration/autoland/rev/67b327d75ce6
Part 4 - Refresh ScreenManager data when detecting orientation changes. r=snorp
https://hg.mozilla.org/integration/autoland/rev/73efbce701a9
Part 5 - Subscribe PromptService to OrientationChangeListener, too. r=snorp
Depends on: 1483893
Whiteboard: [geckoview:klar] → [geckoview:klar:p2]
Assignee

Updated

10 months ago
Depends on: 1484001

Updated

6 months ago
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.