[Webkit-unassigned] [Bug 88406] [WebKit] DeviceOrientation cleanup

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Hans Wennborg <hans at chromium.org>  2012-06-06 03:55:20 PST ---
(From update of attachment 145978)
Thanks very much!

Some more context: this change is a clean-up based on fishd's comments in https://bugs.webkit.org/show_bug.cgi?id=47078#c12


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

> Source/WebKit/chromium/ChangeLog:8
> +        Added a default constructor to WebDeviceOrientation.h and added set functions for each property.

Instead of "Added", maybe we should say "Make default constructor public".
Also, we should note that the reason we're doing this is because we want to remove the big 8-parameter constructor.

> Source/WebKit/chromium/public/WebDeviceOrientation.h:50
> +    WebDeviceOrientation()

Please add something like "// FIXME: Remove once Chromium is updated." above the old constructor.

> Source/WebKit/chromium/public/WebDeviceOrientation.h:51
> +        : m_isNull(true),

for initializer lists in WebKit, we put the commas in the same column as the colon, for example

    : m_isNull(true)
    , m_canProvideAlpha(false)

I realize this was wrong in the file already, but we might as well take the opportunity to fix it while we're here. (See "Other Punctuation" at http://www.webkit.org/coding/coding-style.html)

> Source/WebKit/chromium/public/WebDeviceOrientation.h:67
> +        m_isNull = isNull;

since the function only contains one line, maybe we should just write it as void setNull(bool isNull) { m_isNull = isNull; }

> Source/WebKit/chromium/public/WebDeviceOrientation.h:70
> +    void setAlpha(double alpha)

I think a blank line here would make the file easier to read. That way setAlpha, canProvideAlpha, and alpha would form its own little section, and we should do the same for beta, gamma, and absolute.

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