Bug 35320

Summary:

SVGResourceFilter needs to be moved to under Renderers

Product:

WebKit

Reporter:

David Yonge-Mallo <davinci>

Component:

SVG

Assignee:

Dirk Schulze <krit>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, eric, krit, rjkroege, webkit.review.bot, zimmermann

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

37846

Bug Blocks:

Attachments:

Description	Flags
patch	none
patch	none
patch	zimmermann: review-
patch	none
patch	none
Patch	none
Patch	none
Patch	none

Description David Yonge-Mallo 2010-02-23 16:39:40 PST

Continuation of the refactoring started in https://bugs.webkit.org/show_bug.cgi?id=35020

Cause of Chromium bug http://crbug.com/30939

Comment 1 Dirk Schulze 2010-04-16 10:04:01 PDT

I'm sorry, that I assign this bug to me. I would like to combine this move with little design changes and clean-ups. I'll upload a patch soon. We have to go forward on killing the old resources, also because of speed reasons.

Comment 2 David Yonge-Mallo 2010-04-16 11:31:14 PDT

Ok.  I'm sorry that I wasn't able to get this done.

Comment 3 Dirk Schulze 2010-04-16 12:02:06 PDT

(In reply to comment #2)
> Ok.  I'm sorry that I wasn't able to get this done.

Oh, no problem. But it is a very big patch for the beginning.

Comment 4 Dirk Schulze 2010-04-19 11:44:30 PDT

Created attachment 53698 [details]
patch

patch

Comment 5 WebKit Review Bot 2010-04-19 12:46:48 PDT

Attachment 53698 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1671439

Comment 6 Dirk Schulze 2010-04-19 14:01:42 PDT

Created attachment 53717 [details]
patch

Fogot some witches for filters. On Chromium filters are disabled.

Comment 7 WebKit Review Bot 2010-04-19 14:05:26 PDT

Attachment 53717 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WARNING: Skipping non-existent file: WebCore/svg/graphics/SVGResourceFilter.h
WebCore/rendering/RenderSVGResourceFilter.cpp:291:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
WARNING: Skipping non-existent file: WebCore/svg/graphics/SVGResourceFilter.cpp
Total errors found: 1 in 118 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 8 Eric Seidel (no email) 2010-04-19 14:10:26 PDT

Attachment 53717 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/1676429

Comment 9 WebKit Review Bot 2010-04-19 14:44:34 PDT

Attachment 53717 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1608611

Comment 10 Dirk Schulze 2010-04-19 23:50:40 PDT

Created attachment 53776 [details]
patch

more filter switches.

Comment 11 Nikolas Zimmermann 2010-04-20 01:46:44 PDT

Comment on attachment 53776 [details]
patch

Discussed lots of things during IRC review. Dirk will upload a new version.

Comment 12 Dirk Schulze 2010-04-20 01:59:38 PDT

Created attachment 53785 [details]
patch

Updated patch with some notes of Nikolas.

Comment 13 WebKit Review Bot 2010-04-20 02:02:57 PDT

Attachment 53785 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/rendering/RenderSVGResourceFilter.cpp:169:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 115 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment 14 Dirk Schulze 2010-04-20 02:14:03 PDT

Created attachment 53787 [details]
patch

fixed more style issues.

Comment 15 Nikolas Zimmermann 2010-04-20 02:23:36 PDT

Comment on attachment 53787 [details]
patch

Fantastic patch, moving in the right direction :-)
Still needs a follow-up patch to rework filter effect dumping, but we had nothing before, so this is way better than trunk.

r=me, though a last little nitpick before landing:
> +    if (!filterData->boundaries.width() || !filterData->boundaries.height())
> +        return false;
Please use if (filterData->boundaries.isEmpty()) here :-)

Jeez, what a patch, even larger then those I write ;-)

Comment 16 WebKit Review Bot 2010-04-20 03:06:38 PDT

http://trac.webkit.org/changeset/57880 might have broken Qt Linux Release

Comment 17 Dirk Schulze 2010-04-20 04:58:08 PDT

Created attachment 53797 [details]
Patch

Comment 18 Nikolas Zimmermann 2010-04-20 05:11:54 PDT

Comment on attachment 53797 [details]
Patch

Ok, let's try our luck again, r=me.

Comment 19 Dirk Schulze 2010-04-20 05:36:56 PDT

Created attachment 53801 [details]
Patch

Comment 20 Nikolas Zimmermann 2010-04-20 05:42:56 PDT

Comment on attachment 53801 [details]
Patch

r=me. Please remember to update ChangeLog to include the win/ results before landing.

Comment 21 Dirk Schulze 2010-04-20 05:44:48 PDT

Created attachment 53803 [details]
Patch

Comment 22 Nikolas Zimmermann 2010-04-20 05:48:35 PDT

Comment on attachment 53803 [details]
Patch

r=me, again. I would have trusted you to update the ChangeLog though :-)

Comment 23 Dirk Schulze 2010-04-20 06:30:46 PDT

Comment on attachment 53803 [details]
Patch

Clearing flags on attachment: 53803

Committed r57886: <http://trac.webkit.org/changeset/57886>

Comment 24 Dirk Schulze 2010-04-20 06:31:18 PDT

All reviewed patches have been landed.  Closing bug.