Bug 88663 – Renamed DeviceOrientation to DeviceOrientationData

• Component: New Bugs
• Status: RESOLVED FIXED

[Amy Ousterhout]

Renamed DeviceOrientation to DeviceOrientationData

[Amy Ousterhout]

Created attachment 146589 [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.

[WebKit Review Bot]

Attachment 146589 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit/chromium/src/WebDeviceOrientationController.cpp:38:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 146589 [details]
Patch

Attachment 146589 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12924353

[Build Bot]

Comment on attachment 146589 [details]
Patch

Attachment 146589 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12922539

[Early Warning System Bot]

Comment on attachment 146589 [details]
Patch

Attachment 146589 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12924355

[Alexey Proskuryakov]

What's the reason for this change? In OOP, classes encapsulate both data and behaviors, so there needs to be a specific reason to introduce a "Data" class.

[Adam Barth]

Comment on attachment 146589 [details]
Patch

I have the same question as Alexey.  This patch doesn't explain why we should make this change.  (I don't have an opinion about whether we should make the change because I don't understand the motiviation.)

[Hans Wennborg]

(In reply to comment #7)
> What's the reason for this change? In OOP, classes encapsulate both data and behaviors, so there needs to be a specific reason to introduce a "Data" class.

(In reply to comment #8)
> (From update of attachment 146589 [details])
> I have the same question as Alexey.  This patch doesn't explain why we should make this change.  (I don't have an opinion about whether we should make the change because I don't understand the motiviation.)

The motivation is to match DeviceMotionData, which we think has a better name.

We'll update the description (and make the EWS bots green) next week.

[Amy Ousterhout]

Created attachment 146822 [details]
Patch

[Build Bot]

Comment on attachment 146822 [details]
Patch

Attachment 146822 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12942220

[Amy Ousterhout]

Created attachment 146841 [details]
Patch

[Build Bot]

Comment on attachment 146841 [details]
Patch

Attachment 146841 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12938405

[Amy Ousterhout]

Created attachment 146863 [details]
Patch

[Hans Wennborg]

Comment on attachment 146863 [details]
Patch

Looks great to me! Thanks.

[Adam Barth]

Comment on attachment 146863 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        Udated all files that use DeviceOrientation.

Udated -> Updated

[Adam Barth]

I guess I don't understand why the new name is better so I'll let someone else review this patch.

[Alexey Proskuryakov]

Ditto. This doesn't look like an improvement to me.

[Adam Barth]

(I should also say that I don't have a strong opinion here.)

[Alexey Proskuryakov]

Seeing that there is also a "DeviceOrientationController" class, the whole subsystem likely needs a rewrite anyway.

[Hans Wennborg]

The reason for the patch is to match the DeviceMotion implementation. DeviceOrientation and DeviceMotion are part of the same spec, and work in very similar ways.

Since the DeviceMotion bits were implemented at a later time, the two implementations are slightly different.

We think the code would be made more clear by making the two implementations more consistent.

In the choice between the two names DeviceOrientation and DeviceOrientationData, we think the latter is more self-explanatory. The name DeviceOrientation could just as well be the name of an interface for interacting with the DeviceOrientation API. DeviceOrientationData on the other hand, makes it clear (at least to me) that it's a class holding the raw data.


Hopefully this makes the motivation clear. It would be great if someone would like to review this.

The DeviceMotionData class was added in Bug 42865. Dean, maybe you could comment on the naming?

[Steve Block]

Comment on attachment 146863 [details]
Patch

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

I don't have a strong opinion between DeviceOrientation vs DeviceOrientationData, but agree that consistency is a good idea.

> Source/WebCore/dom/DeviceOrientationData.cpp:1
> +/*

This might just be a display problem with the review tool, but I'm concerned that this isn't showing up as a rename+modify of DeviceOrientation.cpp, as we want to retain version history across the rename. Does it look OK on your client?

[Steve Block]

(In reply to comment #20)
> Seeing that there is also a "DeviceOrientationController" class, the whole subsystem likely needs a rewrite anyway.

Out of interest, why do you say this? When Geolocation was re-written a while back, it adopted Geolocation/GeolocationController/GeolocationClient naming and I thought this was an accepted pattern?

[Hans Wennborg]

Comment on attachment 146863 [details]
Patch

> This might just be a display problem with the review tool, but I'm concerned that this isn't showing up as a rename+modify of DeviceOrientation.cpp, as we want to retain version history across the rename. Does it look OK on your client?

We looked at two recent renames (bug 75785 and bug 89578). When git is used to upload the patch, it doesn't show up as a rename in the review tool, but it will show up as a rename when it is committed to svn in the end.

[WebKit Review Bot]

Comment on attachment 146863 [details]
Patch

Rejecting attachment 146863 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
.h
patching file Source/WebKit/mac/WebCoreSupport/WebDeviceOrientationClient.mm
patching file Source/WebKit/mac/WebView/WebDeviceOrientation.mm
patching file Source/WebKit/mac/WebView/WebDeviceOrientationInternal.h
patching file Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp
Hunk #2 succeeded at 769 (offset -10 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Steve Block']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/13034150

[Alexey Proskuryakov]

> > Seeing that there is also a "DeviceOrientationController" class, the whole subsystem likely needs a rewrite anyway.
> 
> Out of interest, why do you say this? When Geolocation was re-written a while back, it adopted Geolocation/GeolocationController/GeolocationClient naming and I thought this was an accepted pattern?

I'm saying this because in object oriented programming, "data" and "controller" classes are almost always a sign of poorly done job. A class is supposed to encapsulated data and behaviors, and to match a domain concept of "object".

[Amy Ousterhout]

Created attachment 148819 [details]
Patch

[Steve Block]

Comment on attachment 148819 [details]
Patch

r=me

[Build Bot]

Comment on attachment 148819 [details]
Patch

Attachment 148819 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13036146

[Amy Ousterhout]

Created attachment 148978 [details]
Patch

[Steve Block]

Comment on attachment 148978 [details]
Patch

r=me

[WebKit Review Bot]

Comment on attachment 148978 [details]
Patch

Clearing flags on attachment: 148978

Committed r121016: <http://trac.webkit.org/changeset/121016>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.