We should be using DOMException instead
arv@ - did you have any specific implementation thoughts here, or is this just a tracking bug?
My first thought was to just change to DOMException but it seems we should use DOMError for idb?
There are a couple places where DOMError is used (IDBRequest.error) for asynchronous error reporting, but most synchronous error handling in the spec does just have methods throw DOMExceptions. There are IDB-specific exception types, which in WebIDL/DOM4/spec parlance are e.g. "throw a DOMException of type ConstraintError". Right now we use IDBDatabaseException.CONSTRAINT_ERR like all of the legacy modules do, and there are various layers of indirection that map that to a DOMException with code = CONSTRAINT_ERR and name = "ConstraintError", but it would be nice to simplify that. To align with the spec: * we shouldn't be exposing the IDBDatabaseException interface * the codes should be 0 for IDB-specific exception types We'd like to do the above in the next couple of weeks - the big time sink will be updating tests.
Created attachment 175067 [details] Patch
alecflett@, dgrogan@ - general sanity check? abarth@ - could you take a look at the binding "InFile" changes around DOMException? NOTE: This can't land until some tests on the Chromium side have been disabled, as they use IDBDatabaseException from script.
Comment on attachment 175067 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175067&action=review > Source/WebCore/bindings/scripts/InFilesCompiler.pm:207 > + my $noInterface = $parsedItems{$itemName}{"noInterface"}; > + next if $noInterface; This is the wrong layer at which to process this property. You should teach the exception specific backends about noInterface.
(In reply to comment #6) > (From update of attachment 175067 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175067&action=review > > > Source/WebCore/bindings/scripts/InFilesCompiler.pm:207 > > + my $noInterface = $parsedItems{$itemName}{"noInterface"}; > > + next if $noInterface; > > This is the wrong layer at which to process this property. You should teach the exception specific backends about noInterface. Hrm. This is the code generating the binding code's ExceptionHeaders.h file which is essentially a list of lines such as #include "V8IDBDatabaseException.h"; with the rest of this change that file doesn't (and shouldn't) exist. Basically, I need to make IDBDatabaseException only "visible" to the files make_dom_exceptions.pl generates directly (ExceptionCodeDescription.h/.cpp). Several approaches occur to me: * make_dom_exceptions.pl's generateCode() filter out "noInterface" items before calling into the InFileCompiler's methods, by mutating $parsedItemsRef. * make this more abstract by implementing a variant of conditional without the requirement of being wrapped in USE(). Seems like overkill. * leave the IDBDatabaseException.idl around but simply not expose it via document/worker (leaving the clean-up for later) Does the first seem reasonable?
Comment on attachment 175067 [details] Patch So, the problem is that there is an IDBDatabaseException.h but no IDBDatabaseException.idl... You probably need to restructure how make_dom_exceptions.pl works. It was written with the assumption that there is a one-to-one correspondence between the CPP and IDL worlds.
Why does DOMExceptions.in need to list IDBDatabaseException at all if there isn't an IDL interface for it?
Looks like the output of this in file is only used in V8ThrowException::setDOMException and the JSC bindings' setDOMException.
(In reply to comment #9) > Why does DOMExceptions.in need to list IDBDatabaseException at all if there isn't an IDL interface for it? When an ExceptionCode is being translated into a DOMException, to assign a name and description, a generated list of static methods are called with the code and can create the description or indicate it's not handled. See gen/webkit/ExceptionCodeDescription.cpp ExceptionCodeDescription::ExceptionCodeDescription Even though IDBDatabaseException is being removed as an IDL interface it still needs to participate in this description mechanism. (Ideally we'd replace ExceptionCode with a class that allows assigning a name/description directly.)
Maybe we should do bug 102726 first? Then we can add the IDB exceptions to that and map the code to 0 for these?
(In reply to comment #12) > Maybe we should do bug 102726 first? Then we can add the IDB exceptions to that and map the code to 0 for these? If we think that will happen soon I can remove the "tricky" bits of this patch, which I was thinking of doing anyway - leave the IDL in place even though it will disappear from script. The tests can still all be updated, which will help validate 102726.
I'm not sure what the best thing to do here is. You're breaking a basic assumption that the InFileCompiler makes about how things work. It's likely that you'll want to significantly restructure things. Teaching the InFileCompiler.pm about noInterface seems a bit like papering over the structural part of this change.
(In reply to comment #14) > Teaching the InFileCompiler.pm about noInterface seems a bit like papering over the structural part of this change. I completely agree. See my suggestion above: "make_dom_exceptions.pl's generateCode() filter out "noInterface" items before calling into the InFileCompiler's methods, by mutating $parsedItemsRef." - that would remove any changes to InFileCompiler, all changes would be localized to make_dom_exceptions ... but arv's suggestion is the end-goal anyway. I'll upload a new patch soonish.
Created attachment 175089 [details] Patch
New patch just does the "easy" part and hides the interface from script, updates the codes, and all the tests. It leaves the cleanup for later. Still need to disable some Chromium tests before landing, though.
Comment on attachment 175089 [details] Patch lgtm Do all evalAndExpectException calls now have "0" as the second parameter? If so, evalAndExpectException is IDB-specific, we could remove that parameter.
(In reply to comment #18) > (From update of attachment 175089 [details]) > lgtm > > Do all evalAndExpectException calls now have "0" as the second parameter? If so, evalAndExpectException is IDB-specific, we could remove that parameter. Some places throw DOMExceptions with codes so they should be tested for. We could make the expectation of 0 "quiet" though.
(In reply to comment #19) > > Do all evalAndExpectException calls now have "0" as the second parameter? If so, evalAndExpectException is IDB-specific, we could remove that parameter. > > Some places throw DOMExceptions with codes so they should be tested for. Yeah, 147 places look for "0" now, 143 places look for a DOMException constant. > We could make the expectation of 0 "quiet" though. ... but I'd suggest doing that later.
Created attachment 175233 [details] Patch
Should add tests for these to storage/indexeddb/removed.html: shouldBeFalse("'IDBDatabaseException' in self"); shouldBeFalse("'errorCode' in indexeddb.open('')");
Created attachment 175258 [details] Patch
Okay, Chromium side is out of the way, and this is reduced to just IDB code/test changes. r? anyone?
Comment on attachment 175258 [details] Patch rs=me There's still some more clean up to do here to remove the IDBException interface from the codebase.
Comment on attachment 175258 [details] Patch Rejecting attachment 175258 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: e-flag-expected.txt patching file LayoutTests/storage/indexeddb/transaction-basics-expected.txt patching file LayoutTests/storage/indexeddb/transaction-complete-workers-expected.txt patching file LayoutTests/storage/indexeddb/transaction-error-expected.txt patching file LayoutTests/storage/indexeddb/transaction-read-only-expected.txt Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue Full output: http://queues.webkit.org/results/14961026
Created attachment 175481 [details] Patch for landing
Comment on attachment 175481 [details] Patch for landing Clearing flags on attachment: 175481 Committed r135424: <http://trac.webkit.org/changeset/135424>
All reviewed patches have been landed. Closing bug.