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

Set request result/done flag appropriately in open/delete #202

Merged
merged 3 commits into from May 17, 2017

Conversation

inexorabletash
Copy link
Member

For #161:

  • When an open or delete succeeds the done flag should be set.
  • When an open fails aborts, the request's result should be cleared.

I need to verify we have WPT coverage for this.

@brettz9 - can you review?

@inexorabletash
Copy link
Member Author

WPT coverage was lacking! Fortunately, Chrome and Firefox already pass.

New tests: web-platform-tests/wpt#5899

brettz9 added a commit to brettz9/IndexedDBShim that referenced this pull request May 12, 2017
@brettz9
Copy link
Contributor

brettz9 commented May 12, 2017

I think this PR covers it well...

However, besides if you wanted to add that invisible step to unset the done flag for successful transactions, there is the case also of result always needing to return undefined (per the result getter) when there is an error, so I don't know if you wanted to make that explicit for open and deleteDatabase.

For example, for the following in the steps to open a database, "If connection was closed, create and return a newly created "AbortError" DOMException and abort these steps.", this isn't followed by setting the result to undefined (as now is the case with your PR if the steps to abort are triggered). And perhaps this could also occur with other errors of the database (like UnknownError) that didn't cause an abort.

I think mention of setting the request result to undefined could be added close to the end of open() and deleteDatabase() when the request's error properties are set.

@inexorabletash
Copy link
Member Author

Thanks!

...if you wanted to add that invisible step to unset the done flag..

Rather than doing this I updated the non-normative comment so it doesn't claim the done flag will be unset in all cases.

...If connection was closed... this isn't followed by setting the result to undefined

Great catch! I addressed this as you suggested...

.. setting the request result to undefined could be added close to the end of open() and deleteDatabase() when the request's error properties are set.

... but only for open(), since deleteDatabase() doesn't have a path where the request would be "done" and get a result/error except at the very end.

@brettz9
Copy link
Contributor

brettz9 commented May 16, 2017

... but only for open(), since deleteDatabase() doesn't have a path where the request would be "done" and get a result/error except at the very end.

Oh yes... I c...there will be nothing to clear...

So considering the result getter's definition:

The result attribute’s getter must throw an "InvalidStateError" DOMException if the done flag is unset. Otherwise, the attribute’s getter must return the result of the request, or undefined if the request resulted in an error.

...upon a deleteDatabase error, there will be no InvalidStateError error on retrieval of the result property as the done flag has been set by the end of deleteDatabase, and the getter is already spec'd to return undefined if there is an error, so there is no need to explicitly set it (with nothing prior on the delete request to overwrite)...

@inexorabletash
Copy link
Member Author

inexorabletash commented May 16, 2017

Yes. It could be made a bit more rigorous, as "if the request resulted in an error" is bit of action-at-a-distance. But for this PR... everything look good?

@brettz9
Copy link
Contributor

brettz9 commented May 16, 2017

Yup, all good, thanks!

inexorabletash added a commit to web-platform-tests/wpt that referenced this pull request May 17, 2017
…5899)

Tests for cases in w3c/IndexedDB#202 that weren't obviously covered already - the properties of the request (readyState, result, error) during upgradeneeded, after transaction complete/abort, and after success/error.

Some assertions were flaky in Chrome (details in https://bugs.chromium.org/p/chromium/issues/detail?id=723846) and are disabled here.
@inexorabletash inexorabletash merged commit e213bcf into master May 17, 2017
@inexorabletash inexorabletash deleted the openrq-done-flag branch September 26, 2017 18:18
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.

None yet

2 participants