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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

109044

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch for landing	none

Mike West

Reported 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.

Attachments
Patch (11.37 KB, patch) 2013-02-08 03:20 PST, Mike West	no flags	Details Formatted Diff Diff
Patch for landing (11.17 KB, patch) 2013-02-08 04:12 PST, Mike West	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Mike West

Comment 1 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).

Mike West

Comment 2 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.

Mike West

Comment 3 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.

Mike West

Comment 4 2013-02-08 03:20:30 PST

Created attachment 187278 [details] Patch

Mike West

Comment 5 2013-02-08 03:21:27 PST

Hola, Jochen. Interested in reviewing this patch? :)

jochen

Comment 6 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?

Mike West

Comment 7 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. :)

Mike West

Comment 8 2013-02-08 04:12:27 PST

Created attachment 187288 [details] Patch for landing

WebKit Review Bot

Comment 9 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>

WebKit Review Bot

Comment 10 2013-02-08 04:56:36 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.