ExceptionCode.h shouldn't need to know about every feature that throws exceptions
Created attachment 112472 [details] Work in progress
My plan is to add InFilesCompiler.pm in this patch and then refactor the event code generators to use InFilesCompiler.pm in a followup patch.
What is the rationale for this change? Without a good reason, this looks like over-engineering.
> What is the rationale for this change? Without a good reason, this looks like over-engineering. The goal is to reduce the cost the project pays for implementing new features by reducing the number of #ifdefs features impose on the rest of WebCore. I'm happy to discuss more on webkit-dev if you'd like, but I'd rather not discuss further on this bug.
Makes sense to discuss on webkit-dev. Would you be willing to send an e-mail?
https://lists.webkit.org/pipermail/webkit-dev/2011-October/018293.html was a thread on this subject.
Unless I missed some of the e-mails, that thread was not discussing anything close to this patch. Of course, that work was undoubtedly an inspiration.
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/ExceptionCode.cpp#L259 has the same problem as Event.cpp used to have. Namely, every feature needs to "register" itself with ExceptionCode.cpp if it wants to use exceptions, leading to a nest of ifdefs that grows in complexity as WebCore adds features. Anyway, if you have feedback on this work, please feel encouraged to reply to the thread on webkit-dev.
I can see some value in generating parts of ExceptionCode.cpp. Whoever reviews the final patch will need to clearly think about whether it's an overall improvement or not, not just about technical correctness. Looking at ExceptionCode.h alone, replacing conditional blocks with conditional lines in ExceptionCodeDescription.in would be useless, of course. Please re-title the bug accordingly, as it isn't about the header alone.
Created attachment 112642 [details] work in progress
Created attachment 112773 [details] Work in progress
Comment on attachment 112773 [details] Work in progress View in context: https://bugs.webkit.org/attachment.cgi?id=112773&action=review Another way to break this down smaller, woudl be to not autogen any of this. Or rathe rjust check in the autogen'd bit. i.e. do the logic-refactor, before doing the autogen of the logic refactor. I think that would make this much easier to follow. > Source/WebCore/bindings/scripts/InFilesCompiler.pm:3 > +#!/usr/bin/perl -w > + > +# Copyright (C) 2011 Adam Barth <abarth@webkit.org> I would do this InFilesCompiler refactor in a first pass-change. > Source/WebCore/dom/DOMCoreException.cpp:38 > +static const char* const exceptionNames[] = { > + "INDEX_SIZE_ERR", > + "DOMSTRING_SIZE_ERR", > + "HIERARCHY_REQUEST_ERR", > + "WRONG_DOCUMENT_ERR", I would do this as a table of structs. which you can easily define this same way. If you don't do that, please add a FIXME about doing so. > Source/WebCore/dom/EventException.h:39 > + { It would be better if you ddid the un-indent of these files in a separate change. git stash your current work. :) > Source/WebCore/dom/make_exception_code_description.pl:179 > + print F "ExceptionCodeDescription::ExceptionCodeDescription(ExceptionCode ec)\n"; > + print F "{\n"; > + > + for my $exceptionType (sort keys %parsedItems) { > + my $conditional = $parsedItems{$exceptionType}{"conditional"}; > + > + print F "#if ENABLE($conditional)\n" if $conditional; When you write your changelog, it woudl be helpful to explain that this code will die, in the next iteration. I also might suggest adding a FIXME here about that plan.
Good idea. I'll start a series of bugs blocking this bug.
Created attachment 112827 [details] Work in progress
Created attachment 112831 [details] Patch
Attachment 112831 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/CMakeLists.tx..." exit_code: 1 Source/WebCore/bindings/js/JSDOMBinding.cpp:217: A case label should not be indented, but line up with its switch statement. [whitespace/indent] [4] Total errors found: 1 in 20 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 112831 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112831&action=review OK. I get the sense you're setting the stage here. > Source/WebCore/dom/ExceptionCode.cpp:37 > + description = ExceptionCodeDescription(ec); Seems you also want to inline this into ExceptionCode. I assume there will be some sort of ExceptionCodeFactory?
> Seems you also want to inline this into ExceptionCode. I assume there will be some sort of ExceptionCodeFactory? Yeah, that function disappears in the next patch.
Committed r98711: <http://trac.webkit.org/changeset/98711>
Mac build fix in r98720.
> Mac build fix in r98720. Thanks Adam.