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

CSS and SharedArrayBuffer: don't make support conditional #17689

Merged
merged 1 commit into from
Aug 7, 2019
Merged

CSS and SharedArrayBuffer: don't make support conditional #17689

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 3 additions & 8 deletions css/css-layout-api/constraints-data-sab-failure.https.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,14 +35,9 @@
let childFragment = null;

try {
// We need SABs to be enabled to properly run this test.
if (typeof SharedArrayBuffer !== 'undefined') {
childFragment = yield child.layoutNextFragment({
data: { sab: new SharedArrayBuffer(4) }
});
} else {
throw Error();
}
childFragment = yield child.layoutNextFragment({
data: { sab: new SharedArrayBuffer(4) }
});
} catch(e) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This also seems kinda wrong as the specific error is not checked. Why is this not using assert_throws?

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently the test passes if SAB isn't supported (but the API otherwise works). I assume that's intentional. If so, then it can't just use assert_throws, as that method can only check for one type of error at a time.

But I agree that restructuring it to use assert_throws (rather than a catch and then implicit success) would be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

SAB shouldn't be optional.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, your response is too terse for me to understand. Are you saying that it's okay for the test to fail if SAB isn't supported? This isn't intended to be a test for SAB support.

Copy link
Member Author

Choose a reason for hiding this comment

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

The test is only relevant if SharedArrayBuffer is supported. Having a test always succeed when a feature it's testing isn't supported is a not a pattern we generally follow and I'd rather we don't, ever.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's reasonable.

// Success! The structured cloning algorithm should have thrown an error.
childFragment = yield child.layoutNextFragment({});
Expand Down
6 changes: 1 addition & 5 deletions css/css-layout-api/fragment-data-sab-failure.https.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,7 @@
*layout(children, edges, constraints, styleMap) {
const childFragments = yield children.map(child => child.layoutNextFragment());

if (typeof SharedArrayBuffer !== 'undefined') {
return {autoBlockSize: 0, childFragments, data: {sab: new SharedArrayBuffer(4) }};
} else {
throw Error();
}
return {autoBlockSize: 0, childFragments, data: {sab: new SharedArrayBuffer(4) }};
}
});
</script>
Expand Down