Bug 109044 – Replace ExceptionCode assertions with ASSERT_NO_EXCEPTION macro.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

The pattern:

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

is more clearly and succinctly written as:

    methodThatGeneratesException(ASSERT_NO_EXCEPTION);

[Mike West]

(In reply to comment #0)
> The pattern:
> 
>     ExceptionCode ec = 0;
>     methodThatGeneratesException(ec);
>     ASSERT(!ec);
> 
> is more clearly and succinctly written as:
> 
>     methodThatGeneratesException(ASSERT_NO_EXCEPTION);

Note that this only applies to call sites that do nothing else with 'ec'.

    ExceptionCode ec = 0;
    methodThatGeneratesException(ec);
    ASSERT(!ec);
    if (ec)
        return;

should be left alone.

[Mike West]

Created attachment 186839 [details]
Patch

[Early Warning System Bot]

Comment on attachment 186839 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 186839 [details]
Patch

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

[Mike West]

Comment on attachment 186839 [details]
Patch

Ugh. Missed a spot. :)

[Mike West]

Created attachment 186845 [details]
Less broken.

[WebKit Review Bot]

Comment on attachment 186845 [details]
Less broken.

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

New failing tests:
dom/xhtml/level2/core/setAttributeNS10.xhtml
dom/html/level2/core/setAttributeNS10.html
fast/dom/Element/setAttributeNS-namespace-err.html

[Build Bot]

Comment on attachment 186845 [details]
Less broken.

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

New failing tests:
fast/dom/Element/setAttributeNS-namespace-err.html
dom/html/level2/core/setAttributeNS10.html
dom/xhtml/level2/core/setAttributeNS10.xhtml

[Mike West]

Created attachment 186862 [details]
Patch

[Darin Adler]

Comment on attachment 186862 [details]
Patch

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

I like that this cuts down on the "ec" name, and code that knows that it’s an integer.

It’s interesting how much of this is basic DOM tree and Range operations. Makes me think we should have internal functions for these instead of going through the “real DOM” functions. Likely an even better solution to this problem area for many call sites that could even possibly lead to some refactoring opportunities or a performance boost. At some point we should use grep to come up with stats about which functions are used most often with ASSERT_NO_EXCEPTION and IGNORE_EXCEPTION.

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:99
> +    const AtomicString& direction = directionToString(m_direction, ASSERT_NO_EXCEPTION);
>      return direction;

No need for the local variable any more here.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:125
> +    const AtomicString& mode = modeToString(m_mode, ASSERT_NO_EXCEPTION);
>      return mode;

No need for the local variable any more here.

[Joshua Bell]

Comment on attachment 186862 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:99
>>      return direction;
> 
> No need for the local variable any more here.

And FYI the directionToString method is only ever called from here now; it was used when we supported both legacy numeric enum and new string values when called from script. Now that the numeric enum values are not supported from script we can drop the ExceptionCode argument from this method. But that can be done in a separate patch.

>> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:125
>>      return mode;
> 
> No need for the local variable any more here.

And FYI, same as above. We can drop the ExceptionCode param from modeToStrng now.

[Build Bot]

Comment on attachment 186862 [details]
Patch

Attachment 186862 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16401229

[Mike West]

(In reply to comment #10)
> (From update of attachment 186862 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186862&action=review
> 
> I like that this cuts down on the "ec" name, and code that knows that it’s an integer.

Indeed, the less of that the better.

> At some point we should use grep to come up with stats about which functions are used most often with ASSERT_NO_EXCEPTION and IGNORE_EXCEPTION.

My plan is to post just such a list to webkit-dev once this set of patches seems more or less finished.

I've broken jsbell@'s comments out into https://bugs.webkit.org/show_bug.cgi?id=109143; I'll take care of that once this lands.

[Mike West]

Created attachment 187004 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 187004 [details]
Patch for landing

Clearing flags on attachment: 187004

Committed r142118: <http://trac.webkit.org/changeset/142118>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Gavin Peters]

Reverted r142118 for reason:

Broke SVG! Oh noes!

Committed r142126: <http://trac.webkit.org/changeset/142126>

[Alexey Proskuryakov]

Sample test run: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r142124%20(5307)/results.html>.

[Mike West]

(In reply to comment #18)
> Sample test run: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r142124%20(5307)/results.html>.

Thanks ap. I can replicate these locally, though I'm not at all sure why they occur. I'll poke at it tomorrow morning, but I'll probably end up splitting the IDB and SVG portions out into separate patches.

[Stephen Chenney]

(In reply to comment #19)
> (In reply to comment #18)
> > Sample test run: <http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK1%20(Tests)/r142124%20(5307)/results.html>.
> 
> Thanks ap. I can replicate these locally, though I'm not at all sure why they occur. I'll poke at it tomorrow morning, but I'll probably end up splitting the IDB and SVG portions out into separate patches.

A few possibilities I could see:
- The calling object is an element in an array passed by reference. Maybe something odd happens
- More likely, the mess of template code that exists in SVG animations may be interfering with the changes you made.
- Or, the method to which you are passing the ASSERT_NO_EXCEPTIONS parameter is not designed to handle it.

[Mike West]

Created attachment 187259 [details]
Patch

[Mike West]

Comment on attachment 187259 [details]
Patch

I've dropped IDB and SVG from this patch, and the tests pass locally. I'll take another stab at them in https://bugs.webkit.org/show_bug.cgi?id=109266 and https://bugs.webkit.org/show_bug.cgi?id=109267 respectively.

[WebKit Review Bot]

Comment on attachment 187259 [details]
Patch

Clearing flags on attachment: 187259

Committed r142247: <http://trac.webkit.org/changeset/142247>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mike West]

*** Bug 108772 has been marked as a duplicate of this bug. ***