Bug 88663 - Renamed DeviceOrientation to DeviceOrientationData
Summary: Renamed DeviceOrientation to DeviceOrientationData
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-08 09:46 PDT by Amy Ousterhout
Modified: 2012-06-22 04:33 PDT (History)
14 users (show)

See Also:


Attachments
Patch (60.48 KB, patch)
2012-06-08 09:55 PDT, Amy Ousterhout
no flags Details | Formatted Diff | Diff
Patch (66.65 KB, patch)
2012-06-11 04:03 PDT, Amy Ousterhout
no flags Details | Formatted Diff | Diff
Patch (68.03 KB, patch)
2012-06-11 06:07 PDT, Amy Ousterhout
no flags Details | Formatted Diff | Diff
Patch (68.42 KB, patch)
2012-06-11 08:58 PDT, Amy Ousterhout
no flags Details | Formatted Diff | Diff
Patch (67.88 KB, patch)
2012-06-21 09:43 PDT, Amy Ousterhout
no flags Details | Formatted Diff | Diff
Patch (68.44 KB, patch)
2012-06-22 01:52 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-08 09:46:25 PDT
Renamed DeviceOrientation to DeviceOrientationData
Comment 1 Amy Ousterhout 2012-06-08 09:55:57 PDT
Created attachment 146589 [details]
Patch
Comment 2 WebKit Review Bot 2012-06-08 09:59:06 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 WebKit Review Bot 2012-06-08 09:59:31 PDT
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.
Comment 4 Early Warning System Bot 2012-06-08 10:04:46 PDT
Comment on attachment 146589 [details]
Patch

Attachment 146589 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12924353
Comment 5 Build Bot 2012-06-08 10:05:39 PDT
Comment on attachment 146589 [details]
Patch

Attachment 146589 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12922539
Comment 6 Early Warning System Bot 2012-06-08 10:07:25 PDT
Comment on attachment 146589 [details]
Patch

Attachment 146589 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12924355
Comment 7 Alexey Proskuryakov 2012-06-08 15:52:06 PDT
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.
Comment 8 Adam Barth 2012-06-09 11:37:23 PDT
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.)
Comment 9 Hans Wennborg 2012-06-09 12:28:10 PDT
(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.
Comment 10 Amy Ousterhout 2012-06-11 04:03:23 PDT
Created attachment 146822 [details]
Patch
Comment 11 Build Bot 2012-06-11 04:42:47 PDT
Comment on attachment 146822 [details]
Patch

Attachment 146822 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12942220
Comment 12 Amy Ousterhout 2012-06-11 06:07:00 PDT
Created attachment 146841 [details]
Patch
Comment 13 Build Bot 2012-06-11 06:30:10 PDT
Comment on attachment 146841 [details]
Patch

Attachment 146841 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12938405
Comment 14 Amy Ousterhout 2012-06-11 08:58:39 PDT
Created attachment 146863 [details]
Patch
Comment 15 Hans Wennborg 2012-06-11 09:15:23 PDT
Comment on attachment 146863 [details]
Patch

Looks great to me! Thanks.
Comment 16 Adam Barth 2012-06-11 10:34:14 PDT
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
Comment 17 Adam Barth 2012-06-11 10:37:10 PDT
I guess I don't understand why the new name is better so I'll let someone else review this patch.
Comment 18 Alexey Proskuryakov 2012-06-11 12:15:50 PDT
Ditto. This doesn't look like an improvement to me.
Comment 19 Adam Barth 2012-06-12 11:06:53 PDT
(I should also say that I don't have a strong opinion here.)
Comment 20 Alexey Proskuryakov 2012-06-12 11:14:38 PDT
Seeing that there is also a "DeviceOrientationController" class, the whole subsystem likely needs a rewrite anyway.
Comment 21 Hans Wennborg 2012-06-13 02:12:47 PDT
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?
Comment 22 Steve Block 2012-06-21 07:39:16 PDT
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?
Comment 23 Steve Block 2012-06-21 07:41:21 PDT
(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?
Comment 24 Hans Wennborg 2012-06-21 08:00:00 PDT
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.
Comment 25 WebKit Review Bot 2012-06-21 08:02:44 PDT
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
Comment 26 Alexey Proskuryakov 2012-06-21 09:14:03 PDT
> > 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".
Comment 27 Amy Ousterhout 2012-06-21 09:43:03 PDT
Created attachment 148819 [details]
Patch
Comment 28 Steve Block 2012-06-21 09:49:12 PDT
Comment on attachment 148819 [details]
Patch

r=me
Comment 29 Build Bot 2012-06-21 10:04:36 PDT
Comment on attachment 148819 [details]
Patch

Attachment 148819 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13036146
Comment 30 Amy Ousterhout 2012-06-22 01:52:40 PDT
Created attachment 148978 [details]
Patch
Comment 31 Steve Block 2012-06-22 02:09:05 PDT
Comment on attachment 148978 [details]
Patch

r=me
Comment 32 WebKit Review Bot 2012-06-22 04:33:02 PDT
Comment on attachment 148978 [details]
Patch

Clearing flags on attachment: 148978

Committed r121016: <http://trac.webkit.org/changeset/121016>
Comment 33 WebKit Review Bot 2012-06-22 04:33:32 PDT
All reviewed patches have been landed.  Closing bug.