Bug 70890 - ExceptionCode.cpp shouldn't need to know about every feature that throws exceptions
Summary: ExceptionCode.cpp shouldn't need to know about every feature that throws exce...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on: 71075 71081 71094
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-26 02:18 PDT by Adam Barth
Modified: 2011-10-28 11:06 PDT (History)
6 users (show)

See Also:


Attachments
Work in progress (11.42 KB, patch)
2011-10-26 02:19 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
work in progress (21.72 KB, patch)
2011-10-26 23:51 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Work in progress (94.07 KB, patch)
2011-10-27 15:48 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Work in progress (18.12 KB, patch)
2011-10-28 01:10 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (30.41 KB, patch)
2011-10-28 01:32 PDT, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-10-26 02:18:20 PDT
ExceptionCode.h shouldn't need to know about every feature that throws exceptions
Comment 1 Adam Barth 2011-10-26 02:19:08 PDT
Created attachment 112472 [details]
Work in progress
Comment 2 Adam Barth 2011-10-26 02:20:01 PDT
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.
Comment 3 Alexey Proskuryakov 2011-10-26 12:23:52 PDT
What is the rationale for this change? Without a good reason, this looks like over-engineering.
Comment 4 Adam Barth 2011-10-26 13:38:11 PDT
> 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.
Comment 5 Alexey Proskuryakov 2011-10-26 14:03:12 PDT
Makes sense to discuss on webkit-dev. Would you be willing to send an e-mail?
Comment 6 Eric Seidel (no email) 2011-10-26 14:06:01 PDT
https://lists.webkit.org/pipermail/webkit-dev/2011-October/018293.html was a thread on this subject.
Comment 7 Alexey Proskuryakov 2011-10-26 14:10:18 PDT
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.
Comment 8 Adam Barth 2011-10-26 14:49:26 PDT
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.
Comment 9 Alexey Proskuryakov 2011-10-26 16:11:33 PDT
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.
Comment 10 Adam Barth 2011-10-26 23:51:10 PDT
Created attachment 112642 [details]
work in progress
Comment 11 Adam Barth 2011-10-27 15:48:36 PDT
Created attachment 112773 [details]
Work in progress
Comment 12 Eric Seidel (no email) 2011-10-27 16:04:35 PDT
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.
Comment 13 Adam Barth 2011-10-27 16:13:17 PDT
Good idea.  I'll start a series of bugs blocking this bug.
Comment 14 Adam Barth 2011-10-28 01:10:22 PDT
Created attachment 112827 [details]
Work in progress
Comment 15 Adam Barth 2011-10-28 01:32:07 PDT
Created attachment 112831 [details]
Patch
Comment 16 WebKit Review Bot 2011-10-28 01:33:28 PDT
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 17 Eric Seidel (no email) 2011-10-28 01:53:24 PDT
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?
Comment 18 Adam Barth 2011-10-28 01:54:57 PDT
> 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.
Comment 19 Adam Barth 2011-10-28 02:11:56 PDT
Committed r98711: <http://trac.webkit.org/changeset/98711>
Comment 20 Adam Roben (:aroben) 2011-10-28 05:38:01 PDT
Mac build fix in r98720.
Comment 21 Adam Barth 2011-10-28 11:06:36 PDT
> Mac build fix in r98720.

Thanks Adam.