1479246 - Change background-color property in tests which suppose that background-color animation runs on the main-thread

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Because background-color will run on the compositor sooner or later.

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

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1479246 - Use z-index for the property running on the main thread instead of background-color.

background-color animations will run on the compositor.  So we have to change
the property in tests which suppose that background-color animation runs on the
main-thread.

The reason why we use z-index is that the property produces only
nsChangeHint_RepaintFrame, thus it's throttled and won't be unthrottled
periodically on out-of-view elements just like background-color animations.

Review commit: https://reviewboard.mozilla.org/r/260150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/260150/

Comment 2

[Boris Chiou [:boris]]

Comment on attachment 8995774 [details]
Bug 1479246 - Use z-index for the property running on the main thread instead of background-color.

https://reviewboard.mozilla.org/r/260150/#review267148

Using z-index makes sense to me, as you mentioned in the commit message. Thanks for the clear explanation. LGTM.

::: dom/animation/test/mozilla/file_restyles.html:1126
(Diff revision 1)
> -      // Make the background-color style as !important to cause an update
> +      // Make the font-size style as !important to cause an update
>        // to the cascade.
> -      // Bug 1300982: The background-color animation should be no longer
> +      // Bug 1300982: The font-size animation should be no longer
>        // running on the main thread.
> -      div.style.setProperty('background-color', '1', 'important');
> +      div.style.setProperty('font-size', '1px', 'important');

Just wonder why we change `font-size`, instead of `z-index`? It seems what we want here is to change `z-index`, an animation property running on the main thread, from non-important to important.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Boris Chiou [:boris] from comment #2)
> Comment on attachment 8995774 [details]
> Bug 1479246 - Use z-index for the property running on the main thread
> instead of background-color.
> 
> https://reviewboard.mozilla.org/r/260150/#review267148
> 
> Using z-index makes sense to me, as you mentioned in the commit message.
> Thanks for the clear explanation. LGTM.
> 
> ::: dom/animation/test/mozilla/file_restyles.html:1126
> (Diff revision 1)
> > -      // Make the background-color style as !important to cause an update
> > +      // Make the font-size style as !important to cause an update
> >        // to the cascade.
> > -      // Bug 1300982: The background-color animation should be no longer
> > +      // Bug 1300982: The font-size animation should be no longer
> >        // running on the main thread.
> > -      div.style.setProperty('background-color', '1', 'important');
> > +      div.style.setProperty('font-size', '1px', 'important');
> 
> Just wonder why we change `font-size`, instead of `z-index`? It seems what
> we want here is to change `z-index`, an animation property running on the
> main thread, from non-important to important.

Gosh!  Good catch!  Thanks for the review!  I did initially use 'font-size' here but after that I noticed it's not suitable for this bug, since it produces reflow change hint, so I decided to use 'z-index'.  But!  forgot to revert the change here.

Thanks!

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8995774 [details]
Bug 1479246 - Use z-index for the property running on the main thread instead of background-color.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/260150/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec96df034b4e
Use z-index for the property running on the main thread instead of background-color. r=boris

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ec96df034b4e