Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sync DeviceMotionEvent as per web specification #26001

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Sync DeviceMotionEvent as per web specification #26001

wants to merge 1 commit into from

Conversation

Ahmad-S792
Copy link
Contributor

@Ahmad-S792 Ahmad-S792 commented Mar 16, 2024 •
edited by webkit-early-warning-system
Loading 

Sync `DeviceMotionEvent` as per web specification
https://bugs.webkit.org/show_bug.cgi?id=271098
rdar://problem/125271931

Reviewed by NOBODY (OOPS!).

This patch aligns WebKit with Blink / Chrome and Web Specification [1]:

[1] https://w3c.github.io/deviceorientation/#device-motion-event-api

By removing `initDeviceMotionEvent`, WebKit aligns with Web Specification
and Blink also removed it in 2017 via below commit:

Reference: https://chromium.googlesource.com/chromium/src.git/+/0921e8319f228e406019b434701ac9f181ee8ea1

NOTE: We cannot update test to leverage constructor, since
we don't have support for it.

e.g., `event.initDeviceMotionEvent` changed to `event = new DeviceMotionEvent`

Additionally, it adds [SecureContext] and also make 'interval'
non-nullable. Also adding '=null' values for 'Accelerate'
and 'Rotate' dictonaries.

NOTE: We cannot update test to leverage constructor, since
we don't have support for it.

e.g., `event.initDeviceMotionEvent` changed to `event = new DeviceMotionEvent`

Additionally, we have same test as WPT with similar name.
Although, it is `https` due to being `SecureContext`.
For future, I am adding few FIXME as well in IDL file to align
with web specification.

* Source/WebCore/dom/DeviceMotionEvent.cpp:
(convert): DeviceMotionEvent::Acceleration
(convert): DeviceMotionEvent::RotationRate
(DeviceMotionEvent::initDeviceMotionEvent):
* Source/WebCore/dom/DeviceMotionEvent.h:
* Source/WebCore/dom/DeviceMotionEvent.idl:
* Source/WebCore/bindings/js/WebCoreBuiltinNames.h: Add macro for 'SecureContext'
* Source/WebCore/dom/DeviceMotionData.cpp:
(DeviceMotionData::create): Update argument to remove std::optional<>
(DeviceMotionData::DeviceMotionData): Ditto
* Source/WebCore/dom/DeviceMotionData.h:
* Source/WebCore/dom/DeviceMotionEvent.cpp:
(DeviceMotionEvent::interval):
* Source/WebCore/dom/DeviceMotionEvent.h:
* LayoutTests/fast/dom/DeviceMotion/optional-event-properties.html: Rebaselined
* LayoutTests/fast/dom/DeviceMotion/optional-event-properties-expected.txt: Rebaselined

4db865a

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ❌ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
❌ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
❌ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
❌ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
⏳ 🧪 vision-wk2
❌ 🛠 tv
❌ 🛠 tv-sim
❌ 🛠 watch
❌ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Mar 16, 2024
@Ahmad-S792 Ahmad-S792 self-assigned this Mar 16, 2024
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Mar 16, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 36efca1)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ⏳ 🧪 wpe-wk2
✅ 🧪 webkitperl   🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
⏳ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-skia
⏳ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2   🧪 gtk-wk2
✅ 🛠 tv-sim   🧪 api-gtk
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 force-pushed the fix271098-removal-initDeviceMotionEvent branch from 36efca1 to d29e964 Compare March 16, 2024 06:42
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Mar 16, 2024 •
edited
Loading

EWS run on previous version of this PR (hash d29e964)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-skia
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 watch
✅ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 16, 2024
@Ahmad-S792 Ahmad-S792 force-pushed the fix271098-removal-initDeviceMotionEvent branch from d29e964 to 6063077 Compare March 19, 2024 00:15
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Mar 19, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 6063077)

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ⏳ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ⏳ 🛠 wpe-cairo
⏳ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
⏳ 🛠 🧪 jsc-arm64 ✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 watch
✅ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 removed the merging-blocked Applied to prevent a change from being merged label Mar 19, 2024
@Ahmad-S792 Ahmad-S792 marked this pull request as ready for review March 19, 2024 00:33
rniwa
rniwa reviewed Mar 19, 2024
View reviewed changes
Source/WebCore/dom/DeviceMotionEvent.idl Outdated
[
Conditional=DEVICE_ORIENTATION,
Exposed=Window
] interface DeviceMotionEvent : Event {
// FIXME: Add `constructor` by uncommenting below.
// constructor(DOMString type, optional DeviceMotionEventInit eventInitDict = {});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should just add constructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ahmad-S792 did you have a chance to check this? Or do you need help.
I'm asking because of the recent bug
https://bugs.webkit.org/show_bug.cgi?id=271891

Copy link
Contributor Author

@Ahmad-S792 Ahmad-S792 Apr 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked into it yet but I manage to fix another [SecureContext] issue in these though. So some progress but not directly on constructor side. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karlcow - I just rebased it and added few more alignments but still working on how to add 'constructor'. Although it did progressed one or two WPT tests.

@Ahmad-S792 Ahmad-S792 force-pushed the fix271098-removal-initDeviceMotionEvent branch from 6063077 to 60ce731 Compare June 8, 2024 00:44
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jun 8, 2024 •
edited
Loading

EWS run on previous version of this PR (hash 60ce731)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ❌ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
❌ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
⏳ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
⏳ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
⏳ 🧪 vision-wk2
❌ 🛠 tv
❌ 🛠 tv-sim
❌ 🛠 watch
❌ 🛠 watch-sim

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jun 8, 2024
@Ahmad-S792 Ahmad-S792 changed the title Remove non-standard DeviceMotionEvent.initDeviceMotionEvent Sync DeviceMotionEvent as per web specification Jun 8, 2024
@Ahmad-S792
Sync DeviceMotionEvent as per web specification
4db865a 
https://bugs.webkit.org/show_bug.cgi?id=271098
rdar://problem/125271931

Reviewed by NOBODY (OOPS!).

This patch aligns WebKit with Blink / Chrome and Web Specification [1]:

[1] https://w3c.github.io/deviceorientation/#device-motion-event-api

By removing `initDeviceMotionEvent`, WebKit aligns with Web Specification
and Blink also removed it in 2017 via below commit:

Reference: https://chromium.googlesource.com/chromium/src.git/+/0921e8319f228e406019b434701ac9f181ee8ea1

Additionally, it adds [SecureContext] and also make 'interval'
non-nullable. Also adding '=null' values for 'Accelerate'
and 'Rotate' dictonaries.

NOTE: We cannot update test to leverage constructor, since
we don't have support for it.

e.g., `event.initDeviceMotionEvent` changed to `event = new DeviceMotionEvent`

Additionally, we have same test as WPT with similar name.
Although, it is `https` due to being `SecureContext`.
For future, I am adding few FIXME as well in IDL file to align
with web specification.

* Source/WebCore/dom/DeviceMotionEvent.cpp:
(convert): DeviceMotionEvent::Acceleration
(convert): DeviceMotionEvent::RotationRate
(DeviceMotionEvent::initDeviceMotionEvent):
* Source/WebCore/dom/DeviceMotionEvent.h:
* Source/WebCore/dom/DeviceMotionEvent.idl:
* Source/WebCore/bindings/js/WebCoreBuiltinNames.h: Add macro for 'SecureContext'
* Source/WebCore/dom/DeviceMotionData.cpp:
(DeviceMotionData::create): Update argument to remove std::optional<>
(DeviceMotionData::DeviceMotionData): Ditto
* Source/WebCore/dom/DeviceMotionData.h:
* Source/WebCore/dom/DeviceMotionEvent.cpp:
(DeviceMotionEvent::interval):
* Source/WebCore/dom/DeviceMotionEvent.h:
* LayoutTests/fast/dom/DeviceMotion/optional-event-properties.html: Rebaselined
* LayoutTests/fast/dom/DeviceMotion/optional-event-properties-expected.txt: Rebaselined
@Ahmad-S792 Ahmad-S792 force-pushed the fix271098-removal-initDeviceMotionEvent branch from 60ce731 to 4db865a Compare July 29, 2024 14:16
@webkit-early-warning-system
Copy link
Collaborator

webkit-early-warning-system commented Jul 29, 2024 •
edited
Loading

EWS run on current version of this PR (hash 4db865a)

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ❌ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ❌ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ❌ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
❌ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
❌ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
❌ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
❌ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
⏳ 🧪 vision-wk2
❌ 🛠 tv
❌ 🛠 tv-sim
❌ 🛠 watch
❌ 🛠 watch-sim

@Ahmad-S792 Ahmad-S792 marked this pull request as draft July 29, 2024 14:16
@Ahmad-S792
Copy link
Contributor Author

Just rebasing and will see if I can fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rniwa rniwa rniwa left review comments

@karlcow karlcow karlcow left review comments

@cdumez cdumez Awaiting requested review from cdumez cdumez is a code owner

Assignees

@Ahmad-S792 Ahmad-S792

Labels
DOM For bugs specific to XML/HTML DOM elements (including parsing). merging-blocked Applied to prevent a change from being merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ahmad-S792 @webkit-early-warning-system @rniwa @karlcow @webkit-ews-buildbot