Bug 70890 – ExceptionCode.cpp shouldn't need to know about every feature that throws exceptions

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adam Barth]

ExceptionCode.h shouldn't need to know about every feature that throws exceptions

[Adam Barth]

Created attachment 112472 [details]
Work in progress

[Adam Barth]

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.

[Alexey Proskuryakov]

What is the rationale for this change? Without a good reason, this looks like over-engineering.

[Adam Barth]

> 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.

[Alexey Proskuryakov]

Makes sense to discuss on webkit-dev. Would you be willing to send an e-mail?

[Eric Seidel (no email)]

https://lists.webkit.org/pipermail/webkit-dev/2011-October/018293.html was a thread on this subject.

[Alexey Proskuryakov]

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.

[Adam Barth]

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.

[Alexey Proskuryakov]

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.

[Adam Barth]

Created attachment 112642 [details]
work in progress

[Adam Barth]

Created attachment 112773 [details]
Work in progress

[Eric Seidel (no email)]

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 <[e-mail]>

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.

[Adam Barth]

Good idea.  I'll start a series of bugs blocking this bug.

[Adam Barth]

Created attachment 112827 [details]
Work in progress

[Adam Barth]

Created attachment 112831 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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?

[Adam Barth]

> 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.

[Adam Barth]

Committed r98711: <http://trac.webkit.org/changeset/98711>

[Adam Roben (:aroben)]

Mac build fix in r98720.

[Adam Barth]

> Mac build fix in r98720.

Thanks Adam.