Bug 116049

Summary: Add a DisplayNotificationObserver class to handle Display notifications
Product: WebKit Reporter: Roger Fong <roger_fong>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Normal CC: buildbot, commit-queue, eflews.bot, gtk-ews, gyuyoung.kim, gyuyoung.kim, philn, rakuco, rego+ews, rniwa, roger_fong, simon.fraser, thorton, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 115763    
Attachments:
Description Flags
patch
none
patch
eflews.bot: commit-queue-
patch
eflews.bot: commit-queue-
patch
webkit-ews: commit-queue-
patch
simon.fraser: review-, webkit-ews: commit-queue-
patch
none
one patch to rule them all
none
patch...
eflews.bot: commit-queue-
fix elf build...i hope none

Description Roger Fong 2013-05-13 11:19:56 PDT
This requires refactoring out the PowerObserver code that used to live in SharedTimerMac.mm as well.
Comment 1 Roger Fong 2013-05-13 18:38:49 PDT
Created attachment 201661 [details]
patch

patch
Comment 2 Ryosuke Niwa 2013-05-13 19:40:04 PDT
Comment on attachment 201661 [details]
patch

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

> Source/WebCore/platform/DisplayAndPowerController.h:39
> +class DisplayAndPowerController {
> +    WTF_MAKE_NONCOPYABLE(DisplayAndPowerController);
> +private:
> +    DisplayAndPowerController();
> +    ~DisplayAndPowerController();

I’d call this PowerSavingController instead.

> Source/WebCore/platform/DisplayAndPowerController.h:47
> +    bool s_screenDimmed;
> +    void platformInit();
> +#if PLATFORM(MAC)
> +    OwnPtr<struct DisplayAndPowerControllerInternals> m_internals;
> +    static void didReceiveSystemPowerNotification(void* context, io_service_t, uint32_t messageType, void* messageArgument);
> +    static void didReceiveDisplayStateNotification(void* context, io_service_t, uint32_t messageType, void* messageArgument);
> +    void didReceiveSystemPowerNotification(uint32_t messageType, void* messageArgument);
> +    void didReceiveDisplayStateNotification(uint32_t messageType, void* messageArgument);

Why do these member functions and variables need to be in this class if they were private?
Can’t they just be on DisplayAndPowerControllerInternals instead?

> Source/WebCore/platform/DisplayAndPowerController.h:49
> +#endif
> +public:

Nit: You need a blank line between #endif and public.

> Source/WebCore/platform/DisplayAndPowerController.h:51
> +    bool isScreenDimmed() { return s_screenDimmed; }
> +    static DisplayAndPowerController* sharedController();

We normally put public member functions first. isScreenDimmed should have const modifier.

> Source/WebCore/platform/mac/SharedTimerMac.mm:85
> +    //Makes sure that a DisplayAndPowerController has been created
> +    DisplayAndPowerController::sharedController();

I’m not sure it’s such a good idea for a function named sharedController to have a side effect…
Instead of adding a comment above, we should rename it to be more descriptive.
How about something like init() or createSharedController()?
Comment 3 Roger Fong 2013-05-13 21:16:22 PDT
> I’d call this PowerSavingController instead.

Hmm the display notification functionality is for power saving but the power notifications functionality is for shared timer stuff...plus I feel like the class can be used for more than just PowerSaving functionality in the future right?
 
> Why do these member functions and variables need to be in this class if they were private?
> Can’t they just be on DisplayAndPowerControllerInternals instead?

They access private variables of DisplayAndPowerController so I stuck them in the class to get around that.

> 
> I’m not sure it’s such a good idea for a function named sharedController to have a side effect…
> Instead of adding a comment above, we should rename it to be more descriptive.
> How about something like init() or createSharedController()?

Hmm okay, I was just following DisplayRefreshMonitorManager::sharedManager(), but perhaps that too is poorly named.
Comment 4 Ryosuke Niwa 2013-05-13 22:15:09 PDT
(In reply to comment #3)
> > I’d call this PowerSavingController instead.
> 
> Hmm the display notification functionality is for power saving but the power notifications functionality is for shared timer stuff...plus I feel like the class can be used for more than just PowerSaving functionality in the future right?

It seems random that we have a controller for Display and Power. In general, we shouldn't name something in the anticipation of some new functionality in the future. We can rename it later if that day comes.

> > Why do these member functions and variables need to be in this class if they were private?
> > Can’t they just be on DisplayAndPowerControllerInternals instead?
> 
> They access private variables of DisplayAndPowerController so I stuck them in the class to get around that.

Looking at the implementation, why does DisplayAndPowerController need any variable at all? It seems like we should move everything to DisplayAndPowerControllerInternals and just expose initDisplayAndPowerController() instead.

The controller doesn't have any API whatsoever.
Comment 5 Roger Fong 2013-05-14 01:37:55 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > > I’d call this PowerSavingController instead.
> > 
> > Hmm the display notification functionality is for power saving but the power notifications functionality is for shared timer stuff...plus I feel like the class can be used for more than just PowerSaving functionality in the future right?

True, although there's still the problem that currently the Power notifications aspect that is currently implemented is not used for Power Saving purposes.

 
> Looking at the implementation, why does DisplayAndPowerController need any variable at all? It seems like we should move everything to DisplayAndPowerControllerInternals and just expose initDisplayAndPowerController() instead.
> 
> The controller doesn't have any API whatsoever.

I wanted to leave s_screenDimmed as a part of DisplayAndPowerController because I felt natural that a display controller should keep the current state of the display. In addition, FrameSelection.cpp will be calling isScreenDimmed() to determine whether or not to invalidate the caret rect. I suppose then that I could leave the implementation of isScreenDimmed up to the platform as well instead of just returning s_screenDimmed but that seems rather odd to me.
Comment 6 Ryosuke Niwa 2013-05-14 08:18:18 PDT
(In reply to comment #5)
>
> I wanted to leave s_screenDimmed as a part of DisplayAndPowerController because I felt natural that a display controller should keep the current state of the display. In addition, FrameSelection.cpp will be calling isScreenDimmed() to determine whether or not to invalidate the caret rect. I suppose then that I could leave the implementation of isScreenDimmed up to the platform as well instead of just returning s_screenDimmed but that seems rather odd to me.

But everything else can be moved into the internals, right?   Maybe DisplayController needs to be split into itws own class/file.
Comment 7 Roger Fong 2013-05-14 18:43:01 PDT
Created attachment 201778 [details]
patch

I've made some of the aforementioned changes.
In addition I've gotten rid of Power notifications and decided to leave that where it is.
It was pointed out other platforms might not get their power and display notifications from the same library like we do with IOKit so there's no reason to really have them in the same place anyways.
The resulting class in a singleton DisplayNotificationObserver class.

I also believe that keeping track of the display state via s_screenDimmed and having other classes (like FrameSelection) query said display state is the cleanest way to do things for something this lightweight and specific in usage. Plus now there's only two things in the IF PLATFORM(MAC) after getting rid of that power stuff.
Comment 8 EFL EWS Bot 2013-05-14 18:47:22 PDT
Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/476326
Comment 9 Early Warning System Bot 2013-05-14 18:48:11 PDT
Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/473325
Comment 10 kov's GTK+ EWS bot 2013-05-14 18:57:13 PDT
Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/472395
Comment 11 Build Bot 2013-05-14 18:58:30 PDT
Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/472397
Comment 12 Build Bot 2013-05-14 19:07:50 PDT
Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/465881
Comment 13 Build Bot 2013-05-14 19:41:14 PDT
Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/478278
Comment 14 EFL EWS Bot 2013-05-14 19:47:26 PDT
Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/467762
Comment 15 Early Warning System Bot 2013-05-14 19:50:48 PDT
Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/475353
Comment 16 Roger Fong 2013-05-14 23:12:23 PDT
Created attachment 201794 [details]
patch

Fix builds
Comment 17 EFL EWS Bot 2013-05-14 23:19:49 PDT
Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/470444
Comment 18 Early Warning System Bot 2013-05-14 23:20:53 PDT
Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/476403
Comment 19 EFL EWS Bot 2013-05-14 23:21:24 PDT
Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/470446
Comment 20 Early Warning System Bot 2013-05-14 23:23:04 PDT
Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/466859
Comment 21 kov's GTK+ EWS bot 2013-05-14 23:24:38 PDT
Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/468747
Comment 22 Build Bot 2013-05-14 23:28:21 PDT
Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/471443
Comment 23 Build Bot 2013-05-14 23:38:05 PDT
Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/477340
Comment 24 Build Bot 2013-05-15 00:24:00 PDT
Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/478343
Comment 25 Roger Fong 2013-05-15 01:49:12 PDT
Created attachment 201805 [details]
patch
Comment 26 Early Warning System Bot 2013-05-15 01:55:19 PDT
Comment on attachment 201805 [details]
patch

Attachment 201805 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/466891
Comment 27 Early Warning System Bot 2013-05-15 01:55:31 PDT
Comment on attachment 201805 [details]
patch

Attachment 201805 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/468783
Comment 28 Roger Fong 2013-05-15 02:17:57 PDT
Created attachment 201808 [details]
patch
Comment 29 Early Warning System Bot 2013-05-15 02:22:45 PDT
Comment on attachment 201808 [details]
patch

Attachment 201808 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/477387
Comment 30 Early Warning System Bot 2013-05-15 02:23:16 PDT
Comment on attachment 201808 [details]
patch

Attachment 201808 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/468793
Comment 31 kov's GTK+ EWS bot 2013-05-15 03:22:38 PDT
Comment on attachment 201808 [details]
patch

Attachment 201808 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/466921
Comment 32 Build Bot 2013-05-15 03:41:47 PDT
Comment on attachment 201808 [details]
patch

Attachment 201808 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/477406
Comment 33 Simon Fraser (smfr) 2013-05-15 10:47:37 PDT
Comment on attachment 201808 [details]
patch

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

> Source/WebCore/platform/DisplayNotificationObserver.cpp:35
> +DisplayNotificationObserver::DisplayNotificationObserver() { }
> +    
> +DisplayNotificationObserver::~DisplayNotificationObserver() { }
> +
> +void DisplayNotificationObserver::platformInit() { }

Please put the braces on their own lines; we only do this style in headers.

> Source/WebCore/platform/DisplayNotificationObserver.h:31
> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassOwnPtr.h>

You're not using these two headers.

> Source/WebCore/platform/DisplayNotificationObserver.h:43
> +    bool s_screenDimmed;

Why s_screenDimmed? This is not static, so it should be m_screenDimmed.

> Source/WebCore/platform/mac/DisplayNotificationObserver.mm:71
> +    m_internals = adoptPtr(new DisplayNotificationObserverInternals());
> +
> +    m_internals->m_displayWrangler = IOServiceGetMatchingService(NULL, IOServiceNameMatching("IODisplayWrangler"));
> +    if (!m_internals->m_displayWrangler)
> +        return;
> +    
> +    m_internals->m_displayPort = IONotificationPortCreate(NULL);
> +    if (!m_internals->m_displayPort)
> +        return;
> +
> +    IOServiceAddInterestNotification(m_internals->m_displayPort, m_internals->m_displayWrangler, kIOGeneralInterest, didReceiveDisplayStateNotification, NULL, &m_internals->m_displayReference);
> +
> +    m_internals->m_dispatchQueue = dispatch_queue_create("com.apple.WebKit.DisplayAndPowerObserver", 0);
> +    IONotificationPortSetDispatchQueue(m_internals->m_displayPort, m_internals->m_dispatchQueue);
> +}

Why doesn't this set up the power-related stuff?

> Source/WebCore/platform/mac/DisplayNotificationObserver.mm:77
> +DisplayNotificationObserver* DisplayNotificationObserver::createSharedDisplayObserver()
> +{
> +    DEFINE_STATIC_LOCAL(DisplayNotificationObserver, observer, ());
> +    return &observer;
> +}

This only does the create the first time, so I think it should just be sharedDisplayObserver()

> Source/WebCore/platform/mac/DisplayNotificationObserver.mm:90
> +    if (!m_internals->m_powerConnection)
> +        return;

You should never early return from a destructor.

> Source/WebCore/platform/mac/DisplayNotificationObserver.mm:106
> +    createSharedDisplayObserver()->didReceiveDisplayStateNotification(messageType, messageArgument);

Is the first parameter a 'context' param? Normally we'd use that to get to the target object, which would be cleaner than going through createSharedDisplayObserver().
Comment 34 Roger Fong 2013-05-15 13:29:16 PDT
> > Source/WebCore/platform/DisplayNotificationObserver.h:31
> > +#include <wtf/OwnPtr.h>
> > +#include <wtf/PassOwnPtr.h>
> 
> You're not using these two headers.

Oops, I am using OwnPtr though on m_internals.

> Why doesn't this set up the power-related stuff?

I think that's it's probably best to not having the Power and Display stuff together.
While it's true on mac that they both use IOKit, it may not be the case on any other platforms that the two are so closely knit, making the implementation awkward.

> Is the first parameter a 'context' param? Normally we'd use that to get to the target object, which would be cleaner than going through createSharedDisplayObserver().

It is! I will use that instead.
Comment 35 Roger Fong 2013-05-15 13:30:28 PDT
> While it's true on mac that they both use IOKit, it may not be the case on any other platforms that the two are so closely knit, making the implementation awkward.

That being said it looks like I forgot to get rid of some of the power stuff (like destructing it...) :/
Comment 36 Roger Fong 2013-05-15 15:05:25 PDT
Created attachment 201887 [details]
patch

This one's totally going to build...
Comment 37 Roger Fong 2013-05-15 18:35:46 PDT
Created attachment 201907 [details]
one patch to rule them all

fixed changelog
Comment 38 Tim Horton 2013-05-15 18:45:49 PDT
Comment on attachment 201907 [details]
one patch to rule them all

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

> Source/WebCore/platform/DisplayNotificationObserver.h:49
> +    OwnPtr<struct DisplayNotificationObserverInternals> m_internals;

Shouldn't you typedef the struct so that you can just use it as a naked type instead of with the struct prefix everywhere?

> Source/WebCore/platform/mac/DisplayNotificationObserver.mm:61
> +        IOObjectRelease(m_internals->m_displayWrangler);
> +        IOObjectRelease(m_internals->m_displayReference);
> +        IONotificationPortDestroy(m_internals->m_displayPort);

I think you have to test each of these for null before releasing since you seem to think any of them could be null in platformInit().

> Source/WebCore/platform/mac/DisplayNotificationObserver.mm:69
> +    m_internals->m_displayWrangler = IOServiceGetMatchingService(NULL, IOServiceNameMatching("IODisplayWrangler"));

0 instead of null blah blah blah stylebot (see style guidelines even if stylebot is being dumb)

> Source/WebCore/platform/mac/DisplayNotificationObserver.mm:73
> +    m_internals->m_displayPort = IONotificationPortCreate(NULL);

here too

> Source/WebCore/platform/mac/DisplayNotificationObserver.mm:80
> +    m_internals->m_dispatchQueue = dispatch_queue_create("com.apple.WebKit.DisplayNotificationObserver", 0);
> +    IONotificationPortSetDispatchQueue(m_internals->m_displayPort, m_internals->m_dispatchQueue);

Do you have to make your own dispatch queue? Seems like you could just use one of the shared queues, maybe? Anders would know.

> Source/WebCore/platform/mac/DisplayNotificationObserver.mm:87
> +DisplayNotificationObserver* DisplayNotificationObserver::sharedDisplayObserver()
> +{
> +    DEFINE_STATIC_LOCAL(DisplayNotificationObserver, observer, ());
> +    return &observer;
> +}

Why does this need to be in the platform specific file?
Comment 39 Anders Carlsson 2013-05-16 07:43:10 PDT
Comment on attachment 201907 [details]
one patch to rule them all

I don't see why this needs to be exposed as a DisplayNotificationObserver singleton class, couldn't the API just be "isScreenDimmed()"?
Comment 40 Roger Fong 2013-05-16 11:49:08 PDT
Created attachment 201979 [details]
patch...
Comment 41 EFL EWS Bot 2013-05-16 11:56:00 PDT
Comment on attachment 201979 [details]
patch...

Attachment 201979 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/489033
Comment 42 EFL EWS Bot 2013-05-16 11:56:26 PDT
Comment on attachment 201979 [details]
patch...

Attachment 201979 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/482499
Comment 43 Roger Fong 2013-05-16 12:46:01 PDT
Created attachment 201983 [details]
fix elf build...i hope
Comment 44 Roger Fong 2013-05-16 13:40:33 PDT
(In reply to comment #39)
> (From update of attachment 201907 [details])
> I don't see why this needs to be exposed as a DisplayNotificationObserver singleton class, couldn't the API just be "isScreenDimmed()"?

That was my very first patch, but I was told that that was not the way general WebKit way of doing things like this.
As a side note, it does make things more extensible (although I was told that the "more extensible" argument is usually not a good one as well). To me this sounds like a "to each their own" decision.
Comment 45 Ryosuke Niwa 2013-05-16 13:48:36 PDT
(In reply to comment #44)
> (In reply to comment #39)
> > (From update of attachment 201907 [details] [details])
> > I don't see why this needs to be exposed as a DisplayNotificationObserver singleton class, couldn't the API just be "isScreenDimmed()"?
> 
> That was my very first patch, but I was told that that was not the way general WebKit way of doing things like this.

I would much prefer the single function interface andersca suggested. We can use singleton internally but that's an implementation detail.  If you didn't want to pollute WebCore namespace, we can wrap it inside a namespace even.

> As a side note, it does make things more extensible (although I was told that the "more extensible" argument is usually not a good one as well).

We should make things more extensible when it needs to be. It's not like this is a public interface we want to expose somewhere.