Renamed DeviceOrientation to DeviceOrientationData
Created attachment 146589 [details] Patch
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.
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 on attachment 146589 [details] Patch Attachment 146589 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12924353
Comment on attachment 146589 [details] Patch Attachment 146589 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12922539
Comment on attachment 146589 [details] Patch Attachment 146589 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12924355
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 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.)
(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.
Created attachment 146822 [details] Patch
Comment on attachment 146822 [details] Patch Attachment 146822 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12942220
Created attachment 146841 [details] Patch
Comment on attachment 146841 [details] Patch Attachment 146841 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12938405
Created attachment 146863 [details] Patch
Comment on attachment 146863 [details] Patch Looks great to me! Thanks.
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
I guess I don't understand why the new name is better so I'll let someone else review this patch.
Ditto. This doesn't look like an improvement to me.
(I should also say that I don't have a strong opinion here.)
Seeing that there is also a "DeviceOrientationController" class, the whole subsystem likely needs a rewrite anyway.
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 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?
(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 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 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
> > 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".
Created attachment 148819 [details] Patch
Comment on attachment 148819 [details] Patch r=me
Comment on attachment 148819 [details] Patch Attachment 148819 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13036146
Created attachment 148978 [details] Patch
Comment on attachment 148978 [details] Patch r=me
Comment on attachment 148978 [details] Patch Clearing flags on attachment: 148978 Committed r121016: <http://trac.webkit.org/changeset/121016>
All reviewed patches have been landed. Closing bug.