Bug 88406 – [Chromium] DeviceOrientation cleanup

• Component: New Bugs
• Status: RESOLVED FIXED

[Amy Ousterhout]

[WebKit] DeviceOrientation cleanup

[Amy Ousterhout]

Created attachment 145978 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Hans Wennborg]

Comment on attachment 145978 [details]
Patch

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.

[Amy Ousterhout]

Created attachment 145989 [details]
Patch

[Amy Ousterhout]

Thanks for the review. I made the changes you suggested, and also changed the name of the bug to have "[Chromium]" instead of "[WebKit]".

[Hans Wennborg]

Comment on attachment 145989 [details]
Patch

LGTM, thanks!

As noted in comment #2, we'll need one of the Chromium public API gatekeepers to approve this.

[Kent Tamura]

Comment on attachment 145989 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 145989 [details]
Patch

Clearing flags on attachment: 145989

Committed r119661: <http://trac.webkit.org/changeset/119661>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.