Closed Bug 1583575 Opened 9 months ago Closed 4 months ago

Use something better than 1x1 for initial window size

Categories

(GeckoView :: General, enhancement, P1)

Unspecified
Android
enhancement

Tracking

(firefox70 wontfix, firefox71 wontfix, firefox75 fixed)

RESOLVED FIXED
mozilla75
Tracking Status
firefox70 --- wontfix
firefox71 --- wontfix
firefox75 --- fixed

People

(Reporter: snorp, Assigned: bdahl)

References

Details

(Keywords: good-first-bug, Whiteboard: [geckoview:m1910][geckoview:m75])

Attachments

(1 file, 1 obsolete file)

Currently, a new GeckoSession creates a Gecko window of size 1x1. It is later resized to the bounds of the attached Surface, assuming that occurs. This is problematic because we're basically guaranteed to perform a spurious relayout of the page going from 1x1 to whatever the real size is. We could improve upon this by just tracking the most recent resize of any window and using that as the default instead of 1x1.

This would be a small perf improvement when opening new tabs because we could avoid a spurious relayout to right-size the window.

Rank: 55
Keywords: good-first-bug
OS: All → Android
Priority: -- → P2

Taking as my first GV bug to get up to speed.

Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Priority: P2 → P1

Tracking for GV's October sprint.

Whiteboard: [geckoview:m1910]

This obviously only improves the situation when a subsequent session is the
same size as the last session that was opened.

Attachment #9104063 - Attachment description: Bug 1583575: Retain window size between GeckoSessions and use it to set the size when opening a new window; r=#geckoview-reviewers! → Bug 1583575: Retain window size between instnaces and use it to set the size when opening a new window; r=snorp
Attachment #9104063 - Attachment description: Bug 1583575: Retain window size between instnaces and use it to set the size when opening a new window; r=snorp → Bug 1583575: Retain window size between instances and use it to set the size when opening a new window; r=snorp!
Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/10e1c85b13b0
Retain window size between instances and use it to set the size when opening a new window; r=snorp
Depends on: 1592808

I am fixing the test issues in bug 1592808, but it does raise the question: Should the nsWindow::Resize call that is setting the previous window's width and height generate a OnSizeChanged, or should we be avoiding it since we are in the midst of creation?

Flags: needinfo?(aklotz) → needinfo?(snorp)

(In reply to Aaron Klotz [:aklotz] from comment #7)

I am fixing the test issues in bug 1592808, but it does raise the question: Should the nsWindow::Resize call that is setting the previous window's width and height generate a OnSizeChanged, or should we be avoiding it since we are in the midst of creation?

It seems like we'd want to call it, and it looks like it should be calling OnSizeChanged() today even before your patch. Is there a good reason to avoid it?

Flags: needinfo?(snorp)

No reason on my part, just making sure that I wasn't missing anything.

Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/da311583ecc1
Retain window size between instances and use it to set the size when opening a new window; r=snorp

Backed out for gv-junit failures.

Failed on a subsequent push, as the builds did not run on the landed one due to some bustages from a previous push.
e.g. https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=274040241&revision=787484b40d10480d0d375604595c8734e550c70e&searchStr=android%2C7.0%2Cx86-64%2Cdebug%2Ctest-android-em-7.0-x86_64%2Fdebug-geckoview-junit-e10s%2C%28gv-junit%29

backout: https://hg.mozilla.org/integration/autoland/rev/6fab67ec8c3236848965eec3776a3b628d019124

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=274040242&repo=autoland&lineNumber=2071

[task 2019-11-01T09:02:09.107Z] 09:02:09 INFO - TEST-START | org.mozilla.geckoview.test.FinderTest.find
[task 2019-11-01T09:02:09.808Z] 09:02:09 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: numtests=537
[task 2019-11-01T09:02:09.808Z] 09:02:09 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stream=
[task 2019-11-01T09:02:09.808Z] 09:02:09 INFO - org.mozilla.geckoview.test | Error in find(org.mozilla.geckoview.test.FinderTest):
[task 2019-11-01T09:02:09.808Z] 09:02:09 INFO - org.mozilla.geckoview.test | java.lang.AssertionError: Should have wrapped
[task 2019-11-01T09:02:09.808Z] 09:02:09 INFO - org.mozilla.geckoview.test | Expected: <true>
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | but: was <false>
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.Assert.assertThat(Assert.java:956)
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector$1.call(ErrorCollector.java:65)
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector.checkSucceeds(ErrorCollector.java:78)
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector.checkThat(ErrorCollector.java:63)
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.checkThat(GeckoSessionTestRule.java:796)
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.BaseSessionTest.assertThat(BaseSessionTest.kt:75)
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.FinderTest.find(FinderTest.kt:28)
[task 2019-11-01T09:02:09.809Z] 09:02:09 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Native Method)
[task 2019-11-01T09:02:09.810Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
[task 2019-11-01T09:02:09.810Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule$2.lambda$evaluate$0$GeckoSessionTestRule$2(GeckoSessionTestRule.java:1257)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.-$$Lambda$GeckoSessionTestRule$2$mzZNnl5Bu5F2_4xGxj0DHU4J33I.run(lambda)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1950)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.os.Handler.handleCallback(Handler.java:751)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.os.Handler.dispatchMessage(Handler.java:95)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.os.Looper.loop(Looper.java:154)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.app.ActivityThread.main(ActivityThread.java:6077)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Native Method)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:866)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:756)
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test |
[task 2019-11-01T09:02:09.811Z] 09:02:09 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: id=AndroidJUnitRunner
[task 2019-11-01T09:02:09.812Z] 09:02:09 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: test=find
[task 2019-11-01T09:02:09.812Z] 09:02:09 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: class=org.mozilla.geckoview.test.FinderTest
[task 2019-11-01T09:02:09.812Z] 09:02:09 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: stack=java.lang.AssertionError: Should have wrapped
[task 2019-11-01T09:02:09.812Z] 09:02:09 INFO - org.mozilla.geckoview.test | Expected: <true>
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | but: was <false>
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.Assert.assertThat(Assert.java:956)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector$1.call(ErrorCollector.java:65)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector.checkSucceeds(ErrorCollector.java:78)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.rules.ErrorCollector.checkThat(ErrorCollector.java:63)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule.checkThat(GeckoSessionTestRule.java:796)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.BaseSessionTest.assertThat(BaseSessionTest.kt:75)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.FinderTest.find(FinderTest.kt:28)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Native Method)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.GeckoSessionTestRule$2.lambda$evaluate$0$GeckoSessionTestRule$2(GeckoSessionTestRule.java:1257)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at org.mozilla.geckoview.test.rule.-$$Lambda$GeckoSessionTestRule$2$mzZNnl5Bu5F2_4xGxj0DHU4J33I.run(lambda)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.app.Instrumentation$SyncRunnable.run(Instrumentation.java:1950)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.os.Handler.handleCallback(Handler.java:751)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.os.Handler.dispatchMessage(Handler.java:95)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.os.Looper.loop(Looper.java:154)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at android.app.ActivityThread.main(ActivityThread.java:6077)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at java.lang.reflect.Method.invoke(Native Method)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:866)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:756)
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test |
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS: current=30
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - org.mozilla.geckoview.test | INSTRUMENTATION_STATUS_CODE: -2
[task 2019-11-01T09:02:09.817Z] 09:02:09 WARNING - TEST-UNEXPECTED-FAIL | org.mozilla.geckoview.test.FinderTest.find | status -2
[task 2019-11-01T09:02:09.817Z] 09:02:09 INFO - TEST-INFO took 705ms

Flags: needinfo?(aklotz)
Flags: needinfo?(aklotz)
Priority: P1 → P2

This issue has a patch but has failing tests. Whoever picks this up should coordinate with :aklotz, but it should be fairly simple to take it over and run with it.

Rank: 55
Priority: P2 → P1
Whiteboard: [geckoview:m1910] → [geckoview:m1910][geckoview:m75]

persist=attributes on window might be solution according to bdahl.

Assignee: aklotz → bdahl
Duplicate of this bug: 1613488

The other widget backends do not call resize during nsWindow creation and
there's not really a need to do it. The bounds can just be adjusted at the
beginning.

After some digging, I don't think this is actually an issue. During both startup and new tab creation all of the resize events happen before we ever begin layout for geckoview.xhtml (when using the PrototypeCache gecko doesn't start layout until READYSTATE_INTERACTIVE). I don't think it really matters much, but we can avoid doing a resize during nsWindow::Create and just initialize the first window size to 0x0. I'll attach a patch to do that.

For example starting gecko view example (without doing the 0x0 resize):

onScreenOriginChanged 0 63
Surface changed 1080 1584
onScreenOriginChanged 0 63
nsWindow[0x719b531e8000]::Create 0x0 [0 0 1 1]
nsWindow[0x719b531e8000]::Resize [0.000000 63.000000 1080.000000 1584.000000] (repaint 0)
nsWindow: 0x719b531e8000 OnSizeChanged [1080 1584]
nsWindow[0x719b531e8000]::Resize [0.000000 63.000000 1080.000000 1584.000000] (repaint 0)
BeforeStartLayout[0x719b531e8000]
nsWindow[0x719b531e8000]::Show 1
Pushed by bdahl@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5c14336bb46c
Remove resize when creating GeckoView window. r=geckoview-reviewers,snorp
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Attachment #9104063 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.