1063265 - Stop Blinking the Text Caret when it isn't visible

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Mason Chang [Inactive] [:mchang]]

+++ This bug was initially created as a clone of Bug #1062119 +++

The caret is blinking even when the screen is off, causing the refresh driver to tick and paint even though it isn't visible. Change the behavior so that the caret doesn't tick or request a paint unless the caret is visible.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: caret.patch

On nsCaret::ResetBlinking, if the caret isn't visible, we return early. We only schedule paints if the caret is visible now. I see most of the time ResetBlinking is followed by SchedulePaint, but we still sometimes SchedulePaint without ResetBlinking. Just a first stab. Thanks!

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: caret.patch

Fixed a whitespace.

Comment 3

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

Comment on attachment 8484674 [details] [diff] [review]
caret.patch

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

diff -U8 please

::: layout/base/nsCaret.cpp
@@ +403,5 @@
>      return;
>    }
> +  if (!IsVisible()) {
> +    return;
> +  }

We shouldn't do this. If the caret changes from visible to not visible we need to schedule a paint.

@@ +553,5 @@
>  
> +  if (!IsVisible()) {
> +    StopBlinking();
> +    return;
> +  }

Let's just check mVisible here. IsVisible might return false because of IsMenuPopupHidingCaret() and in that case we should probably keep running the timer, since we might not get notified when IsMenuPopupHidingCaret() becomes false again.

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Bug 1063265: Don't blick the caret unless it is visible

Updated with feedback from comment 3. Thanks!

Comment 5

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

Comment on attachment 8485050 [details] [diff] [review]
Bug 1063265: Don't blick the caret unless it is visible

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

::: layout/base/nsCaret.cpp
@@ +547,5 @@
>  void nsCaret::ResetBlinking()
>  {
>    mIsBlinkOn = true;
>  
> +  if (mReadOnly || mVisible) {

!mVisible !!!

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: caret.patch

Ahh fail, sorry!

Comment 7

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

Comment on attachment 8485194 [details] [diff] [review]
caret.patch

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

:-)

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Bug 1063265: Don't blick the caret unless it is visible. r=roc

Carrying r+ forward. Updated with proper commit message and author. Successful try: https://tbpl.mozilla.org/?tree=Try&rev=23e1167ca8e5

Comment 9

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8485436 [details] [diff] [review]
Bug 1063265: Don't blick the caret unless it is visible. r=roc

Approval Request Comment
[Feature/regressing bug #]: Bug 1062119, continuous CPU usage while the display is off.
[User impact if declined]: We will waste CPU usage even while the screen is off, thus burning battery. Battery with the screen currently lasts less than 24 hours without this patch.
[Describe test coverage new/current, TBPL]: Manual testing to see if CPU is still being used while the screen is off. Turned power on / off 10 times and waited 30 seconds to ensure CPU is idle.
[Risks and why]: Low risk - Previously, we were painting the text carat even though it wasn't actually being displayed. This patch stops painting the text carat when it is not displayed to the user.
[String/UUID change made/needed]: None

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7bd750b8352e

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7bd750b8352e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b73932b2be07