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

document.applets.length is O(n) #12689

Merged
merged 1 commit into from Apr 13, 2023
Merged

document.applets.length is O(n) #12689

merged 1 commit into from Apr 13, 2023

Conversation

rniwa
Copy link
Member

@rniwa rniwa commented Apr 13, 2023 •
edited by webkit-early-warning-system

db245cc

document.applets.length is O(n)
https://bugs.webkit.org/show_bug.cgi?id=255389

Reviewed by Chris Dumez.

Introduce and deploy EmptyHTMLCollection, which is always empty, for document.applets.

* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::applets):
* Source/WebCore/html/EmptyHTMLCollection.h: Added.
* Source/WebCore/html/GenericCachedHTMLCollection.cpp:
(WebCore::GenericCachedHTMLCollection<traversalType>::elementMatches const):

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

bd86b3b

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 ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@rniwa rniwa requested a review from cdumez as a code owner April 13, 2023 05:40
@rniwa rniwa self-assigned this Apr 13, 2023
@rniwa rniwa added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Apr 13, 2023
@rniwa rniwa requested a review from anttijk April 13, 2023 05:40
cdumez
cdumez reviewed Apr 13, 2023
View reviewed changes
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

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

This is about as much code as my EmptyHTMLCollection proposal. Also, the document.applets collection will still have an unnecessary CollectionIndexCache. It will also still register with the document for invalidation and get calls for invalidation when the DOM tree changes AFAICT.

Could you clarify why this is better?

@rniwa
Copy link
Member Author

rniwa commented Apr 13, 2023

This is about as much code as my EmptyHTMLCollection proposal. Also, the document.applets collection will still have an unnecessary CollectionIndexCache. It will also still register with the document for invalidation and get calls for invalidation when the DOM tree changes AFAICT.

That's true. Maybe we should go with EmptyHTMLCollection then.

@webkit-early-warning-system
Copy link
Collaborator

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

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 ✅ 🛠 gtk
⏳ 🧪 ios-wk2-wpt   🧪 mac-wk1 ⏳ 🧪 gtk-wk2
✅ 🧪 api-ios   🧪 mac-wk2 ⏳ 🧪 api-gtk
✅ 🛠 tv   🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim   🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

@cdumez
Copy link
Contributor

cdumez commented Apr 13, 2023

I don't mind writing up the EmptyHTMLCollection patch tomorrow. I assume it will be trivial but we can re-evaluate when we see the alternative patch.

@webkit-early-warning-system
Copy link
Collaborator

EWS run on previous version of this PR (hash 7d0a912)

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 ✅ 🛠 gtk
⏳ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ⏳ 🧪 gtk-wk2
  🧪 api-ios   🧪 mac-wk2 ⏳ 🧪 api-gtk
✅ 🛠 tv ⏳ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 watch
✅ 🛠 watch-sim

cdumez
cdumez approved these changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

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

LGTM with nit.

Source/WebCore/dom/Document.cpp
@@ -6294,7 +6295,7 @@ Ref<HTMLCollection> Document::images()

Ref<HTMLCollection> Document::applets()
{
return ensureCachedCollection<DocEmpty>();
return ensureRareData().ensureNodeLists().addCachedCollection<EmptyHTMLCollection>(*this, DocEmpty);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be nice to move DocEmpty to the ASSERT_NOT_REACHED() section (with DocAll) inside
GenericCachedHTMLCollection<traversalType>::elementMatches(Element& instead of making it look like GenericCachedHTMLCollection is dealing with this type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, I forgot to do that.

cdumez
cdumez approved these changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

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

LGTM with nit.

@cdumez
Copy link
Contributor

cdumez commented Apr 13, 2023

And thanks for writing up the patch!

@webkit-early-warning-system
Copy link
Collaborator

EWS run on current version of this PR (hash bd86b3b)

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 ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim ✅ 🧪 mac-wk2-stress
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@cdumez cdumez added the merge-queue Applied to send a pull request to merge-queue label Apr 13, 2023
@rniwa @cdumez
document.applets.length is O(n)
db245cc 
https://bugs.webkit.org/show_bug.cgi?id=255389

Reviewed by Chris Dumez.

Introduce and deploy EmptyHTMLCollection, which is always empty, for document.applets.

* Source/WebCore/WebCore.xcodeproj/project.pbxproj:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::applets):
* Source/WebCore/html/EmptyHTMLCollection.h: Added.
* Source/WebCore/html/GenericCachedHTMLCollection.cpp:
(WebCore::GenericCachedHTMLCollection<traversalType>::elementMatches const):

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

Committed 262911@main (db245cc): https://commits.webkit.org/262911@main

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

@webkit-commit-queue webkit-commit-queue merged commit db245cc into WebKit:main Apr 13, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Apr 13, 2023
@rniwa rniwa deleted the fix255389 branch April 14, 2023 05:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdumez cdumez cdumez approved these changes

@anttijk anttijk Awaiting requested review from anttijk

Assignees

@rniwa rniwa

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