Status

()

Core
SVG
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: tor, Assigned: tor)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 274687 [details] [diff] [review]
feImage for raster images

Comment 1

10 years ago
http://www.w3.org/TR/SVG/filters.html#FilterPrimitiveSubRegion
AcquireSourceImage should take an ALL rather than a UNION argument for this filter (or if bug 389865 lands it would need the same virtual function as feTile).

(Assignee)

Comment 2

10 years ago
Created attachment 274999 [details] [diff] [review]
use ALL
Attachment #274687 - Attachment is obsolete: true
Attachment #274999 - Flags: review?(longsonr)

Updated

10 years ago
Attachment #274999 - Flags: review?(longsonr) → review+
(Assignee)

Updated

10 years ago
Attachment #274999 - Flags: superreview?(roc)
+  // fake a message so the filter will invalidate
+  nsNodeUtils::ContentAppended(this, 0);

This could fire DOM mutation events. That's not really what we want. You should be able to invalidate the element without this hack.

You don't seem to be taking the image frame offset into account.
(Assignee)

Comment 4

10 years ago
(In reply to comment #3)
> +  // fake a message so the filter will invalidate
> +  nsNodeUtils::ContentAppended(this, 0);
> 
> This could fire DOM mutation events. That's not really what we want. You should
> be able to invalidate the element without this hack.

Offhand, I can think of a couple other ways of doing the notifications, of various degrees of hackiness:

1) add a "CustomEvent(nsIContent *aContent, void *aContinuation)" event to nIMutationObserver and fire that instead.

2) add a new interface to nsSVGFilterProperty and add a method to nsSVGFilterElement to notify observers that checks for this interface and only fires on those.

Any preference, or should I think of some other possibilities?
let's see how #2 looks?
(Assignee)

Comment 6

10 years ago
Created attachment 276533 [details] [diff] [review]
option #2 - I needed to modify nsINode to get access to the mutation observers
Attachment #276533 - Flags: superreview?(roc)

Comment 7

10 years ago
Comment on attachment 276533 [details] [diff] [review]
option #2 - I needed to modify nsINode to get access to the mutation observers

>+void
>+nsSVGFilterElement::Invalidate() {

Nit: Can we have the { on a new line?

>+NS_IMETHODIMP
>+nsSVGFEImageElement::OnStopDecode(imgIRequest *aRequest,
>+                                  nsresult status,
>+                                  const PRUnichar *statusArg)
>+{
>+  nsresult rv =
>+    nsImageLoadingContent::OnStopDecode(aRequest, status, statusArg);
>+
>+  nsCOMPtr<nsIDOMSVGFilterElement> filter = do_QueryInterface(GetParent());
>+  if (filter) {
>+    static_cast<nsSVGFilterElement*>(GetParent())->Invalidate();
>+  }

How about an Invalidate private member of nsSVGFEImageElement to do the above three lines as this is repeated in other places.
(Assignee)

Comment 8

10 years ago
Created attachment 276555 [details] [diff] [review]
option #2 with longsonr's suggestions
Attachment #276533 - Attachment is obsolete: true
Attachment #276555 - Flags: superreview?(roc)
Attachment #276533 - Flags: superreview?(roc)
I'm not super exited about GetMutationObservers. One reason is that ideally I don't think nsIMutationObserver should be a real interface.

At the very least, make GetMutationObservers protected so that it can't be called from the outside.
(Assignee)

Comment 10

10 years ago
Created attachment 276655 [details] [diff] [review]
make GetMutationObservers protected
Attachment #276555 - Attachment is obsolete: true
Attachment #276655 - Flags: superreview?(roc)
Attachment #276555 - Flags: superreview?(roc)
I was actually thinking of having a separate list of nsISVGFilterProperty observers for filter elements. This feels rather hacky, although I suppose I could live with it.
(Assignee)

Comment 12

10 years ago
(In reply to comment #11)
> I was actually thinking of having a separate list of nsISVGFilterProperty
> observers for filter elements. This feels rather hacky, although I suppose I
> could live with it.

Having a separate list seemed a bit of a waste, since the filters are already being observed for other changes (attributes, content add/delete, etc).

(Assignee)

Updated

10 years ago
Attachment #274999 - Attachment is obsolete: true
Attachment #274999 - Flags: superreview?(roc)
Comment on attachment 276655 [details] [diff] [review]
make GetMutationObservers protected

okay, assuming you address Jonas' comment.
Attachment #276655 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 14

10 years ago
(In reply to comment #13)
> (From update of attachment 276655 [details] [diff] [review])
> okay, assuming you address Jonas' comment.

I addressed the protection bit in this version of the patch.  Not sure what I can do about jonas' excitement level over GetMutationObservers.
Having GetMutationObservers as is is ok with me.
(Assignee)

Comment 16

10 years ago
Comment on attachment 276655 [details] [diff] [review]
make GetMutationObservers protected

Currently our SVG implementation supports all but two filters from the specification - feDisplacementMap (bug 389746) and feImage (this bug).  We would like to have all filters to make life easier for content developers.

This is a new element which didn't involve altering other codepaths, so the risk of it causing problems outside feImage is very minimal.
Attachment #276655 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Attachment #276655 - Flags: approval1.9?
(Assignee)

Comment 17

10 years ago
Created attachment 282752 [details] [diff] [review]
update to tip
Attachment #276655 - Attachment is obsolete: true
Assignee: nobody → tor
Given the state of things now, I think we should take this bug.   What's the current status?

Comment 19

10 years ago
As far as I know it's ready to go.

Comment 20

10 years ago
Created attachment 293824 [details] [diff] [review]
update to tip

This also fixes compilation on Windows where LoadImage is redefined by Windows.h
Attachment #282752 - Attachment is obsolete: true

Updated

10 years ago
Attachment #293824 - Flags: approval1.9?

Comment 21

10 years ago
Asking for approval based on comment 18. Note that this is new functionality but it's fairly well isolated as a new filter.

Updated

10 years ago
Attachment #293824 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 22

10 years ago
Created attachment 295124 [details] [diff] [review]
checkin version - update to tip again
(Assignee)

Comment 23

10 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 410602

Updated

10 years ago
Depends on: 410659

Updated

10 years ago
Depends on: 411120

Updated

9 years ago
Blocks: 455986
You need to log in before you can comment on or make changes to this bug.