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

Make the Web Locks uniqueName helper (likely) globally unique #15018

Closed
wants to merge 1 commit into from
Closed

Make the Web Locks uniqueName helper (likely) globally unique #15018

wants to merge 1 commit into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Jan 23, 2019

Fixes #15015.

@Ms2ger
Copy link
Contributor

Ms2ger commented Jan 23, 2019

I think we'd usually use token() from common/utils.js to get a unique identifier; I haven't checked if it'd be appropriate here.

@foolip
Copy link
Member Author

foolip commented Jan 23, 2019

I didn't know we had a helper. @inexorabletash does that look good to you, are there constraints on what the lock name should look like to be "realistic"?

@foolip foolip mentioned this pull request Jan 23, 2019
Merged
@inexorabletash
Copy link
Member

Using token() sgtm

No "realistic" constraints on lock names.

@foolip foolip mentioned this pull request Jan 24, 2019
Open
pwnall
pwnall reviewed Jan 31, 2019
View reviewed changes
web-locks/resources/helpers.js
self.uniqueName = (testCase, prefix) => {
return `${self.location.pathname}-${prefix}-${testCase.name}-${++res_num}`;
const rand = Math.floor(1e10 * Math.random());
Copy link
Contributor

Choose a reason for hiding this comment

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

If it helps, I've used this in some personal projects -- Math.random().toString(36).substr(2)

inexorabletash
inexorabletash approved these changes Jan 31, 2019
View reviewed changes
Copy link
Member

@inexorabletash inexorabletash left a comment •
edited by foolip
Loading

Choose a reason for hiding this comment

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

Ooops, forgot to actually review this.

This is fine, but using token() would be better, wouldn't it?

@foolip
Copy link
Member Author

foolip commented Feb 15, 2019

Yes, I should use token(), I just neglected this PR. Only trouble is that all tests that use uniqueName() will have to know that it depends on token() and also include common/utils.js. That's not very elegant.

@gsnedders gsnedders closed this Jan 24, 2020
@gsnedders gsnedders deleted the foolip/uniqueName branch January 24, 2020 18:06
@gsnedders gsnedders restored the foolip/uniqueName branch January 24, 2020 18:51
@Hexcles Hexcles reopened this Jan 24, 2020
@foolip
Copy link
Member Author

foolip commented Dec 15, 2020

Closing this before it's two years old, since clearly I'm not going to prioritize fixing this using token() correctly.

@foolip foolip closed this Dec 15, 2020
@foolip foolip deleted the foolip/uniqueName branch December 15, 2020 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pwnall pwnall pwnall left review comments

@inexorabletash inexorabletash inexorabletash approved these changes

Assignees

@inexorabletash inexorabletash

Labels
web-locks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

web-locks/signal.tentative.https.any.serviceworker.html is flaky
7 participants
@foolip @Ms2ger @inexorabletash @pwnall @gsnedders @Hexcles @wpt-pr-bot