Bug 109267

Summary: Migrate ExceptionCode ASSERTs in SVG to ASSERT_NO_EXCEPTION.
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Mike West <mkwst>
Status: RESOLVED FIXED    
Severity: Normal CC: d-r, eric, fmalita, jochen, ojan.autocc, pdr, schenney, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 109044    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Mike West 2013-02-08 00:22:58 PST
I tried to land changes in SVG code as part of https://bugs.webkit.org/show_bug.cgi?id=109044; they caused test failures, and I'm not sure why.

I'll find out and fix the problems in a smaller, more focused patch.
Comment 1 Mike West 2013-02-08 01:33:14 PST
It looks like ExceptionCodePlaceholder doesn't do the right thing with the negation operator, which is upsetting to SVGLength::setValue, which has different behavior based on 'ec's value (http://trac.webkit.org/browser/trunk/Source/WebCore/svg/SVGLength.cpp?rev=142247#L213).
Comment 2 Mike West 2013-02-08 02:38:42 PST
Ugh. Now I understand what ASSERT_NO_EXCEPTION is doing. It burns.

Off the top of my head, I have no idea how to make it behave in a way that would make `if (!ec)` work in debug. :( I will ponder.
Comment 3 Mike West 2013-02-08 03:09:06 PST
So, as it turns out, this behavior is intentional: ASSERT_NO_EXCEPTION attempts to ensure that methods which care about the value of an ExceptionCode must initialize that ExceptionCode to 0 (see https://bugs.webkit.org/show_bug.cgi?id=78194). SVGLength::setValue does not.
Comment 4 Mike West 2013-02-08 03:20:30 PST
Created attachment 187278 [details]
Patch
Comment 5 Mike West 2013-02-08 03:21:27 PST
Hola, Jochen. Interested in reviewing this patch? :)
Comment 6 jochen 2013-02-08 03:39:40 PST
Comment on attachment 187278 [details]
Patch

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

ok

> Source/WebCore/ChangeLog:19
> +        again with the latter. It does the same for 'ASSERT(ec == 0);' (and, as

I don't see wuch a change in the code?
Comment 7 Mike West 2013-02-08 04:11:00 PST
(In reply to comment #6)
> (From update of attachment 187278 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187278&action=review
> 
> ok
> 
> > Source/WebCore/ChangeLog:19
> > +        again with the latter. It does the same for 'ASSERT(ec == 0);' (and, as
> 
> I don't see wuch a change in the code?

The joys of copy/paste. I'll fix that. :)
Comment 8 Mike West 2013-02-08 04:12:27 PST
Created attachment 187288 [details]
Patch for landing
Comment 9 WebKit Review Bot 2013-02-08 04:56:31 PST
Comment on attachment 187288 [details]
Patch for landing

Clearing flags on attachment: 187288

Committed r142261: <http://trac.webkit.org/changeset/142261>
Comment 10 WebKit Review Bot 2013-02-08 04:56:36 PST
All reviewed patches have been landed.  Closing bug.