Implement Filter Effects Module feDropShadow filter

RESOLVED FIXED in mozilla30

Status

()

Core
SVG
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: longsonr, Assigned: longsonr)

Tracking

({dev-doc-needed})

Trunk
mozilla30
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

4 years ago
Created attachment 8365851 [details] [diff] [review]
shadow1.txt

See https://dvcs.w3.org/hg/FXTF/raw-file/tip/filters/index.html#elementdef-fecustom

Chromium implements this filter.
Attachment #8365851 - Flags: review?(hsivonen)
(Assignee)

Comment 2

4 years ago
Created attachment 8365856 [details] [diff] [review]
shadow2.txt
Assignee: nobody → longsonr
Attachment #8365856 - Flags: review?(hsivonen)
(Assignee)

Comment 3

4 years ago
Created attachment 8365859 [details] [diff] [review]
shadow3.txt
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+
(Assignee)

Comment 5

4 years ago
Created attachment 8365915 [details] [diff] [review]
use the new feature
Attachment #8365915 - Flags: review?(weinjared+bmo)
(Assignee)

Comment 6

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 8365998 [details] [diff] [review]
address review comments

This compiles even if UNIFIED_SOURCES is changed to SOURCES in moz.build
Attachment #8365859 - Attachment is obsolete: true
Created attachment 8366538 [details] [diff] [review]
Changes to the HTML parser
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 #8365856 - Flags: review?(hsivonen) → review+
Attachment #8366538 - Flags: review?(wchen)

Updated

4 years ago
Attachment #8366538 - Flags: review?(wchen) → review+
Keywords: dev-doc-needed
(Assignee)

Updated

4 years ago
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.
(Assignee)

Comment 15

4 years ago
It seemed to be OK on try (comment 10). I wonder if it needs a clobber?
(Assignee)

Updated

4 years ago
Attachment #8366538 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8365856 - Flags: checkin+
(Assignee)

Updated

4 years ago
Attachment #8365998 - Flags: checkin+
(Assignee)

Comment 24

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Comment 26

3 years ago
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)
(Assignee)

Updated

3 years ago
Blocks: 1120071
(Assignee)

Comment 27

3 years ago
Raised bug 1120071 for this.
Flags: needinfo?(longsonr)
(Assignee)

Updated

3 years ago
Attachment #8365915 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.