Stop Blinking the Text Caret when it isn't visible

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

Firefox OS
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mchang, Assigned: mchang)

Tracking

unspecified
2.1 S4 (12sep)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

4 years ago
+++ 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.
(Assignee)

Comment 1

4 years ago
Created attachment 8484670 [details] [diff] [review]
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!
Attachment #8484670 - Flags: review?(roc)
(Assignee)

Comment 2

4 years ago
Created attachment 8484674 [details] [diff] [review]
caret.patch

Fixed a whitespace.
Attachment #8484670 - Attachment is obsolete: true
Attachment #8484670 - Flags: review?(roc)
Attachment #8484674 - Flags: review?(roc)
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.
Attachment #8484674 - Flags: review?(roc) → review-
(Assignee)

Comment 4

4 years ago
Created attachment 8485050 [details] [diff] [review]
Bug 1063265: Don't blick the caret unless it is visible

Updated with feedback from comment 3. Thanks!
Attachment #8484674 - Attachment is obsolete: true
Attachment #8485050 - Flags: review?(roc)
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 !!!
Attachment #8485050 - Flags: review?(roc) → review-
(Assignee)

Comment 6

4 years ago
Created attachment 8485194 [details] [diff] [review]
caret.patch

Ahh fail, sorry!
Attachment #8485050 - Attachment is obsolete: true
Attachment #8485194 - Flags: review?(roc)
Comment on attachment 8485194 [details] [diff] [review]
caret.patch

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

:-)
Attachment #8485194 - Flags: review?(roc) → review+
(Assignee)

Comment 8

4 years ago
Created attachment 8485436 [details] [diff] [review]
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
Attachment #8485194 - Attachment is obsolete: true
Attachment #8485436 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 9

4 years ago
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
Attachment #8485436 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

4 years ago
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → affected
https://hg.mozilla.org/mozilla-central/rev/7bd750b8352e
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
status-b2g-v2.2: affected → fixed

Updated

4 years ago
Attachment #8485436 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/b73932b2be07
status-b2g-v2.1: affected → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
status-firefox35: --- → fixed
Target Milestone: --- → 2.1 S4 (12sep)
You need to log in before you can comment on or make changes to this bug.