Bug 88406 - [Chromium] DeviceOrientation cleanup
Summary: [Chromium] DeviceOrientation cleanup
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-06 03:14 PDT by Amy Ousterhout
Modified: 2012-06-06 18:36 PDT (History)
8 users (show)

See Also:


Attachments
Patch (5.43 KB, patch)
2012-06-06 03:30 PDT, Amy Ousterhout
no flags Details | Formatted Diff | Diff
Patch (6.35 KB, patch)
2012-06-06 04:28 PDT, Amy Ousterhout
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Amy Ousterhout 2012-06-06 03:14:16 PDT
[WebKit] DeviceOrientation cleanup
Comment 1 Amy Ousterhout 2012-06-06 03:30:34 PDT
Created attachment 145978 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-06 03:34:17 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 Hans Wennborg 2012-06-06 03:55:20 PDT
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.
Comment 4 Amy Ousterhout 2012-06-06 04:28:26 PDT
Created attachment 145989 [details]
Patch
Comment 5 Amy Ousterhout 2012-06-06 04:29:50 PDT
Thanks for the review. I made the changes you suggested, and also changed the name of the bug to have "[Chromium]" instead of "[WebKit]".
Comment 6 Hans Wennborg 2012-06-06 04:36:50 PDT
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.
Comment 7 Kent Tamura 2012-06-06 17:34:31 PDT
Comment on attachment 145989 [details]
Patch

ok
Comment 8 WebKit Review Bot 2012-06-06 18:36:32 PDT
Comment on attachment 145989 [details]
Patch

Clearing flags on attachment: 145989

Committed r119661: <http://trac.webkit.org/changeset/119661>
Comment 9 WebKit Review Bot 2012-06-06 18:36:39 PDT
All reviewed patches have been landed.  Closing bug.