Closed Bug 964200 Opened 7 years ago Closed 7 years ago

Implement Filter Effects Module feDropShadow filter

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: longsonr, Assigned: longsonr)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 2 obsolete files)

No description provided.
Attached patch shadow1.txtSplinter Review
See https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#elementdef-fecustom

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]
shadow3.txt

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 http://mozilla.org/MPL/2.0/. */
> 
>-#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"
> 
>-NS_IMPL_NS_NEW_NAMESPACED_SVG_ELEMENT(FEGaussianBlur)
>-
>-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 moz.build.)

>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 moz.build
Attachment #8365859 - Attachment is obsolete: true
Comment on attachment 8365851 [details] [diff] [review]
shadow1.txt

>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/ElementName.java b/parser/html/javasrc/ElementName.java

>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 ElementName.java 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 https://www.w3.org/Bugs/Public/show_bug.cgi?id=24424 and sent http://lists.w3.org/Archives/Public/public-fx/2014JanMar/0046.html .
Attachment #8365851 - Flags: review?(hsivonen) → review-
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 https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=e1fb8eeca62e 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bbf7117f577
Flags: in-testsuite+
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/5bbf7117f577
Status: NEW → RESOLVED
Closed: 7 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.