Closed Bug 1217230 Opened 4 years ago Closed 4 years ago

nsRefreshDriver::mNeedToRecomputeVisibility does not work as expected.

Categories

(Core :: Layout: Images, Video, and HTML Frames, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- wontfix
firefox42 + wontfix
firefox43 + fixed
firefox44 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(1 file)

From bug 1169880#1:

 Here's the patch. We schedule an image visibility update in the refresh driver if:

 - A layout or style flush has occurred.

But I noticed in the current implementation the flag is always set to true whenever normal Tick process runs.
Move 'mNeedToRecomputeVisibility = true' into style or layout observer's loop.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d711fa66e00
Attachment #8677152 - Flags: review?(seth)
Comment on attachment 8677152 [details] [diff] [review]
Set mNeedToRecomputeVisibility true only when style or layout flush.

Good find!

I'm surprised this didn't cause any regressions in talos.

I'm stealing review here, hope that's okay.

We should uplift this so we minimize the window that this regression is shipped to users.
Attachment #8677152 - Flags: review?(seth) → review+
Assignee: nobody → hiikezoe
[Tracking Requested - why for this release]:
I think this will make us walk the entire frame tree on every refresh driver tick, and it's completely unnecessary. The fix is easy and simple. We shouldn't ship this regression any longer than needed.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1)

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d711fa66e00

There was a new orange (filed as bug 1217314), but I am convinced it's not related to this patch.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b8ca53e9a3a0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Timothy, we are going to build 42 rc 1 today. So, it is likely that 42 will ship wit this regression. Do you know when it was introduced ? (and if 41 is affected or not).
What is the impact to the user?

Hiroyuki, could you fill the uplift request? (at least to aurora) Thanks
Flags: needinfo?(tnikkel)
Flags: needinfo?(hiikezoe)
(In reply to Sylvestre Ledru [:sylvestre] from comment #7)
> Timothy, we are going to build 42 rc 1 today. So, it is likely that 42 will
> ship wit this regression. Do you know when it was introduced ? (and if 41 is
> affected or not).

I think it was introduced by bug 1169880. So that would mean that it's in 41. So if this is very late in the cycle we can probably wait since it doesn't seem to have caused any visible problems that have been reported elsewhere.

> What is the impact to the user?

Slower performance in general. But the fact that talos didn't catch this makes me wonder how visible it is in practice.
Flags: needinfo?(tnikkel)
OK, thanks. Not taking the patch for 42 then.
Comment on attachment 8677152 [details] [diff] [review]
Set mNeedToRecomputeVisibility true only when style or layout flush.

Approval Request Comment
[Feature/regressing bug #]: 1169880
[User impact if declined]: More power is consumed on some sort of pages. (I guess the pages might have low frequency animations)
[Describe test coverage new/current, TreeHerder]: Confirmed with a test case attached in bug 1168985 (which is a duplicate as bug 1169880). Every images are inserted every 100ms instead of every tick (16.6ms usualy).
[Risks and why]: Low because this fix is very simple. 
[String/UUID change made/needed]: None
Flags: needinfo?(hiikezoe)
Attachment #8677152 - Flags: approval-mozilla-aurora?
Comment on attachment 8677152 [details] [diff] [review]
Set mNeedToRecomputeVisibility true only when style or layout flush.

Power/perf regression fix. New tests included. OK to uplift to aurora.
Attachment #8677152 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.