[webkit-reviews] review denied: [Bug 116049] Add a DisplayAndPowerController class to handle IOKit Display and Power notifications : [Attachment 201808] patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Roger Fong
<roger_fong at apple.com>'s request for review:
Bug 116049: Add a DisplayAndPowerController class to handle IOKit Display and
Power notifications
https://bugs.webkit.org/show_bug.cgi?id=116049

Attachment 201808: patch
https://bugs.webkit.org/attachment.cgi?id=201808&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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().