526715 - Text blinking timer does not get suspended when window on background

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Ilkka Otsala]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.14) Gecko/2009090216 Ubuntu/8.04 (hardy) Firefox/3.0.14
Build Identifier: microb-engine (20091016-1.9.2-4)

If a web page contains blinking content: <blink>blinking text</blink>, the blinking timer doesn't get suspended when browser window gets background. This affects to power management.

This was found from microb-engine when enhancing power saving.

Reproducible: Always

Steps to Reproduce:
1.Open a page containing some blinking content: <blink>blinking text</blink>
2.Minimize the page or place the browser window on background.

Actual Results:  
Blinking timer is not stopped, nsBlinkTimer::Notify gets called every 250ms.

Expected Results:  
Blinking timer should be stopped when browser window on background or minimized, or device on power saving mode.

Comment 1

[Ilkka Otsala]

Added bug #446418 to blocker list and Romaxa to cc list.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

My plan was to took up this sort of suppression in nsRefreshDriver and then hook up all timers like this to nsRefreshDriver.  nsRefreshDriver might be overkill for (text and caret) blink timers, though.

Comment 3

[Ilkka Otsala]

Attachment: Fix proposal

Issue fixed so that, when suspending animations, at same phase go through all frames and stop blinking timer for text frame if such timer exists.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

can you hook it up to nsRefreshDriver like dbaron suggested?

Comment 5

[Ilkka Otsala]

The problem with nsRefreshDriver is that Microb-Engine uses 1.9.2 version, and the nsRefreshDriver is introduced in 1.9.3. 

So the cannot do that there. Fennec browser is also using 1.9.2 version, so this patch is also an improvement for power management on that branch.

Comment 6

[Ilkka Otsala]

This patch needs to be reviewed for 1.9.2 branch, since nsRefreshDriver is not available on 1.9.2. 

So, could you Robert please go through the patch that we could integrate that on MicroB-engine.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm quite shocked that there are enough pages using 'blink' that this matters to you.

+  // go through all children of current frame if exists:
+  nsIFrame* childFrame = aFrame->GetFirstChild(nsnull);

You need to iterate through all child lists, not just the main child list, using GetAdditionalListName.

But why can't you do all this through nsTextBlinker, which has a list of all the frames that are blinking?

Also I don't think we want this on our 1.9.2 branch. Why don't you just do whatever you want on your 1.9.2 branch and fix this properly on trunk using nsRefreshDriver?

Comment 8

[Ilkka Otsala]

Attachment: Fix proposal V2

>You need to iterate through all child lists, not just the main child list,
>using GetAdditionalListName.
New patch contains this enhancement.

>But why can't you do all this through nsTextBlinker, which has a list of all
>the frames that are blinking?
nsTextBlinker can be accessed only via nsTextFrame, and the suspension starts from nsPresContext::SetImageAnimationModeInternal

>Also I don't think we want this on our 1.9.2 branch. Why don't you just do
>whatever you want on your 1.9.2 branch and fix this properly on trunk using
>nsRefreshDriver?
Yes, this fix will be pushed to our microb-engine branch, could you please recheck the new patch since you know the area and probably could give some feedback?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #8)
> >But why can't you do all this through nsTextBlinker, which has a list of all
> >the frames that are blinking?
> nsTextBlinker can be accessed only via nsTextFrame, and the suspension starts
> from nsPresContext::SetImageAnimationModeInternal

One option would be store nsTextBlinker in nsPresContext and create an nsPresContext method to access it from nsTextFrame.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 414705 [details] [diff] [review]
Fix proposal V2

see comment #9

Comment 11

[Christian Ascheberg]

Support for blink effect was dropped in bug 857820.