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

Use getElementsByClassName so that we don't need to use querySelectorAll at every step of the loop #368

Closed
wants to merge 2 commits into from
Closed

Use getElementsByClassName so that we don't need to use querySelectorAll at every step of the loop #368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
01009bf
This PR fixes the issue #365: jQuery test only ever marks the first t…
rniwa Jan 27, 2024
07a8c6a
Use getElementsByClassName so that we don't need to use querySelectorAll
julienw Jan 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions resources/benchmark-runner.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ class Page {
return this._wrapElement(element);
}

getElementsByClassName(className) {
return this._frame.contentDocument.getElementsByClassName(className);
Copy link
Member

Choose a reason for hiding this comment

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

Hm... I guess this doesn't wrap each element in PageElement. Maybe we want to wrap this in a Proxy object which creates PageElement for each item in the list?

Copy link
Contributor Author

@julienw julienw Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah exactly. This was more a cheap experiment to look at the performance characteristics. Spoiler: this doesn't change much from your querySelectorAll run in every step of the loop.

IMO wrapping in a Proxy is too complex for what we want to do.
This also led me to take a step back and think more about the PageElement wrapper. What is the purpose of this wrapper? My understanding is that this serves 2 purposes:
1/ Limits the things we can do with a retrieved element. Is this purpose useful in the context of this benchmark? I'd like to argue that it's not.
2/ Provides some common functionality (all the event dispatchers especially). This is useful, but could be provided with a more functional model taking the element as parameter.

Therefore my proposal (for speedometer 4) would be to get rid of the wrapper that brings more complexity than it resolves problems. I'll file an issue about that.

Note: I also wrote #367 which is IMO more suitable in this case. Tell me what you think about it :-)

}

call(functionName) {
this._frame.contentWindow[functionName]();
return null;
Expand Down
8 changes: 4 additions & 4 deletions resources/tests.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -566,9 +566,9 @@ Suites.push({
}
}),
new BenchmarkTestStep("CompletingAllItems", (page) => {
const checkboxes = page.querySelectorAll(".toggle");
const allItems = page.getElementsByClassName("toggle");
for (let i = 0; i < numberOfItemsToAdd; i++)
checkboxes[i].click();
allItems[i].click();
}),
new BenchmarkTestStep("DeletingAllItems", (page) => {
for (let i = numberOfItemsToAdd - 1; i >= 0; i--)
Expand Down Expand Up @@ -596,9 +596,9 @@ Suites.push({
}
}),
new BenchmarkTestStep("CompletingAllItems", (page) => {
const checkboxes = page.querySelectorAll(".toggle");
const allItems = page.getElementsByClassName("toggle");
for (let i = 0; i < numberOfItemsToAdd; i++)
checkboxes[i].click();
allItems[i].click();
}),
new BenchmarkTestStep("DeletingAllItems", (page) => {
for (let i = numberOfItemsToAdd - 1; i >= 0; i--)
Expand Down
Loading