Bug 102961

Summary: IndexedDB: Remove IDBDatabaseException.idl
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: WebCore Misc.Assignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, alecflett, arv, dgrogan, gtk-ews, gyuyoung.kim, ojan, rakuco, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 98050, 102514, 102726    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Joshua Bell
Reported 2012-11-21 10:31:00 PST
See wkbug.com/102514 for more history, but: * IDBDatabaseException is no longer exposed to script * IDBDatabaseException.idl has not been deleted: DOMExceptions are thrown by using an ExceptionCode to look up an ExceptionCodeDescription which provides name/code/message for the DOMException. This is done via a generated file c/o dom/make_dom_exceptions.pl which processes DOMExceptions.in and makes calls to each listed exception type to allow it to try and decode the ExceptionCode and return an ExceptionCodeDescription. IDBDatabaseException.cpp implements this for IDB-specific exception types, even though they are not instances of an IDBDatabaseException. So without rewriting the DOMException code we need to keep IDBDatabaseException listed in DOMExceptions.in However, the DOMExceptions.in is also used to generate headers used in the binding code - specifically, setDOMException (JSC and V8 variants). Those headers rely on a V8<blah>.h / JS<blah>.h file existing, which is generated off the IDL. So if we have IDBDatabaseEception listed in DOMExceptions.in we need to keep IDBDatabaseException.idl around even though it's not used. ... The "real" fix is probably to do wkbug.com/102726 and possibly wkbug.com/98050 first, and make the means by which an ExceptionCodeDescription is populated a little smarter.
Attachments
Patch (10.86 KB, patch)
2012-11-28 16:28 PST, Joshua Bell 		no flags
DetailsFormatted DiffDiff
Patch (18.21 KB, patch)
2012-11-28 16:42 PST, Joshua Bell 		no flags
DetailsFormatted DiffDiff
Patch for landing (18.66 KB, patch)
2012-12-06 11:38 PST, Joshua Bell 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Joshua Bell
Comment 1 2012-11-28 10:14:54 PST
Going forward, new DOMException types just need a hook for the type to fill it in, not the IDLs. At that point using DOMExceptions.in is overkill. So a simple proposal here - just "hard code" a hook call into IDBDatabaseException::getExceptionCodeDescription() from the code generated by make_dom_exceptions.pm. Over time, additional exception generators would need similar hooks, but they would be present as a couple of lines in only that one file, so autogenerating them from an InFile is overkill.
Joshua Bell
Comment 2 2012-11-28 16:28:58 PST
Created attachment 176598 [details] Patch
Joshua Bell
Comment 3 2012-11-28 16:33:03 PST
Straw-man approach attached. An alternate approach would be to keep IDBDatabaseException in DOMExceptions.in but marked with another property (e.g. "noInterface") and filter those out of $parsedItemsRef when generateHeader(), and $InCompiler->generateXXXHeader() are called. We can also just punt on this until a glorious future like https://bugs.webkit.org/show_bug.cgi?id=98050 arrives and we can throw DOMExceptions by name/description rather than having these initializeDescription() hooks.
kov's GTK+ EWS bot
Comment 4 2012-11-28 16:37:46 PST
Comment on attachment 176598 [details] Patch Attachment 176598 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/15010701
Joshua Bell
Comment 5 2012-11-28 16:42:50 PST
Created attachment 176604 [details] Patch
Erik Arvidsson
Comment 6 2012-12-05 07:52:26 PST
Comment on attachment 176604 [details] Patch Looks good to me but I'm not yet a reviewer
Joshua Bell
Comment 7 2012-12-05 08:21:15 PST
abarth@ - thoughts? land this, go for something fancier, or just wait?
Adam Barth
Comment 8 2012-12-05 17:49:06 PST
Comment on attachment 176604 [details] Patch I don't love this, but perhaps we should walk this path for a while and then refactor when a pattern emerges.
Adam Barth
Comment 9 2012-12-05 17:50:23 PST
Comment on attachment 176604 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176604&action=review > Source/WebCore/dom/make_dom_exceptions.pl:180 > + print F "#if ENABLE(INDEXED_DATABASE)\n"; > + print F " if (IDBDatabaseException::initializeDescription(ec, this))\n"; > + print F " return;\n"; > + print F "#endif\n"; Would you be willing to add a FIXME comment about us not really wanting to special-case IDB here? We want to guide people who work on this code next that we'd like the code generator to be general-purpose. We're just not sure what the general case looks like yet because we only have one example.
Joshua Bell
Comment 10 2012-12-06 11:38:37 PST
Created attachment 178045 [details] Patch for landing
Joshua Bell
Comment 11 2012-12-06 11:42:00 PST
(In reply to comment #9) > Would you be willing to add a FIXME comment about us not really wanting to special-case IDB here? I'd be thrilled to. (in patch for landing) Searching around, it looks like there's been some discussion on W3C and Mozilla bug trackers around removing types such as SVGException and XPathException as well, not sure if those initiatives have made it very far.
Erik Arvidsson
Comment 12 2012-12-06 11:55:20 PST
(In reply to comment #11) > Searching around, it looks like there's been some discussion on W3C and Mozilla bug trackers around removing types such as SVGException and XPathException as well, not sure if those initiatives have made it very far. FYI, RangeException and XMLHttpRequestException have been removed from the specs too. Bug 102515 removes RangeException (waiting for review, hint hint)
WebKit Review Bot
Comment 13 2012-12-06 12:12:37 PST
Comment on attachment 178045 [details] Patch for landing Clearing flags on attachment: 178045 Committed r136869: <http://trac.webkit.org/changeset/136869>
WebKit Review Bot
Comment 14 2012-12-06 12:12:42 PST
All reviewed patches have been landed. Closing bug.
Erik Arvidsson
Comment 15 2012-12-06 14:45:26 PST
What is the plan for removing the .h and .cpp files? Shouldn't these exceptions just be DOMExceptions?
Joshua Bell
Comment 16 2012-12-06 14:53:52 PST
(In reply to comment #15) > What is the plan for removing the .h and .cpp files? Shouldn't these exceptions just be DOMExceptions? I don't have a plan yet. Since we want custom messages it will depend on how https://bugs.webkit.org/show_bug.cgi?id=98050 shakes out. The WebCore::IDBDatabaseException type is unused - IDBDatabaseException::create should be removed (and I would have done it if I'd even noticed it was still around); those files currently just implement an (overly elaborate) way to map the enum value to a name/message/code.
Note You need to log in before you can comment on or make changes to this bug.