964200 - Implement Filter Effects Module feDropShadow filter

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Robert Longson [:longsonr]]

Attachment: shadow1.txt

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

Chromium implements this filter.

Comment 2

[Robert Longson [:longsonr]]

Attachment: shadow2.txt

Comment 3

[Robert Longson [:longsonr]]

Attachment: shadow3.txt

Comment 4

[Markus Stange [: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?

Comment 5

[Robert Longson [:longsonr]]

Attachment: use the new feature

Comment 6

[Robert Longson [:longsonr]]

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

Comment 7

[Robert Longson [:longsonr]]

Attachment: address review comments

This compiles even if UNIFIED_SOURCES is changed to SOURCES in moz.build

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Changes to the HTML parser

Comment 9

[Henri Sivonen (:hsivonen)]

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 .

Comment 10

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=d13bb1334f86

Comment 11

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87e3729c065a

Comment 12

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af184f7760b9

Comment 13

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/19993e76f4a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbe53176a38

Comment 14

[Phil Ringnalda (:philor)]

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.

Comment 15

[Robert Longson [:longsonr]]

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

Comment 16

[Phil Ringnalda (:philor)]

Also, https://tbpl.mozilla.org/php/getParsedLog.php?id=34147081&tree=Mozilla-Inbound

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87e3729c065a
https://hg.mozilla.org/mozilla-central/rev/af184f7760b9

Comment 18

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=2d0ccb3fd5d0

https://hg.mozilla.org/integration/mozilla-inbound/rev/01617899e713

Comment 19

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85b2df0dfa13

Comment 20

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d3d6b83f46b

Comment 21

[Robert Longson [:longsonr]]

This time with a clobber, should stick I hope.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7e7180113da
https://hg.mozilla.org/integration/mozilla-inbound/rev/542ea91fc525

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b7e7180113da
https://hg.mozilla.org/mozilla-central/rev/542ea91fc525

Comment 23

[Robert Longson [:longsonr]]

https://tbpl.mozilla.org/?tree=Try&rev=4f6163423a45
https://tbpl.mozilla.org/?tree=Try&rev=9cc6844e381c

Comment 24

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bbf7117f577

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5bbf7117f577

Comment 26

[Nobody; Do Not Use]

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.

Comment 27

[Robert Longson [:longsonr]]

Raised bug 1120071 for this.