Bug 115763 - Make caret not blink when entering dimmed display state before going to display sleep
Summary: Make caret not blink when entering dimmed display state before going to displ...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Roger Fong
URL:
Keywords: InRadar
Depends on: 116049
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-07 15:12 PDT by Roger Fong
Modified: 2013-10-02 12:29 PDT (History)
12 users (show)

See Also:


Attachments
patch (7.06 KB, patch)
2013-05-07 17:05 PDT, Roger Fong
simon.fraser: review-
Details | Formatted Diff | Diff
patch (22.27 KB, patch)
2013-05-12 02:42 PDT, Roger Fong
rniwa: review-
eflews.bot: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roger Fong 2013-05-07 15:12:30 PDT
Make the caret not blink on mac when the display goes dim (which happens about 15 seconds before the display goes to sleep).
Comment 1 Roger Fong 2013-05-07 16:48:25 PDT
<rdar://problem/13521485>
Comment 2 Roger Fong 2013-05-07 17:05:48 PDT
Created attachment 200999 [details]
patch
Comment 3 Simon Fraser (smfr) 2013-05-07 17:10:31 PDT
Comment on attachment 200999 [details]
patch

I don't think RenderTheme is the right place for the IOKit code; we might even want it in WebKit/WebKit2. WebCore shouldn't know about screen dimming; it should just be told that the caret should stop blinking.
Comment 4 Tim Horton 2013-05-07 17:11:44 PDT
Comment on attachment 200999 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200999&action=review

> Source/WebCore/ChangeLog:9
> +        Update RenderTheme to keep track of the receive display sleep notifications and keep track of the display's state.

keep track of the receive?

> Source/WebCore/editing/FrameSelection.cpp:1849
> +    if (m_frame->view()->renderView()->theme()->isScreenDimmed() || (isCaretBlinkingSuspended() && caretPaint))

Can any of these ever be null?

> Source/WebCore/rendering/RenderTheme.h:350
> +    static bool m_screenDimmed;

I think that statics use s instead of m or something but you should look it up. Also maybe this should just be in the RenderThemeMac file and not in this class at all? But I dunno.

> Source/WebCore/rendering/RenderThemeMac.h:262
> +    io_service_t m_displayWrangler;

This is pretty bizarre to be in RenderTheme*.

> Source/WebCore/rendering/RenderThemeMac.mm:2316
> +        case kIOMessageDeviceWillPowerOff:
> +            m_screenDimmed = true;
> +            break;
> +            
> +        case kIOMessageDeviceHasPoweredOn:
> +            m_screenDimmed = false;
> +            break;

This is not indented correctly. Does this really need to be a switch?
Comment 5 Roger Fong 2013-05-10 17:05:13 PDT
I'm going to move PowerObserver code into a class that handles both Display/Power notifications (the both use IOKit) (calling it DisplayAndPowerController until I or someone else comes up with a better name).

This requires me however to also modify the SharedTimerMac.mm (I'm creating a SharedTimerMac.h as well).

One piece that I still haven't figured out is where I'm going to instantiate said DisplayAndPowerController.
Thoughts?
Comment 6 Roger Fong 2013-05-12 02:42:04 PDT
Created attachment 201484 [details]
patch

I may want to split this into two patches.
One that adds the DisplayAndPowerController class (and gets rid of old PowerObserver stuff)
And then another that actually has FrameSelection check the screen state.
Comment 7 EFL EWS Bot 2013-05-12 02:49:49 PDT
Comment on attachment 201484 [details]
patch

Attachment 201484 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/385415
Comment 8 EFL EWS Bot 2013-05-12 02:51:24 PDT
Comment on attachment 201484 [details]
patch

Attachment 201484 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/404368
Comment 9 Early Warning System Bot 2013-05-12 03:00:41 PDT
Comment on attachment 201484 [details]
patch

Attachment 201484 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/404370
Comment 10 Early Warning System Bot 2013-05-12 03:02:15 PDT
Comment on attachment 201484 [details]
patch

Attachment 201484 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/395428
Comment 11 Build Bot 2013-05-12 03:13:31 PDT
Comment on attachment 201484 [details]
patch

Attachment 201484 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/460343
Comment 12 Build Bot 2013-05-12 03:18:48 PDT
Comment on attachment 201484 [details]
patch

Attachment 201484 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/286334
Comment 13 kov's GTK+ EWS bot 2013-05-12 04:03:47 PDT
Comment on attachment 201484 [details]
patch

Attachment 201484 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/417719
Comment 14 Ryosuke Niwa 2013-05-12 10:22:09 PDT
(In reply to comment #6)
> Created an attachment (id=201484) [details]
> patch
> 
> I may want to split this into two patches.
> One that adds the DisplayAndPowerController class (and gets rid of old PowerObserver stuff)
> And then another that actually has FrameSelection check the screen state.

Please do.
Comment 15 Build Bot 2013-05-12 15:05:45 PDT
Comment on attachment 201484 [details]
patch

Attachment 201484 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/417862
Comment 16 Ryosuke Niwa 2013-05-13 09:32:04 PDT
Comment on attachment 201484 [details]
patch

Please split the patch to add DisplayAndPowerController.