Bug 88690 – IndexedDB: Error codes, phase two

• Component: New Bugs
• Status: RESOLVED FIXED

[Alec Flett]

IndexedDB: Error codes, phase two

[Alec Flett]

Created attachment 146655 [details]
Patch

[Alec Flett]

jsbell@ / dgrogan@ - sanity check?

This is a few small things that span a lot of files. They're all kind of interconnected, and would be a mess to try to decouple:
- fix tests that try to create transactions in the setversion transaction (removal of that, along with a patch that verifies that you CANNOT do that, is coming up when this lands)
- removes bit about I/O errors and error codes - spec now says UNKNOWN_ERR is correct
- moves some readonly checks into the frontend, and puts an assert in the backend - this helps reduce RPCs as well as consolidates error checks (otherwise, trying to implement which exception takes precedence over other exceptions is near-impossible)

[Alec Flett]

Created attachment 146899 [details]
Patch

[Joshua Bell]

Comment on attachment 146899 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146899&action=review

Overall lgtm, just nits.

> Source/WebCore/ChangeLog:17
> +        already correct.

Need to close the parentheses.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:209
> +        ec = IDBDatabaseException::READ_ONLY_ERR;

FYI, in one of my in-progress patches I added IDBTransaction::isVersionChange() as a convenience. Maybe add IDBTransaction::isReadOnly() ?

> LayoutTests/storage/indexeddb/mozilla/index-prev-no-duplicate.html:85
> +function createObjectStore(request)

The onsuccess callback parameter is an event, not a request. This is also not necessary as the request object from line 80 is global, and the eval in line 89 has a global context - the parameter would not be visible to it anyway.

> LayoutTests/storage/indexeddb/mozilla/index-prev-no-duplicate.html:107
> +    }

Should have a ; after the brace.

> LayoutTests/storage/indexeddb/resources/cursor-advance.js:51
> +function createObjectStore(request)

Ditto.

[Alec Flett]

Created attachment 146955 [details]
Patch

[Alec Flett]

tony@ - r? cq?

[Tony Chang]

Comment on attachment 146955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146955&action=review

> Source/WebCore/Modules/indexeddb/IDBTransaction.h:73
> +    bool isReadOnly() const { return m_mode == READ_ONLY; };

Nit: The semicolon at the end isn't necessary.

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.h:79
> +    const unsigned short m_mode;

Nit: Can we change this from unsigned short to Mode in a follow up change?

[Alec Flett]

Created attachment 147114 [details]
Patch

[Alec Flett]

Comment on attachment 147114 [details]
Patch

tony@ or jsbell@ - cq?
(nit addressed - we're cleaning up the whole mode thing over a series of patches, and I'll try to make the enum thing part of that)

[WebKit Review Bot]

Comment on attachment 147114 [details]
Patch

Rejecting attachment 147114 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12946582

[Alec Flett]

Comment on attachment 147114 [details]
Patch

sorry tony@ - one more try? At least my committer paperwork is in the mail :)

[Tony Chang]

(In reply to comment #10)
> (From update of attachment 147114 [details])
> Rejecting attachment 147114 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> 
> ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://queues.webkit.org/results/12946582

You should be able to use "webkit-patch land-safely" when you have r+ now.  It will fill in the reviewer name, upload the patch and set cq?.

[WebKit Review Bot]

Comment on attachment 147114 [details]
Patch

Clearing flags on attachment: 147114

Committed r120114: <http://trac.webkit.org/changeset/120114>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.