Closed Bug 964200 Opened 11 years ago Closed 11 years ago

Implement Filter Effects Module feDropShadow filter


(Core :: SVG, defect)

Not set





(Reporter: longsonr, Assigned: longsonr)


(Blocks 1 open bug)


(Keywords: dev-doc-needed)


(4 files, 2 obsolete files)

      No description provided.
Attached patch shadow1.txtSplinter Review

Chromium implements this filter.
Attachment #8365851 - Flags: review?(hsivonen)
Attached patch shadow2.txtSplinter Review
Assignee: nobody → longsonr
Attachment #8365856 - Flags: review?(hsivonen)
Attached patch shadow3.txt (obsolete) — Splinter Review
Attachment #8365859 - Flags: review?(mstange)
Comment on attachment 8365859 [details] [diff] [review]

Very nice!

>diff --git a/content/svg/content/src/SVGFEGaussianBlurElement.cpp b/content/svg/content/src/SVGFEDropShadowElement.cpp
>copy from content/svg/content/src/SVGFEGaussianBlurElement.cpp
>copy to content/svg/content/src/SVGFEDropShadowElement.cpp
>--- a/content/svg/content/src/SVGFEGaussianBlurElement.cpp
>+++ b/content/svg/content/src/SVGFEDropShadowElement.cpp
>@@ -1,125 +1,179 @@
> /* a*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>  * file, You can obtain one at */
>-#include "mozilla/dom/SVGFEGaussianBlurElement.h"
>-#include "mozilla/dom/SVGFEGaussianBlurElementBinding.h"
>+#include "mozilla/dom/SVGFEDropShadowElement.h"
>+#include "mozilla/dom/SVGFEDropShadowElementBinding.h"
> #include "nsSVGFilterInstance.h"
>-#include "nsSVGUtils.h"
>-using namespace mozilla::gfx;

I don't think you can remove this line. If it compiles, then probably because the file is getting unified with other files that have this declaration.
(You can confirm whether this is the case by moving this file from the UNIFIED_SOURCES list to the SOURCES list in

>diff --git a/gfx/src/FilterSupport.cpp b/gfx/src/FilterSupport.cpp

>+      if (aDescription.InputColorSpace(0) == LINEAR_RGB) {
>+        flood = FilterWrappers::SRGBToLinearRGB(aDT, flood);
>       }

Do we need to do something similar for the lighting filters? Also, it would be much better for performance if this transformation were only applied to the single Color value that we set on the filter, and not on the whole filter output. Can you add helper methods for that?
Attachment #8365859 - Flags: review?(mstange) → review+
Attached patch use the new feature (obsolete) — Splinter Review
Attachment #8365915 - Flags: review?(weinjared+bmo)
(In reply to Markus Stange [:mstange] from comment #4)
> Do we need to do something similar for the lighting filters?

Lighting filters were originally fixed in bug 828526 although there's no test for that. Hopefully that carried over into the Moz2D implementation. It's only because we need a colour in the middle of filter processing that we run into an issue. Normally the colour space would be adjusted pre/post filter processing.
This compiles even if UNIFIED_SOURCES is changed to SOURCES in
Attachment #8365859 - Attachment is obsolete: true
Comment on attachment 8365851 [details] [diff] [review]

>diff --git a/content/base/src/nsGkAtomList.h b/content/base/src/nsGkAtomList.h

>diff --git a/content/base/src/nsTreeSanitizer.cpp b/content/base/src/nsTreeSanitizer.cpp

r+ on these.

>diff --git a/parser/html/javasrc/ b/parser/html/javasrc/

>diff --git a/parser/html/nsHtml5ElementName.cpp b/parser/html/nsHtml5ElementName.cpp

r- on these for the failure  to update the precomputed hashes and to put the new entry in the place where the self-generator for would put it. To save you the trouble of dealing with two layers of code generation, I've attached the right parser changes as attachment 8366538 [details] [diff] [review].

Also, changing the parser like this should be accompanied with a corresponding spec change. I've filed and sent .
Attachment #8365851 - Flags: review?(hsivonen) → review-
Attachment #8365856 - Flags: review?(hsivonen) → review+
Attachment #8366538 - Flags: review?(wchen)
Attachment #8366538 - Flags: review?(wchen) → review+
Whiteboard: [leave open]
Backed out both for build bustage - since they were on build bustage, hard to see on their landings, should be easier to see what's yours in where your parent bustage was backed out.
It seemed to be OK on try (comment 10). I wonder if it needs a clobber?
Attachment #8366538 - Flags: checkin+
Attachment #8365856 - Flags: checkin+
Attachment #8365998 - Flags: checkin+
Flags: in-testsuite+
Whiteboard: [leave open]
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8365915 [details] [diff] [review]
use the new feature

Robert, this review was attached to an account that I don't log in to often and such I didn't see this until now. If this still needs review, please re-request review but from :jaws. Thanks.
Flags: needinfo?(longsonr)
Attachment #8365915 - Flags: review?(weinjared+bmo)
Blocks: 1120071
Raised bug 1120071 for this.
Flags: needinfo?(longsonr)
Attachment #8365915 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.