Bug 109044 - Replace ExceptionCode assertions with ASSERT_NO_EXCEPTION macro.
Summary: Replace ExceptionCode assertions with ASSERT_NO_EXCEPTION macro.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
: 108772 (view as bug list)
Depends on:
Blocks: 108180 109266 109267
  Show dependency treegraph
 
Reported: 2013-02-06 05:47 PST by Mike West
Modified: 2013-02-08 05:20 PST (History)
23 users (show)

See Also:


Attachments
Patch (56.51 KB, patch)
2013-02-06 06:09 PST, Mike West
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Less broken. (56.95 KB, patch)
2013-02-06 06:44 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (56.09 KB, patch)
2013-02-06 08:42 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (56.15 KB, patch)
2013-02-07 00:20 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (43.84 KB, patch)
2013-02-08 00:51 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2013-02-06 05:47:15 PST
The pattern:

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

is more clearly and succinctly written as:

    methodThatGeneratesException(ASSERT_NO_EXCEPTION);
Comment 1 Mike West 2013-02-06 05:52:42 PST
(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.
Comment 2 Mike West 2013-02-06 06:09:55 PST
Created attachment 186839 [details]
Patch
Comment 3 Early Warning System Bot 2013-02-06 06:16:46 PST
Comment on attachment 186839 [details]
Patch

Attachment 186839 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16395204
Comment 4 Early Warning System Bot 2013-02-06 06:16:55 PST
Comment on attachment 186839 [details]
Patch

Attachment 186839 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16400112
Comment 5 Mike West 2013-02-06 06:21:35 PST
Comment on attachment 186839 [details]
Patch

Ugh. Missed a spot. :)
Comment 6 Mike West 2013-02-06 06:44:42 PST
Created attachment 186845 [details]
Less broken.
Comment 7 WebKit Review Bot 2013-02-06 07:59:58 PST
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
Comment 8 Build Bot 2013-02-06 08:25:19 PST
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
Comment 9 Mike West 2013-02-06 08:42:34 PST
Created attachment 186862 [details]
Patch
Comment 10 Darin Adler 2013-02-06 09:30:06 PST
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.
Comment 11 Joshua Bell 2013-02-06 09:49:45 PST
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.
Comment 12 Build Bot 2013-02-06 12:30:14 PST
Comment on attachment 186862 [details]
Patch

Attachment 186862 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16401229
Comment 13 Mike West 2013-02-07 00:12:05 PST
(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.
Comment 14 Mike West 2013-02-07 00:20:20 PST
Created attachment 187004 [details]
Patch for landing
Comment 15 WebKit Review Bot 2013-02-07 07:15:42 PST
Comment on attachment 187004 [details]
Patch for landing

Clearing flags on attachment: 187004

Committed r142118: <http://trac.webkit.org/changeset/142118>
Comment 16 WebKit Review Bot 2013-02-07 07:15:48 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Gavin Peters 2013-02-07 08:24:29 PST
Reverted r142118 for reason:

Broke SVG! Oh noes!

Committed r142126: <http://trac.webkit.org/changeset/142126>
Comment 19 Mike West 2013-02-07 10:47:28 PST
(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.
Comment 20 Stephen Chenney 2013-02-07 10:55:07 PST
(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.
Comment 21 Mike West 2013-02-08 00:51:18 PST
Created attachment 187259 [details]
Patch
Comment 22 Mike West 2013-02-08 00:53:26 PST
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.
Comment 23 WebKit Review Bot 2013-02-08 01:14:29 PST
Comment on attachment 187259 [details]
Patch

Clearing flags on attachment: 187259

Committed r142247: <http://trac.webkit.org/changeset/142247>
Comment 24 WebKit Review Bot 2013-02-08 01:14:35 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Mike West 2013-02-08 05:20:04 PST
*** Bug 108772 has been marked as a duplicate of this bug. ***