We need a C API to send OrientationChange events.
Created attachment 151647 [details] Patch
Comment on attachment 151647 [details] Patch Attachment 151647 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13200271
Created attachment 151669 [details] Patch
Comment on attachment 151669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151669&action=review > Source/WebKit2/UIProcess/API/C/WKPage.cpp:694 > +void WKPageSendOrientationChangeEvent(WKPageRef page, int orientation) So it is not very clear what to pass as orientation. What about an enum like PORTRAIT_PRIMARY, PORTRAIT_SECONDARY, LANDSCALE_PRIMARY etc? > Source/WebKit2/UIProcess/API/C/WKPage.h:473 > +WK_EXPORT void WKPageSendOrientationChangeEvent(WKPageRef page, int orientation); It is not very clear what to pass as orientaiton. What about an enum like PORTRAIT_PRIMARY, PORTRAIT_SECONDARY, LANDSCALE... etc
(In reply to comment #4) > (From update of attachment 151669 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151669&action=review > > > Source/WebKit2/UIProcess/API/C/WKPage.cpp:694 > > +void WKPageSendOrientationChangeEvent(WKPageRef page, int orientation) > > So it is not very clear what to pass as orientation. What about an enum like PORTRAIT_PRIMARY, PORTRAIT_SECONDARY, LANDSCALE_PRIMARY etc? > > > Source/WebKit2/UIProcess/API/C/WKPage.h:473 > > +WK_EXPORT void WKPageSendOrientationChangeEvent(WKPageRef page, int orientation); > > It is not very clear what to pass as orientaiton. What about an enum like PORTRAIT_PRIMARY, PORTRAIT_SECONDARY, LANDSCALE... etc Thank you for your review. I agree that 'int orientation' is really ambiguous and I considered it. I also wanted to make it clear, but unfortunately I can not determine whether 0 indicates portrait for all devices including mobile devices, tablet, monitor and so on. apple document [1] mentioned that portrait indicates 0 degrees in iphone and it is same as ipad also. (tested) I think that iphone and ipad oriented to portrait as a natural orientation. Few android devices oriented to landscape as a natural orientation. I realize that android's similar native API [2] used that 0 degree is a natural orientation. If I read source correctly, android browser used same APIs for onorientationchanged javascript event. (I checked my tablet(galaxy tab 10') and it returned 0 for landscape!) I thought that the meaning of orientation is not clear now. Because this value is just passed to javascript side transparently as `orientation` of onorientationchanged event and WebCore is also using 'int orientation', I chose transparent way. I think that new W3C spec [3] solve this unclear situation, but we need onorientationchange also because many contents are using this. [1] http://developer.apple.com/library/ios/#DOCUMENTATION/AppleApplications/Reference/SafariWebContent/HandlingEvents/HandlingEvents.html [2] http://developer.android.com/reference/android/view/OrientationEventListener.html#onOrientationChanged(int) [3] http://www.w3.org/TR/screen-orientation/
(In reply to comment #4) > (From update of attachment 151669 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151669&action=review > > > Source/WebKit2/UIProcess/API/C/WKPage.cpp:694 > > +void WKPageSendOrientationChangeEvent(WKPageRef page, int orientation) > > So it is not very clear what to pass as orientation. What about an enum like PORTRAIT_PRIMARY, PORTRAIT_SECONDARY, LANDSCALE_PRIMARY etc? While thinking little bit more, how do you think about ORIENTATION_0, ORIENTATION_90, ORIENTATION_180, ORIENTATION_270 ?
I guess iOS uses 0 for portrait (primary) and though I am not sure about Android, I bet they are using the same for Jelly Beans. If that is the case it makes sense to just follow the new spec. Kenneth
(In reply to comment #7) > I guess iOS uses 0 for portrait (primary) and though I am not sure about Android, I bet they are using the same for Jelly Beans. If that is the case it makes sense to just follow the new spec. > > Kenneth Updated information about android. I am not sure about the 'chrome for android' in jelly beans. But, 'chrome for android' snapshot in below url uses 0 as natural orientation like current android browser. https://developers.google.com/chrome/mobile/docs/faq IMHO, I think that using 0 as natural orientation is not wrong for IOS and android because current IOS devices looks naturally oriented to portrait. But if we want to use enumeration or share the logic with screen orientation spec, ambiguity of 0 degree should be solved.
Comment on attachment 151669 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151669&action=review Shouldn't this change perhaps also come with WTR support and a layout test? It sounds like a straight-forward thing to test :) >>>> Source/WebKit2/UIProcess/API/C/WKPage.cpp:694 >>>> +void WKPageSendOrientationChangeEvent(WKPageRef page, int orientation) >>> >>> So it is not very clear what to pass as orientation. What about an enum like PORTRAIT_PRIMARY, PORTRAIT_SECONDARY, LANDSCALE_PRIMARY etc? >> >> Thank you for your review. >> I agree that 'int orientation' is really ambiguous and I considered it. >> >> I also wanted to make it clear, but unfortunately I can not determine whether 0 indicates portrait for all devices including mobile devices, tablet, monitor and so on. >> >> apple document [1] mentioned that portrait indicates 0 degrees in iphone and it is same as ipad also. (tested) >> I think that iphone and ipad oriented to portrait as a natural orientation. >> >> Few android devices oriented to landscape as a natural orientation. >> I realize that android's similar native API [2] used that 0 degree is a natural orientation. >> If I read source correctly, android browser used same APIs for onorientationchanged javascript event. >> (I checked my tablet(galaxy tab 10') and it returned 0 for landscape!) >> >> I thought that the meaning of orientation is not clear now. >> Because this value is just passed to javascript side transparently as `orientation` of onorientationchanged event and WebCore is also using 'int orientation', >> I chose transparent way. >> >> I think that new W3C spec [3] solve this unclear situation, but we need onorientationchange also because many contents are using this. >> >> [1] http://developer.apple.com/library/ios/#DOCUMENTATION/AppleApplications/Reference/SafariWebContent/HandlingEvents/HandlingEvents.html >> [2] http://developer.android.com/reference/android/view/OrientationEventListener.html#onOrientationChanged(int) >> [3] http://www.w3.org/TR/screen-orientation/ > > While thinking little bit more, > how do you think about ORIENTATION_0, ORIENTATION_90, ORIENTATION_180, ORIENTATION_270 ? I think providing the values from iOS (0, 90, -90 and 180) as enum values along with brief documentation would be good. The documentation should probably state that the provided orientation in degress is _relative_ to the native orientation of the device.
Comment on attachment 151669 [details] Patch Would be nice if you could add a C API test. All the ports can benefit from it and it. They are located at Tools/TestWebKitAPI/ and EFL are running most of them at the bots.
(In reply to comment #10) > (From update of attachment 151669 [details]) > Would be nice if you could add a C API test. All the ports can benefit from it and it. They are located at Tools/TestWebKitAPI/ and EFL are running most of them at the bots. Would be nice if you could add a C API test. All the ports can benefit from it. They are located at Tools/TestWebKitAPI/. EFL is running most of them at the bots.
(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 151669 [details] [details]) > > Would be nice if you could add a C API test. All the ports can benefit from it and it. They are located at Tools/TestWebKitAPI/ and EFL are running most of them at the bots. > > Would be nice if you could add a C API test. All the ports can benefit from it. They are located at Tools/TestWebKitAPI/. EFL is running most of them at the bots. I think a layout test would be more suitable, to also set the successful propagation to the JS side. It should be straight-forward to add to WTR and WTR would use the new WK2 C API.
Created attachment 160913 [details] Patch
(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (From update of attachment 151669 [details] [details] [details]) > > > Would be nice if you could add a C API test. All the ports can benefit from it and it. They are located at Tools/TestWebKitAPI/ and EFL are running most of them at the bots. > > > > Would be nice if you could add a C API test. All the ports can benefit from it. They are located at Tools/TestWebKitAPI/. EFL is running most of them at the bots. > > I think a layout test would be more suitable, to also set the successful propagation to the JS side. It should be straight-forward to add to WTR and WTR would use the new WK2 C API. Try to introduce kWKOrientation enums, but I am not sure whether it is what you want. And I added simple layout test. But I can not find proper way to make test case for asynchronous API.
Comment on attachment 160913 [details] Patch Attachment 160913 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13651195 New failing tests: fast/dom/Orientation/basic-orientationchange.html
Created attachment 160936 [details] Archive of layout-test-results from gce-cr-linux-07 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-07 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
This is a little old. Are you still planning to land this? (Should it be reviewed still?)
Comment on attachment 160913 [details] Patch Cleared flags. Now, I am not following this well.