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

[bugzilla-daemon at webkit.org]

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





--- Comment #2 from Ryosuke Niwa <rniwa at webkit.org>  2013-05-13 19:40:04 PST ---
(From update of attachment 201661)
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()?

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