Upstream an assortment of IndexedDB tests to WPT.

Upstream an assortment of IndexedDB tests to WPT.

BUG=683463
Review-Url: https://codereview.chromium.org/2659513002
Cr-Commit-Position: refs/heads/master@{#446901}
Committed: https://chromium.googlesource.com/chromium/src/+/39c1cbc54f0ae2cebe8245d848f649bf3e1fb809

[pwnall, 2017-01-26 02:20:14]

Description was changed from

==========
Upstream an assortment of IndexedDB tests to WPT.

These tests cover functionality introduced by the IndexedDB 2.0
specification, and need to be upstreamed before we can get a TAG review.

BUG=683463
==========

to

==========
Upstream an assortment of IndexedDB tests to WPT.

BUG=683463
==========

[pwnall, 2017-01-26 02:20:25]

Patchset #1 (id:1) has been deleted

[pwnall, 2017-01-26 02:27:35]

pwnall@chromium.org changed reviewers:
+ jsbell@chromium.org

[pwnall, 2017-01-26 02:27:35]

PTAL?

The tests are straightforward upstreams, except for the fact that indexeddb_test
is slightly different in WPT's support.js and in Blink's testharness-support.js.
Specifically, the first callback's arguments are (t, db, txn) in WPT, and (t,
db, req) in testharness-helpers.js.

I suspect that Blink's version is newer, because passing in the request is
necessary for tests that want to abort the transaction, as they need to null out
the request's onerror handler.

Instead of going for the same (leaky, IMHO) approach here, I monkey-patch
IDBTransaction so that calling its abort() nulls out the onerror handler.

I'll openly admit that my main motivation for going this route is that I'm
trying to avoid sifting through all the WPT tests that use indexeddb_test, and
I'm using the leaky argument because it happens to fit my goal.

Are you OK with this approach, or would you like me to switch indexeddb_test in
WPT to match the one in Blink?

[pwnall, 2017-01-26 02:27:36]

PTAL?

The tests are straightforward upstreams, except for the fact that indexeddb_test
is slightly different in WPT's support.js and in Blink's testharness-support.js.
Specifically, the first callback's arguments are (t, db, txn) in WPT, and (t,
db, req) in testharness-helpers.js.

I suspect that Blink's version is newer, because passing in the request is
necessary for tests that want to abort the transaction, as they need to null out
the request's onerror handler.

Instead of going for the same (leaky, IMHO) approach here, I monkey-patch
IDBTransaction so that calling its abort() nulls out the onerror handler.

I'll openly admit that my main motivation for going this route is that I'm
trying to avoid sifting through all the WPT tests that use indexeddb_test, and
I'm using the leaky argument because it happens to fit my goal.

Are you OK with this approach, or would you like me to switch indexeddb_test in
WPT to match the one in Blink?

[pwnall, 2017-01-26 02:27:47]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-26 02:28:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 03:47:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-26 03:47:13]

Dry run: This issue passed the CQ dry run.

[jsbell, 2017-01-26 19:27:30]

I think monkeypatching the API being tested is not a good idea, since the
behavior of abort() in the test will now be unexpectedly different in a very
subtle way.

Why do you describe passing the request as leaky?

https://codereview.chromium.org/2659513002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbdatabase-createObjectStore-exception-order.htm
(right):

https://codereview.chromium.org/2659513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbdatabase-createObjectStore-exception-order.htm:35:
txn.onerror = null;
Is txn.onerror set or was this an accidental conversion?

[pwnall, 2017-01-27 00:01:52]

Thank you very much for the quick feedback!

Also, sorry for the unrelated support.js diffs -- there's a new presubmit check
that forced me to run "git cl format third_party/WebKit", which caused all the
style changes.

https://codereview.chromium.org/2659513002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbdatabase-createObjectStore-exception-order.htm
(right):

https://codereview.chromium.org/2659513002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/idbdatabase-createObjectStore-exception-order.htm:35:
txn.onerror = null;
On 2017/01/26 19:27:29, jsbell wrote:
> Is txn.onerror set or was this an accidental conversion?

Accidental. Thanks for catching this!

[pwnall, 2017-01-27 00:04:58]

PTAL?

Also, documenting the results of an in-person conversation.

On 2017/01/26 19:27:30, jsbell wrote:
> I think monkeypatching the API being tested is not a good idea, since the
> behavior of abort() in the test will now be unexpectedly different in a very
> subtle way.

indexeddb_test got an optional "options" argument. If will_abort is true, the
test is expected to call abort() on the IDBTransaction in the database migration
callback. 

> 
> Why do you describe passing the request as leaky?

I was unclear. Thanks for forcing me to explain this more clearly!

In Blink, tests receive the request because they're responsible for setting
request.onabort to null if they call abort() on the transaction. I think this
leaks an indexeddb_test() implementation detail. The will_abort option fixes
this problem without introducing the monkey-patching issue described above.

[pwnall, 2017-01-27 00:05:06]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 00:08:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2017-01-27 00:31:30]

Now that I'm seeing it in context, maybe 'upgrade_will_abort' to make it clear
what the option refers to?

https://codereview.chromium.org/2659513002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js (right):

https://codereview.chromium.org/2659513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:67: un this.db
= e.target.result;
un -> editing typo?

https://codereview.chromium.org/2659513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:113: if
(!(options && options.will_abort)) {
Aside: one nice way to handle an options argument in a self-documenting way is
to say near the top:

options = Object.assign({will_abort: false}, options);

This produces a new object with default properties assigned per the first
argument, overwritten with anything from the passed |options|, ignoring
null/undefined.

https://codereview.chromium.org/2659513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:122: tx.abort =
function() {
we don't want to monkey patch here any more, do we?

[pwnall, 2017-01-27 01:05:16]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-27 01:06:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pwnall, 2017-01-27 01:23:02]

The CQ bit was checked by pwnall@chromium.org to run a CQ dry run

[pwnall, 2017-01-27 01:23:50]

Wow, sorry I rushed through this, and thanks for putting up with my careless
patch :(
PTAL?

https://codereview.chromium.org/2659513002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js (right):

https://codereview.chromium.org/2659513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:67: un this.db
= e.target.result;
On 2017/01/27 00:31:29, jsbell wrote:
> un -> editing typo?

Done.
Thank you :(
This somehow crawled in after I ran tests locally.

https://codereview.chromium.org/2659513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:113: if
(!(options && options.will_abort)) {
On 2017/01/27 00:31:29, jsbell wrote:
> Aside: one nice way to handle an options argument in a self-documenting way is
> to say near the top:
> 
> options = Object.assign({will_abort: false}, options);
> 
> This produces a new object with default properties assigned per the first
> argument, overwritten with anything from the passed |options|, ignoring
> null/undefined.

Done.
I didn't know this pattern. Thank you very much!

https://codereview.chromium.org/2659513002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/external/wpt/IndexedDB/support.js:122: tx.abort =
function() {
On 2017/01/27 00:31:29, jsbell wrote:
> we don't want to monkey patch here any more, do we?

Done.
Sorry for the carelessness :(

[commit-bot: I haz the power, 2017-01-27 01:24:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 03:31:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 03:31:09]

Dry run: This issue passed the CQ dry run.

[jsbell, 2017-01-27 17:27:31]

lgtm

[pwnall, 2017-01-27 22:34:20]

The CQ bit was checked by pwnall@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:35:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-27 22:46:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-27 22:46:54]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pwnall, 2017-01-28 02:48:48]

The CQ bit was checked by pwnall@chromium.org

[pwnall, 2017-01-28 02:48:49]

The patchset sent to the CQ was uploaded after l-g-t-m from jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/2659513002/#ps100001
(title: "Rebased MANIFEST.json")

[commit-bot: I haz the power, 2017-01-28 02:49:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 03:55:51]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485571728602970,
"parent_rev": "d1351ea096f7b4c2c74df84c75e8666d80c9831f", "commit_rev":
"39c1cbc54f0ae2cebe8245d848f649bf3e1fb809"}

[commit-bot: I haz the power, 2017-01-28 03:56:24]

Description was changed from

==========
Upstream an assortment of IndexedDB tests to WPT.

BUG=683463
==========

to

==========
Upstream an assortment of IndexedDB tests to WPT.

BUG=683463

Review-Url: https://codereview.chromium.org/2659513002
Cr-Commit-Position: refs/heads/master@{#446901}
Committed:
https://chromium.googlesource.com/chromium/src/+/39c1cbc54f0ae2cebe8245d848f6...
==========

[commit-bot: I haz the power, 2017-01-28 03:56:25]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/39c1cbc54f0ae2cebe8245d848f6...