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

data-x-2="" not represented in dataset #3498

Merged
merged 1 commit into from Aug 21, 2022
Merged

data-x-2="" not represented in dataset #3498

merged 1 commit into from Aug 21, 2022

Conversation

rniwa
Copy link
Member

@rniwa rniwa commented Aug 20, 2022 •
edited by webkit-early-warning-system

8a40482

data-x-2="" not represented in dataset
https://bugs.webkit.org/show_bug.cgi?id=123890

Reviewed by Brent Fulgham.

The bug was caused by propertyNameMatchesAttributeName implementing comparison incorrectly.

Fixed the bug by eliminating this function and reusing convertAttributeNameToPropertyName
when there is exactly one attribute. While the new code does allocate a String object,
it's still faster than creating a AtomString.

* LayoutTests/fast/dom/dataset-expected.txt:
* LayoutTests/fast/dom/dataset.html: Added a test case.

* Source/WebCore/dom/DatasetDOMStringMap.cpp:
(WebCore::propertyNameMatchesAttributeName): Deleted.
(WebCore::DatasetDOMStringMap::isSupportedPropertyName const):
(WebCore::DatasetDOMStringMap::item const):

Canonical link: https://commits.webkit.org/253625@main

26defca

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

@rniwa rniwa requested a review from cdumez as a code owner August 20, 2022 08:05
@rniwa rniwa self-assigned this Aug 20, 2022
@rniwa rniwa added 528+ (Nightly build) DOM For bugs specific to XML/HTML DOM elements (including parsing). labels Aug 20, 2022
@webkit-early-warning-system
Copy link
Collaborator

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

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

@webkit-early-warning-system
Copy link
Collaborator

26defca

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

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 20, 2022
@rniwa rniwa removed the merging-blocked Applied to prevent a change from being merged label Aug 20, 2022
brentfulgham
brentfulgham approved these changes Aug 21, 2022
View reviewed changes
Copy link
Contributor

@brentfulgham brentfulgham left a comment

Choose a reason for hiding this comment

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

Looks like a functional improvement, and a possible performance improvement!

@rniwa
Copy link
Member Author

rniwa commented Aug 21, 2022

Looks like a functional improvement, and a possible performance improvement!

It's worse in terms of perf but at least it's more correct.

@rniwa rniwa added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Aug 21, 2022
@rniwa
data-x-2="" not represented in dataset
8a40482 
https://bugs.webkit.org/show_bug.cgi?id=123890

Reviewed by Brent Fulgham.

The bug was caused by propertyNameMatchesAttributeName implementing comparison incorrectly.

Fixed the bug by eliminating this function and reusing convertAttributeNameToPropertyName
when there is exactly one attribute. While the new code does allocate a String object,
it's still faster than creating a AtomString.

* LayoutTests/fast/dom/dataset-expected.txt:
* LayoutTests/fast/dom/dataset.html: Added a test case.

* Source/WebCore/dom/DatasetDOMStringMap.cpp:
(WebCore::propertyNameMatchesAttributeName): Deleted.
(WebCore::DatasetDOMStringMap::isSupportedPropertyName const):
(WebCore::DatasetDOMStringMap::item const):

Canonical link: https://commits.webkit.org/253625@main
@webkit-commit-queue
Copy link
Collaborator

Committed 253625@main (8a40482): https://commits.webkit.org/253625@main

Reviewed commits have been landed. Closing PR #3498 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system merged commit 8a40482 into WebKit:main Aug 21, 2022
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Aug 21, 2022
@rniwa rniwa deleted the fix123890 branch August 21, 2022 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brentfulgham brentfulgham brentfulgham approved these changes

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

Assignees

@rniwa rniwa

Labels
DOM For bugs specific to XML/HTML DOM elements (including parsing).
Projects
None yet
Milestone
No milestone
5 participants
@rniwa @webkit-early-warning-system @webkit-commit-queue @brentfulgham @webkit-ews-buildbot