Bug 116049 – Add a DisplayNotificationObserver class to handle Display notifications

• Component: WebCore Misc.
• Status: NEW

[Roger Fong]

This requires refactoring out the PowerObserver code that used to live in SharedTimerMac.mm as well.

[Roger Fong]

Created attachment 201661 [details]
patch

patch

[Ryosuke Niwa]

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()?

[Roger Fong]

> 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.

[Ryosuke Niwa]

(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.

[Roger Fong]

(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.

[Ryosuke Niwa]

(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.

[Roger Fong]

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.

[EFL EWS Bot]

Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/476326

[Early Warning System Bot]

Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/473325

[kov's GTK+ EWS bot]

Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/472395

[Build Bot]

Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/472397

[Build Bot]

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

[Build Bot]

Comment on attachment 201778 [details]
patch

Attachment 201778 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/478278

[EFL EWS Bot]

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

[Early Warning System Bot]

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

[Roger Fong]

Created attachment 201794 [details]
patch

Fix builds

[EFL EWS Bot]

Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/470444

[Early Warning System Bot]

Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/476403

[EFL EWS Bot]

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

[Early Warning System Bot]

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

[kov's GTK+ EWS bot]

Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/468747

[Build Bot]

Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/471443

[Build Bot]

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

[Build Bot]

Comment on attachment 201794 [details]
patch

Attachment 201794 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/478343

[Roger Fong]

Created attachment 201805 [details]
patch

[Early Warning System Bot]

Comment on attachment 201805 [details]
patch

Attachment 201805 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/466891

[Early Warning System Bot]

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

[Roger Fong]

Created attachment 201808 [details]
patch

[Early Warning System Bot]

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

[Early Warning System Bot]

Comment on attachment 201808 [details]
patch

Attachment 201808 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/468793

[kov's GTK+ EWS bot]

Comment on attachment 201808 [details]
patch

Attachment 201808 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/466921

[Build Bot]

Comment on attachment 201808 [details]
patch

Attachment 201808 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/477406

[Simon Fraser (smfr)]

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().

[Roger Fong]

> > 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.

[Roger Fong]

> 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...) :/

[Roger Fong]

Created attachment 201887 [details]
patch

This one's totally going to build...

[Roger Fong]

Created attachment 201907 [details]
one patch to rule them all

fixed changelog

[Tim Horton]

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?

[Anders Carlsson]

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()"?

[Roger Fong]

Created attachment 201979 [details]
patch...

[EFL EWS Bot]

Comment on attachment 201979 [details]
patch...

Attachment 201979 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/489033

[EFL EWS Bot]

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

[Roger Fong]

Created attachment 201983 [details]
fix elf build...i hope

[Roger Fong]

(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.

[Ryosuke Niwa]

(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.