1432354 - stylo: disable stylo on Android build

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 1

[Liz Henry (:lizzard) (relman/hg->git project)]

According to Chris we are planning to disable this in the next beta. 
Once we have a patch, please request uplift to beta.

Comment 2

[Chris Peterson [:cpeterson]]

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).

Comment 3

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0ef580f0bd1a85da0350909a709d35fcceb3e50

Comment 4

[Makoto Kato [:m_kato]]

Attachment: Turn off Stylo on Android beta and release

Comment 5

[Makoto Kato [:m_kato]]

Attachment: Turn off Stylo on Android beta and release

Comment 6

[Makoto Kato [:m_kato]]

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).

Comment 7

[Makoto Kato [:m_kato]]

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

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(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!

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8945012 [details] [diff] [review]
Turn off Stylo on Android beta and release

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

bummer!

Comment 10

[Makoto Kato [:m_kato]]

Attachment: Part 2. Skip test_restyles.html on android

Comment 11

[Makoto Kato [:m_kato]]

I forget comment for part 2. This change is beta branch only

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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!

Comment 13

[Makoto Kato [:m_kato]]

Attachment: Part 2. Skip test_restyles.html on android. r=hiro

carry r+

Comment 14

[Makoto Kato [:m_kato]]

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

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

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)

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8945321 [details] [diff] [review]
Part 2. Skip test_restyles.html on android. r=hiro

Thanks for changing the tests too.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a27738f5d328
https://hg.mozilla.org/releases/mozilla-beta/rev/27c0ba4c8dad