Bug 63688 – Refactoring: ExceptionCode value which never become non-zero should be replaced with NeverThrown

• Component: DOM
• Status: RESOLVED FIXED

[Hajime Morrita]

This refactoring will introduce a temporal object which replaces
our tedious pattern like this:

ExceptionCode ec = 0;
weKnowThisNeverFail(ec);
ASSERT(!ec);

We can have a helper class like this:
weKnowThisNeverFail(NeverThrown());

[Hajime Morrita]

Created attachment 99248 [details]
Patch

[Hajime Morrita]

I wonder if this matches the WebKit taste. But we should get bored to write ExceptionCode ec =.

[Eric Seidel (no email)]

Comment on attachment 99248 [details]
Patch

We could even make an enum which auto-converts to this class to get rid of the ().

[Hajime Morrita]

Created attachment 99252 [details]
Patch

[Hajime Morrita]

> We could even make an enum which auto-converts to this class to get rid of the ().
Tried but compiler didn't allow me to do it.
It looks because we need to level of conversion:
NeverThrown(enum) -> NeverThrownWapper(class) -> ExceptionCode&
The compiler cannot find this type of conversion. for type matching.

[e-mail] also suggest that this kind of magic should be visually explicit.
So I just adopt a macro called NEVER_THROWN.

What do you think?

[Darin Adler]

Comment on attachment 99252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99252&action=review

> Source/WebCore/dom/NeverThrown.h:33
> +#include "ExceptionCode.h"

It seems a shame to include this just for the typedef. We often just do typedef int ExceptionCode elsewhere, and I think we could do that here as well.

> Source/WebCore/dom/NeverThrown.h:39
> +class NeverThrownWrapper {

I think we need a name that is more specific. I never talk about “throwing” exceptions myself, even though that’s the name in JavaScript. I think the sense we want for the name is not “an exception that is never thrown”, but rather “a place where we know an exception can never happen”. This is sort of an assertion that an exception will not occur. I think perhaps NoExceptionAssertion is the right name for the class. Or maybe someone can shorten that.

> Source/WebCore/dom/NeverThrown.h:50
> +    : m_code(0)

The initialization to zero is only needed if exceptions are enabled. I suggest putting it in an ifdef.

> Source/WebCore/dom/NeverThrown.h:56
> +    ASSERT(!m_code);

When this assertion does fire in a build not running under a debugger, is it easy to figure out which line of code threw the exception?

> Source/WebCore/dom/NeverThrown.h:59
> +#define NEVER_THROWN NeverThrownWrapper()

I don’t think we really need a macro for this. I liked the way you had it before in the first patch better.

> Source/WebCore/html/HTMLDetailsElement.cpp:161
> +        defaultSummary->appendChild(Text::create(document(), defaultDetailsSummaryText()), NEVER_THROWN);

This is much better than constantly passing ec, but I think that for most cases it would be even better to have functions without the ExceptionCode& argument. We need the ExceptionCode& argument to communicate exceptions to the DOM bindings for the DOM itself, but outside of that I think we should strive to leave out these ExceptionCode& arguments.

And for basic functions like appendChild that are in the DOM but also need to be used extensively inside WebCore, we should have alternate versions, either overloads or separate functions, for internal use that do not take ExceptionCode&.

So the examples you changed in this patch would *not* be ones where I would use the new class, but rather where I would use a new appendChild. I would eliminate the boolean argument in the non-ExceptionCode function, and have both an overload of appendChild without ExceptionCode& and a new appendChildLazyAttach without an ExceptionCode& argument.

You can see an example of this in Element::setAttribute.

[Hajime Morrita]

Created attachment 99440 [details]
Patch

[Kent Tamura]

Comment on attachment 99440 [details]
Patch

r- because of missing IgnoringExceptionCode.h.

[Hajime Morrita]

Created attachment 99442 [details]
Patch

[Hajime Morrita]

Comment on attachment 99252 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99252&action=review

Hi Darin, thank you for the feedback! I addressed points in the updated patch.

>> Source/WebCore/dom/NeverThrown.h:33
>> +#include "ExceptionCode.h"
> 
> It seems a shame to include this just for the typedef. We often just do typedef int ExceptionCode elsewhere, and I think we could do that here as well.

Well,  replaced it with a typedef.

>> Source/WebCore/dom/NeverThrown.h:39
>> +class NeverThrownWrapper {
> 
> I think we need a name that is more specific. I never talk about “throwing” exceptions myself, even though that’s the name in JavaScript. I think the sense we want for the name is not “an exception that is never thrown”, but rather “a place where we know an exception can never happen”. This is sort of an assertion that an exception will not occur. I think perhaps NoExceptionAssertion is the right name for the class. Or maybe someone can shorten that.

I noticed that in many case the passed ExceptionCode is not (and should not) checked at all. 
So I splited the class into 2: IgnoringExceptionCode for non-checked one (which is the default) and 
one with assertion.

>> Source/WebCore/dom/NeverThrown.h:50
>> +    : m_code(0)
> 
> The initialization to zero is only needed if exceptions are enabled. I suggest putting it in an ifdef.

Sure. done.

>> Source/WebCore/dom/NeverThrown.h:56
>> +    ASSERT(!m_code);
> 
> When this assertion does fire in a build not running under a debugger, is it easy to figure out which line of code threw the exception?

Good point. I added the code location information (which is enabled only unless ASSETION_DISABLED

>> Source/WebCore/dom/NeverThrown.h:59
>> +#define NEVER_THROWN NeverThrownWrapper()
> 
> I don’t think we really need a macro for this. I liked the way you had it before in the first patch better.

That's true.
On the other hand, we need a macro for capturing the code location.
I define a macro ASSERT_NEVER_THROWN, which is only used where ASSERT was originally used.

>> Source/WebCore/html/HTMLDetailsElement.cpp:161
>> +        defaultSummary->appendChild(Text::create(document(), defaultDetailsSummaryText()), NEVER_THROWN);
> 
> This is much better than constantly passing ec, but I think that for most cases it would be even better to have functions without the ExceptionCode& argument. We need the ExceptionCode& argument to communicate exceptions to the DOM bindings for the DOM itself, but outside of that I think we should strive to leave out these ExceptionCode& arguments.
> 
> And for basic functions like appendChild that are in the DOM but also need to be used extensively inside WebCore, we should have alternate versions, either overloads or separate functions, for internal use that do not take ExceptionCode&.
> 
> So the examples you changed in this patch would *not* be ones where I would use the new class, but rather where I would use a new appendChild. I would eliminate the boolean argument in the non-ExceptionCode function, and have both an overload of appendChild without ExceptionCode& and a new appendChildLazyAttach without an ExceptionCode& argument.
> 
> You can see an example of this in Element::setAttribute.

That's totally true.
The updated patch uses IgnoringExceptionCode as the default value for ExceptionCode parameters.
We now eliminate many existing ExceptionCode usage with doing so.
In many case having a default parameter is more handy than defining alternative version.
In some case an alternative version might be preferable though.

[Hajime Morrita]

(In reply to comment #8)
> (From update of attachment 99440 [details])
> r- because of missing IgnoringExceptionCode.h.
Ouch. Added.

[Early Warning System Bot]

Comment on attachment 99442 [details]
Patch

Attachment 99442 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8970364

[WebKit Review Bot]

Comment on attachment 99442 [details]
Patch

Attachment 99442 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8962754

[WebKit Review Bot]

Comment on attachment 99442 [details]
Patch

Attachment 99442 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8970374

[WebKit Review Bot]

Comment on attachment 99442 [details]
Patch

Attachment 99442 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8963779

[Gustavo Noronha (kov)]

Comment on attachment 99442 [details]
Patch

Attachment 99442 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8959998

[Collabora GTK+ EWS bot]

Comment on attachment 99442 [details]
Patch

Attachment 99442 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/8963775

[WebKit Review Bot]

Comment on attachment 99442 [details]
Patch

Attachment 99442 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/8974063

[Hajime Morrita]

Created attachment 99455 [details]
Fixing release builds

[Darin Adler]

Comment on attachment 99455 [details]
Fixing release builds

View in context: https://bugs.webkit.org/attachment.cgi?id=99455&action=review

This patch changes far too much at once. We need to be careful about where we choose to ignore exception codes and where we choose to check them. Almost all the examples here that are ignoring exceptions should instead be asserting them by default.

Lets do a smaller version of this patch that tackles a much smaller set of functions at first. I know you held back to only functions in the dom directory, but I think this needs more thought for each function.

Specifically, adding a checker to assert there is no exception by default should be the first thing we consider for any given function. Ignoring the exception instead should be done only rarely when that is helpful for a reason specific to that function.

> Source/WebCore/dom/CharacterData.h:33
> -    void setData(const String&, ExceptionCode&);
> +    void setData(const String&, ExceptionCode& = IgnoringExceptionCode());

This should not ignore the exception code by default. The only way we’d get an exception would be if the node was read-only. At the moment that isn’t even implemented!

Generally speaking the default should be to assert the exception code, not ignore it, and this is a perfect example.

> Source/WebCore/dom/CharacterData.h:35
> +    String substringData(unsigned offset, unsigned count, ExceptionCode& = IgnoringExceptionCode());

Since the behavior here is to return the null string if the offset is past the end of the data, I suppose we could default to ignoring the exception. But I would prefer that the default be to check the exception, and the call sites that need to be tolerant can explicitly ask to ignore.

> Source/WebCore/dom/CharacterData.h:36
> +    void appendData(const String&, ExceptionCode& = IgnoringExceptionCode());

Since this will just throw away the new data if the node is read-only, this should assert the exception code, not ignore it.

> Source/WebCore/dom/CharacterData.h:70
> +    void checkCharDataOperation(unsigned offset, ExceptionCode& = IgnoringExceptionCode());

The only reason to call this function is to set the exception code. This should not have a default argument for the ExceptionCode&.

> Source/WebCore/dom/IgnoringExceptionCode.h:39
> +class IgnoringExceptionCode {

The grammar here is not right. Class names should be nouns. Given this name, an object of this class would be an "ignoring exception code"? That’s not good terminology.

I think IgnoredExceptionCode would be an OK name, although it’s a pretty long name. A possibly-better name is ExceptionCodeIgnorer or ExceptionIgnorer.

> Source/WebCore/dom/IgnoringExceptionCode.h:51
> +#if !defined(ASSERT_DISABLED)
> +    : m_code(0)
> +#endif

If we are ignoring the exception code then we don’t want to initialize m_code at all. The only reason we have this here is that this class is not really the “ignored exception code” class, since we plan to derive from it and not ignore the exception code!

There’s no reason t have this unclear design. I’d rather see repeated code.

> Source/WebCore/dom/IgnoringExceptionCode.h:76
> +class NeverThrownWithLocation : public IgnoringExceptionCode {
> +public:
> +    NeverThrownWithLocation(const char*, size_t, const char*);
> +    ~NeverThrownWithLocation();
> +
> +private:
> +    const char* m_file;
> +    size_t m_line;
> +    const char* m_function;
> +};
> +
> +inline NeverThrownWithLocation::NeverThrownWithLocation(const char* file, size_t line, const char* function)
> +    : m_file(file), m_line(line), m_function(function)
> +{
> +}
> +
> +inline NeverThrownWithLocation::~NeverThrownWithLocation()
> +{
> +    ASSERT_AT(!m_code, m_file, m_line, m_function);
> +}
> +
> +}

This entire class should be inside an #if !ASSERT_DISABLED since we don’t want to use it at all in that case.

This is a misuse of inheritance. A derived class should not contradict something from a base class. The classic example is that ellipse should not inherit from circle, because an ellipse is not a circle. So having a class that asserts an exception code is zero be a subclass of something called “ignored exception code” is not good design.

A class name should be a noun phrase describing the object. The name "never thrown with location" is not good because it’s not sensible to say “this object is a never thrown with location”.

I would call the class something like ExceptionCodeAssertionChecker. Or NoExceptionAssertionChecker.

> Source/WebCore/dom/IgnoringExceptionCode.h:82
> +#if ASSERT_DISABLED
> +#define ASSERT_NEVER_THROWN WebCore::IgnoringExceptionCode()
> +#else
> +#define ASSERT_NEVER_THROWN WebCore::NeverThrownWithLocation(__FILE__, __LINE__, WTF_PRETTY_FUNCTION)
> +#endif

I am not fond of “never thrown” as a way to say there is no exception. I would prefer ASSERT_NO_EXCEPTION.

It’s neat that we can tell you exactly where the exception occurred if you use the macro at the call site, but generally I don’t think callers should be thinking about this at all. I think that most of the DOM functions should default to asserting there is no exception. There are very few DOM functions where we’d want to call them internally and actually ignore the exception.

[Darin Adler]

I commented on the first few functions where the ignorer was added. I didn’t try to review all of them at this time, because the considerations for the first few may well apply to many others.

[Hajime Morrita]

Created attachment 99598 [details]
Patch

[Hajime Morrita]

Hi Darin, thank you for having another turn!
I agree your concern about the size of this change. 
So I changed to apply this for a subset of Range method, where I relatively know well.

(In reply to comment #20)
> (From update of attachment 99455 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99455&action=review
> 
> This patch changes far too much at once. We need to be careful about where we choose to ignore exception codes and where we choose to check them. Almost all the examples here that are ignoring exceptions should instead be asserting them by default.
> 
> Lets do a smaller version of this patch that tackles a much smaller set of functions at first. I know you held back to only functions in the dom directory, but I think this needs more thought for each function.
> 
> Specifically, adding a checker to assert there is no exception by default should be the first thing we consider for any given function. Ignoring the exception instead should be done only rarely when that is helpful for a reason specific to that function.

Well, there are many paths that don't assert the exception code, and some of them actually
ignore the error intentionally.
In this change, I leave such kind of delicate parts and start from a obvious one
where original code asserts the exception code, or it should never throw an error.

> > Source/WebCore/dom/IgnoringExceptionCode.h:51
> > +#if !defined(ASSERT_DISABLED)
> > +    : m_code(0)
> > +#endif
> 
> If we are ignoring the exception code then we don’t want to initialize m_code at all. The only reason we have this here is that this class is not really the “ignored exception code” class, since we plan to derive from it and not ignore the exception code!
> 
> There’s no reason t have this unclear design. I’d rather see repeated code.
> 
OK, 

> > Source/WebCore/dom/IgnoringExceptionCode.h:76
> > +class NeverThrownWithLocation : public IgnoringExceptionCode {
> > +public:
> > +    NeverThrownWithLocation(const char*, size_t, const char*);
> > +    ~NeverThrownWithLocation();
> > +
> > +private:
> > +    const char* m_file;
> > +    size_t m_line;
> > +    const char* m_function;
> > +};
> > +
> > +inline NeverThrownWithLocation::NeverThrownWithLocation(const char* file, size_t line, const char* function)
> > +    : m_file(file), m_line(line), m_function(function)
> > +{
> > +}
> > +
> > +inline NeverThrownWithLocation::~NeverThrownWithLocation()
> > +{
> > +    ASSERT_AT(!m_code, m_file, m_line, m_function);
> > +}
> > +
> > +}
> 
> This entire class should be inside an #if !ASSERT_DISABLED since we don’t want to use it at all in that case.
Done.

> 
> This is a misuse of inheritance. A derived class should not contradict something from a base class. The classic example is that ellipse should not inherit from circle, because an ellipse is not a circle. So having a class that asserts an exception code is zero be a subclass of something called “ignored exception code” is not good design.
That's true. I introduced a superclass called ExceptionCodePlaceholder, 
then make two subclasses named IgnorableExceptionCode and NoExceptionAssertionChecker.

> 
> A class name should be a noun phrase describing the object. The name "never thrown with location" is not good because it’s not sensible to say “this object is a never thrown with location”.
> 
> I would call the class something like ExceptionCodeAssertionChecker. Or NoExceptionAssertionChecker.
> 
> > Source/WebCore/dom/IgnoringExceptionCode.h:82
> > +#if ASSERT_DISABLED
> > +#define ASSERT_NEVER_THROWN WebCore::IgnoringExceptionCode()
> > +#else
> > +#define ASSERT_NEVER_THROWN WebCore::NeverThrownWithLocation(__FILE__, __LINE__, WTF_PRETTY_FUNCTION)
> > +#endif
> 
> I am not fond of “never thrown” as a way to say there is no exception. I would prefer ASSERT_NO_EXCEPTION.
Renamed.

> 
> It’s neat that we can tell you exactly where the exception occurred if you use the macro at the call site, but generally I don’t think callers should be thinking about this at all. I think that most of the DOM functions should default to asserting there is no exception. There are very few DOM functions where we’d want to call them internally and actually ignore the exception.
Agreed. So I keep ASSERT_NO_EXCEPTION and use it as a default parameter.
It's less powerful than using it on caller side, but it stays useful in certain level
because we can see which the problematic callee is.

[Hajime Morrita]

Created attachment 99659 [details]
Patch

[Darin Adler]

Comment on attachment 99659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99659&action=review

Otherwise, this looks good.

> Source/WebCore/dom/ExceptionCodePlaceholder.h:30
> + */
> +#ifndef ExceptionCodePlaceholder_h

Normally we put a blank line there before the #ifndef.

> Source/WebCore/dom/ExceptionCodePlaceholder.h:39
> +class ExceptionCodePlaceholder {

Probably should use WTF_MAKE_NONCOPYABLE on this class.

> Source/WebCore/dom/ExceptionCodePlaceholder.h:55
> +
> +

We use single blank lines, not double.

> Source/WebCore/dom/ExceptionCodePlaceholder.h:58
> +public:
> +    IgnorableExceptionCode() { }

This explicit public constructor is not needed, and gives us the same constructor we’d get if we did not explicitly define the constructor. We should just leave it out.

> Source/WebCore/dom/ExceptionCodePlaceholder.h:61
> +#if !defined(ASSERT_DISABLED)

ASSERT_DISABLED is a macro with a value, not a macro that has definition vs. non-definition. So this must be:

    #if !ASSERT_DISABLED

I also suggest reversing the sense and putting the simpler macro first.

> Source/WebCore/dom/ExceptionCodePlaceholder.h:65
> +    NoExceptionAssertionChecker(const char*, size_t, const char*);

Arguments need names here. The types do not make them clear.

The type size_t is wrong here. The __LINE__ macro has type int, and you should use either int or unsigned here, not size_t.

> Source/WebCore/dom/ExceptionCodePlaceholder.h:90
> +#define ASSERT_NO_EXCEPTION WebCore::NoExceptionAssertionChecker(__FILE__, __LINE__, WTF_PRETTY_FUNCTION)
> +#else
> +#define ASSERT_NO_EXCEPTION WebCore::IgnorableExceptionCode()
> +#endif

Would be better to use ::WebCore:: rather than just WebCore:: in these macros.

More blank lines would make the #if easier to read.

> Source/WebCore/dom/ExceptionCodePlaceholder.h:94
> +
> +

We use single blank lines, not double.

[Darin Adler]

Comment on attachment 99659 [details]
Patch

review- because of the incorrect use of ASSERT_DISABLED

[Hajime Morrita]

Created attachment 100429 [details]
Patch

[Hajime Morrita]

Comment on attachment 99659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99659&action=review

Hi Darin, Thanks you for coming back and taking a look again!
I updated the patch to address the feedback.
Could you take another (hopefully final) round when you have time?
Thanks in advance.

--

>> Source/WebCore/dom/ExceptionCodePlaceholder.h:30
>> +#ifndef ExceptionCodePlaceholder_h
> 
> Normally we put a blank line there before the #ifndef.

Added a line.

>> Source/WebCore/dom/ExceptionCodePlaceholder.h:39
>> +class ExceptionCodePlaceholder {
> 
> Probably should use WTF_MAKE_NONCOPYABLE on this class.

Done.

>> Source/WebCore/dom/ExceptionCodePlaceholder.h:55
>> +
> 
> We use single blank lines, not double.

Removed a line.

>> Source/WebCore/dom/ExceptionCodePlaceholder.h:58
>> +    IgnorableExceptionCode() { }
> 
> This explicit public constructor is not needed, and gives us the same constructor we’d get if we did not explicitly define the constructor. We should just leave it out.

Ah, yes. Removed.

>> Source/WebCore/dom/ExceptionCodePlaceholder.h:61
>> +#if !defined(ASSERT_DISABLED)
> 
> ASSERT_DISABLED is a macro with a value, not a macro that has definition vs. non-definition. So this must be:
> 
>     #if !ASSERT_DISABLED
> 
> I also suggest reversing the sense and putting the simpler macro first.

Thanks for Good catch!!
I fixed it.

>> Source/WebCore/dom/ExceptionCodePlaceholder.h:65
>> +    NoExceptionAssertionChecker(const char*, size_t, const char*);
> 
> Arguments need names here. The types do not make them clear.
> 
> The type size_t is wrong here. The __LINE__ macro has type int, and you should use either int or unsigned here, not size_t.

OK. turned it into int.

>> Source/WebCore/dom/ExceptionCodePlaceholder.h:90
>> +#endif
> 
> Would be better to use ::WebCore:: rather than just WebCore:: in these macros.
> 
> More blank lines would make the #if easier to read.

Sure. adding "::" prefix, organizing code with additional blank lines.

>> Source/WebCore/dom/ExceptionCodePlaceholder.h:94
>> +
> 
> We use single blank lines, not double.

Removed a line.

[Darin Adler]

Comment on attachment 100429 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100429&action=review

review- because of the Windows problem; otherwise looks good

> Source/WebCore/dom/ExceptionCodePlaceholder.h:91
> +#define ASSERT_NO_EXCEPTION ::WebCore::NoExceptionAssertionChecker(__FILE__, __LINE__, WTF_PRETTY_FUNCTION)

The function-name-finding part of this does not work when the macro is used outside a function, and on Windows it won’t compile. That’s what the Windows EWS bot is telling us. I suggest leaving out the function since the vast majority of uses of this macro will be outside any function. Or you could keep this and leave out the function names when compiling with the Windows compiler.

[Hajime Morrita]

Created attachment 100614 [details]
Patch

[Hajime Morrita]

> The function-name-finding part of this does not work when the macro is used outside a function, and on Windows it won’t compile. That’s what the Windows EWS bot is telling us. I suggest leaving out the function since the vast majority of uses of this macro will be outside any function. Or you could keep this and leave out the function names when compiling with the Windows compiler.
Oops, I missed the failure...
An update patch no longer has function name in the macro.
Thanks for the catch.

[Darin Adler]

Comment on attachment 100614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100614&action=review

> Source/WebCore/dom/ExceptionCodePlaceholder.h:78
> +inline NoExceptionAssertionChecker::NoExceptionAssertionChecker(const char* file, int line)

Normally when we disable assertions we also disable inlining. I don’t think we really would want this constructor to be inline if we had a .cpp file to put it in. Same for the destructor.

[Hajime Morrita]

Comment on attachment 100614 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=100614&action=review

Thanks for take a look! I'm landing this...

>> Source/WebCore/dom/ExceptionCodePlaceholder.h:78
>> +inline NoExceptionAssertionChecker::NoExceptionAssertionChecker(const char* file, int line)
> 
> Normally when we disable assertions we also disable inlining. I don’t think we really would want this constructor to be inline if we had a .cpp file to put it in. Same for the destructor.

Well, I thought this file as same as Assertions.h so omitted a .cpp file for this.

[WebKit Review Bot]

Comment on attachment 100614 [details]
Patch

Clearing flags on attachment: 100614

Committed r90911: <http://trac.webkit.org/changeset/90911>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to comment #33)
> >> Source/WebCore/dom/ExceptionCodePlaceholder.h:78
> >> +inline NoExceptionAssertionChecker::NoExceptionAssertionChecker(const char* file, int line)
> > 
> > Normally when we disable assertions we also disable inlining. I don’t think we really would want this constructor to be inline if we had a .cpp file to put it in. Same for the destructor.
> 
> Well, I thought this file as same as Assertions.h so omitted a .cpp file for this.

I don’t understand what you are saying. There’s an Assertions.cpp file. Why wouldn’t there be an ExceptionCodePlaceholder.cpp file?

[Hajime Morrita]

> I don’t understand what you are saying. There’s an Assertions.cpp file. Why wouldn’t there be an ExceptionCodePlaceholder.cpp file?

Oops, I totally missed it. I'll add ExceptionCodePlaceholder.cpp.
Thanks for pointing that out...

[Hajime Morrita]

(In reply to comment #37)
> > I don’t understand what you are saying. There’s an Assertions.cpp file. Why wouldn’t there be an ExceptionCodePlaceholder.cpp file?
> 
> Oops, I totally missed it. I'll add ExceptionCodePlaceholder.cpp.
Uploaded a fix: https://bugs.webkit.org/show_bug.cgi?id=64503

[Antti Koivisto]

We should avoid using external API functions (which all functions that take ExceptionCode should be) for internal purposes. It confuses our architecture and can lead to poor code factoring. Conforming with unneeded DOM API behaviors can have performance impact.

This type makes using API functions internally easier and so takes us to wrong direction. Rather, we should think of ways to make it harder to do and easier to spot (for example by using a naming pattern). Further use of this type should be discouraged and we should eventually remove it.

[Darin Adler]

(In reply to comment #39)
> We should avoid using external API functions (which all functions that take ExceptionCode should be) for internal purposes. It confuses our architecture and can lead to poor code factoring. Conforming with unneeded DOM API behaviors can have performance impact.
> 
> This type makes using API functions internally easier and so takes us to wrong direction. Rather, we should think of ways to make it harder to do and easier to spot (for example by using a naming pattern). Further use of this type should be discouraged and we should eventually remove it.

I agree with Antti, although I worded my concerns a bit differently. See what I said in comment #6:

(In reply to comment #6)
> I think that for most cases it would be even better to have functions without the ExceptionCode& argument. We need the ExceptionCode& argument to communicate exceptions to the DOM bindings for the DOM itself, but outside of that I think we should strive to leave out these ExceptionCode& arguments.
> 
> And for basic functions like appendChild that are in the DOM but also need to be used extensively inside WebCore, we should have alternate versions, either overloads or separate functions, for internal use that do not take ExceptionCode&.