stylo: disable stylo on Android build

RESOLVED FIXED in Firefox 59

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: hiro, Assigned: m_kato)

Tracking

unspecified
mozilla59
Unspecified
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59+ fixed, firefox60 wontfix)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Though I am not sure this bug blocks bug 1366049, just in case.

If we decide to not ship stylo on Android in Firefox 59, we need to build the binary with --disable-stylo.  This bug is for it.
OS: Unspecified → Android
Priority: -- → P1
According to Chris we are planning to disable this in the next beta. 
Once we have a patch, please request uplift to beta.
Priority: P1 → P3
Makoto, when you have a chance, can you please uplift a patch to Beta 59 that disables Stylo for Android? We should do this soon because we didn't get much Nightly testing of Gecko's old style system on Android (because Stylo was enabled for most of Nightly 59).
Assignee: nobody → m_kato
Flags: needinfo?(m_kato)
Flags: needinfo?(m_kato)
Attachment #8945011 - Attachment is obsolete: true
Comment on attachment 8945012 [details] [diff] [review]
Turn off Stylo on Android beta and release

This patch is for beta branch (59).

stylo team postpones stylo  for android until 60 due to performance regression (bug 1430706 and bug 1422522).
Attachment #8945012 - Flags: review?(snorp)
Ikezoe-san, after turning off stylo, the following test doesn't pass.  Could I turn off this on android/beta and release?  Or could you this fix?

dom/animation/test/mozilla/test_restyles.html | Animations running on the main-thread which were in nested scrolled out elements should update restyle soon after the element moved in view by scrolling
got 7, expected 5
Flags: needinfo?(hikezoe)
(In reply to Makoto Kato [:m_kato] from comment #7)
> Ikezoe-san, after turning off stylo, the following test doesn't pass.  Could
> I turn off this on android/beta and release?  Or could you this fix?
> 
> dom/animation/test/mozilla/test_restyles.html | Animations running on the
> main-thread which were in nested scrolled out elements should update restyle
> soon after the element moved in view by scrolling
> got 7, expected 5

This one has been enabled since bug 1427868, I didn't dig into the reason why the test passed at that time, it seem to be thanks to stylo. :)  So, I think we can disable this test case.

But in the try there is another failure case;

 FAIL | dom/animation/test/mozilla/test_restyles.html | CSS animations running on the main-thread should update style on the main thread 

This failure is really bad. This smells there may be some regressions since bug 1335986.

That's said, I have no time to investigate the failure reason, and honestly I don't want spent time to track down the *old* style system failure.  So, please disable whole test_restyles.html on Android.

Thanks!
Flags: needinfo?(hikezoe)
Comment on attachment 8945012 [details] [diff] [review]
Turn off Stylo on Android beta and release

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

bummer!
Attachment #8945012 - Flags: review?(snorp) → review+
Attachment #8945277 - Flags: review?(hikezoe)
I forget comment for part 2. This change is beta branch only
Comment on attachment 8945277 [details] [diff] [review]
Part 2. Skip test_restyles.html on android

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

Please add !stylo condition there, i.e skip the test only on Android without stylo.

Thanks!
Attachment #8945277 - Flags: review?(hikezoe) → review+
carry r+
Attachment #8945277 - Attachment is obsolete: true
Attachment #8945321 - Flags: review+
Comment on attachment 8945012 [details] [diff] [review]
Turn off Stylo on Android beta and release

Approval Request Comment
[Feature/Bug causing the regression]:
bug 1366049

[User impact if declined]:
Disable stylo on 59 release due to performance regressions

[Is this code covered by automated tests?]:
I test on try server

[Has the fix been verified in Nightly?]:
No.  This is 59 branch only

[Needs manual test from QE? If yes, steps to reproduce]: 
Nothing

[List of other uplifts needed for the feature/fix]:
No.

[Is the change risky?]:
Low.

[Why is the change risky/not risky?]:
Until 58, we doesn't turn on stylo on Android.  Also, 59 branch, we run disable-stylo unit tests on desktop build.

[String changes made/needed]:
Nothing
Attachment #8945012 - Flags: approval-mozilla-beta?
Attachment #8945321 - Flags: approval-mozilla-beta?
Comment on attachment 8945012 [details] [diff] [review]
Turn off Stylo on Android beta and release

Turning off Stylo for Android as agreed (for the beta 5 build)
Attachment #8945012 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8945321 [details] [diff] [review]
Part 2. Skip test_restyles.html on android. r=hiro

Thanks for changing the tests too.
Attachment #8945321 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/a27738f5d328
https://hg.mozilla.org/releases/mozilla-beta/rev/27c0ba4c8dad
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.