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.

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

[presentation-api] Stop using EventWatcher for IndexedDB #9046

Merged

Conversation

tomoyukilabs
Copy link
Member

@tomoyukilabs tomoyukilabs commented Jan 16, 2018

This PR fixes the "Creating a receiving browsing context" test again.

@ghost
Copy link

ghost commented Jan 16, 2018

Build PASSED

Started: 2018-01-18 01:52:46
Finished: 2018-01-18 01:56:06

View more information about this build on:

@@ -61,31 +61,43 @@

// Indexed Database
let db;
const storeName = 'store-controlling-ua';
const checkIndexedDB = () => {
if ('indexedDB' in window) {
message = 'Indexed Database is shared by top-level and nested browsing contexts.';
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this required by same-origin policy? Do we need to test this here?

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 intention of this is to check if both top-level and nested browsing contexts share the same storage, according to the following spec described in Creating a receiving browsing context:

All nested browsing contexts created by the presented document, i.e. that have the receiving browsing context as their top-level browsing context, [...] MUST also share the same browsing state (storage) for features 5-10 listed above.

db = null;
let req = indexedDB.open(dbName, 1), store, transaction;
return new Promise((resolve, reject) => {
// The test would fail when the database is already created by the controlling UA
Copy link
Member

Choose a reason for hiding this comment

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

According to the IndexedDB spec, you can test for existence of a database by a non-empty version property on the result object.

https://developer.mozilla.org/en-US/docs/Web/API/IDBDatabase

Perhaps the test would be simpler by just checking whether a database already exists (thus indicating whether the database was leaked across receiving browsing contexts).

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good idea. Regarding IndexedDB, we can simply check version of a database. I'll simplify and submit these codes later.

BTW, I'm afraid that the MDN article might be incorrect. With regard to indexedDB.open, the IndexedDB spec says:

If version is not given and no database with that name exists, a new database will be created with version equal to 1.

Anyway, we can still check whether the database has been previously created or not by looking at version of the database.

@tomoyukilabs
Copy link
Member Author

I've update the codes by simplifying how to check IndexedDB. Now EventWatcher is used here again.

Many thanks, @mfoltzgoogle!

Copy link
Member

@markafoltz markafoltz left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes. However, I don't have write access to this repository.

@tomoyukilabs
Copy link
Member Author

@mfoltzgoogle Thanks for your review and approval.

@tidoust @louaybassbouss Could you review this PR?

Copy link
Contributor

@tidoust tidoust left a comment

Choose a reason for hiding this comment

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

Thanks for the simplification!

@tidoust tidoust merged commit c35e3d1 into web-platform-tests:master Jan 19, 2018
@tomoyukilabs
Copy link
Member Author

@tidoust Thanks!

@tomoyukilabs tomoyukilabs deleted the fix-receiver-create-test branch June 4, 2018 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants