Closed Bug 1536781 Opened 7 months ago Closed 7 months ago

Consider lowering paint suppression delay on mobile

Categories

(Core :: Layout, defect, P3)

Unspecified
Android
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

(Regressed 1 open bug)

Details

(Whiteboard: [geckoview:p1])

Attachments

(1 file, 2 obsolete files)

On Android the delay is 250, desktop only 5ms.
Painting during load is rather different these days, we lower frame rate after first contentful paint, so hopefully smaller value is ok on mobile too -
at least initial raptor testing hints about it (but tests are noisy).

other tests
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79c53e7f577e6cf047c546653097aea101861db3

The patch reveals unrelated issue breaking one wpt test related to fragment navigation and scrolling.

This does affect to fcp on some pages quite a lot https://i.ibb.co/zRcjwCv/image.png

Aha, the issue is in the test, I'd say. It doesn't work too well on mobile, and with the long suppression which just don't trigger the issue while the test is running. But visually the behavior is there, since there isn't anything to scroll once page has been painted. (Mobile Chrome seems to have issues with the test anyhow)
http://mozilla.pettay.fi/moztests/fragment_nav_orig.html vs
http://mozilla.pettay.fi/moztests/fragment_nav.html

Attachment #9052230 - Attachment is obsolete: true
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a04938b3a757
use the same paint suppression delay on mobile and desktop, r=emilio

Let's see to what all numbers this affects. I wouldn't be surprised if paint scheduling after fcp will need to be tweaked a bit.

Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df1007a56ae9
Backed out changeset a04938b3a757 for android mochitest failures on test_settings_fontinflation.html.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=235813460&revision=a04938b3a757b9141600b2dbc80267fa310c7764

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=235813460&repo=autoland&lineNumber=2002

Backout link: https://hg.mozilla.org/integration/autoland/rev/df1007a56ae9

task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - 134 INFO TEST-PASS | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | system font scale is enabled
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - 135 INFO TEST-PASS | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | text zoom remains at default value
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - Buffered messages finished
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - 136 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | Snapshot canvases are not the same size: 400x559 vs. 980x1370
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
[task 2019-03-25T11:28:53.491Z] 11:28:53 INFO - compareSnapshots@chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js:24:5
[task 2019-03-25T11:28:53.492Z] 11:28:53 INFO - assertSnapshots@chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js:61:5
[task 2019-03-25T11:28:53.492Z] 11:28:53 INFO - test_fontInflationPrecedence@chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_settings_fontinflation.html:159:5
[task 2019-03-25T11:28:53.496Z] 11:28:53 INFO - 137 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_settings_fontinflation.html | reftest comparison: == fontInflationOn fontScaleWithFontInflation
[task 2019-03-25T11:28:53.496Z] 11:28:53 INFO - SimpleTest.ok@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:275:18
[task 2019-03-25T11:28:53.498Z] 11:28:53 INFO - assertSnapshots@chrome://mochikit/content/tests/SimpleTest/WindowSnapshot.js:63:3
[task 2019-03-25T11:28:53.498Z] 11:28:53 INFO - test_fontInflationPrecedence@chrome://mochitests/content/chrome/mobile/android/tests/browser/chrome/test_settings_fontinflation.html:159:5
[task 2019-03-25T11:28:53.498Z] 11:28:53 INFO - 138 INFO Now waiting for load event from browser
[task 2019-03-25T11:29:03.809Z] 11:29:03 INFO - 139 INFO Received event load from browser

Flags: needinfo?(bugs)

FWIW, I filed bug 1538685

JanH, test_settings_fontinflation.html seems to depend on the current very slow paint suppression timer.
Does the test failure ring any bells?

(I'm doing currently some tryserver-debugging, perhaps that reveals something)

Flags: needinfo?(bugs) → needinfo?(jh+bugzilla)

https://hg.mozilla.org/try/rev/f974d7d7d46c0b7f79721beb13fd989743c7fe5a seems to fix the testcase.
Now making that code a bit nicer.

Flags: needinfo?(jh+bugzilla)

No, never encountered that.

Given bug 1538685, is the viewport really ending up with a different size for good, or is that just a transient effect until the MobileViewportManager kicks in and calculates the real correct viewport size, and the changed paint suppression is just somehow causing the test code to run at a different moment relative to viewport calculation than it used to?

Priority: -- → P3
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bfa4f84a030
use the same paint suppression delay on mobile and desktop, r=emilio
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/16103 for changes under testing/web-platform/tests
Duplicate of this bug: 1454529
Attachment #9053165 - Attachment is obsolete: true

Comment on attachment 9053428 [details]
Bug 1536781, use the same paint suppression delay on mobile and desktop, r=emilio

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: NA
  • User impact if declined: Slower first contentful paint during page load
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: I guess this gets tested anyhow while doing any testing on mobile, this is after all about page load.
  • List of other uplifts needed: NA
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Changing just the timer value of paint suppression to the same what desktop has had for ages.
  • String changes made/needed: NA
Attachment #9053428 - Flags: approval-mozilla-beta?
Flags: qe-verify?
OS: Unspecified → Android
See Also: → 1538685
Whiteboard: [geckoview:p1]
Flags: qe-verify? → qe-verify+
QA Whiteboard: [qa-triaged]
  • If yes, steps to reproduce: I guess this gets tested anyhow while doing any testing on mobile, this is after all about page load.

For testing: Just browsing and pay attention to the loading time, or we have to measure also the time compared with an affected build?

Flags: needinfo?(bugs)

For QA just looking if there is something weird happening during page load, I'd say. Some weird painting issues.
Performance measurements are done by raptor tests and such.

Flags: needinfo?(bugs)

smaug: Hi and thank you for the quick reply!

I tested this on the latest version of Nightly 68.0a1 (2019-03-29) with Nexus 6P (Android 8.1.0), Samsung Galaxy Note 8 (Android 9), Samsung Galaxy Tab S3 (Android 8; Tablet) and I didn't see any weird painting issues during page load.
Pages tested:

  • amazon.com;
  • youtube.com;
  • facebook.com;
  • wikipedia.com;
  • twitter.com.
QA Whiteboard: [qa-triaged]
Flags: qe-verify+

thanks for testing. And if in the near future something weird shows up, one can change the pref value from about:config nglayout.initialpaint.delay
It used to be 250 on android but is now 5.

Olli, this bug is a P3, is there a compelling reason to have it in 67 and not let it ride the trains?

Flags: needinfo?(bugs)

Just because it affects to first contentful paint on Android so much.
I don't have too strong opinion.

Flags: needinfo?(bugs)

(In reply to Pascal Chevrel:pascalc from comment #24)

Olli, this bug is a P3, is there a compelling reason to have it in 67 and not let it ride the trains?

67=fix-optional. Neither Fenix MVP nor Firefox for Fire TV will use GeckoView 67, so we don't need to uplift this fix to 67 Beta unless you want it in Fennec 67.

Upstream PR merged

Comment on attachment 9053428 [details]
Bug 1536781, use the same paint suppression delay on mobile and desktop, r=emilio

Given that we are no longer targeting 67 for Geckoview, I think that this can ride the trains to 68, thanks.

Attachment #9053428 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

(In reply to Pascal Chevrel:pascalc from comment #28)

Given that we are no longer targeting 67 for Geckoview, I think that this can ride the trains to 68, thanks.

OK. 67=wontfix

No longer depends on: 1539727
Regressions: 1539727
You need to log in before you can comment on or make changes to this bug.