[Webkit-unassigned] [Bug 116049] Add a DisplayAndPowerController class to handle IOKit Display and Power notifications

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed May 15 10:49:11 PDT 2013


https://bugs.webkit.org/show_bug.cgi?id=116049


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #201808|review?                     |review-
               Flag|                            |




--- Comment #33 from Simon Fraser (smfr) <simon.fraser at apple.com>  2013-05-15 10:47:37 PST ---
(From update of attachment 201808)
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().

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.



More information about the webkit-unassigned mailing list