Closed Bug 390379 Opened 13 years ago Closed 13 years ago

Implement feImage

Categories

(Core :: SVG, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: tor, Assigned: tor)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 6 obsolete files)

Attached patch feImage for raster images (obsolete) — Splinter Review
No description provided.
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).

Attached patch use ALL (obsolete) — Splinter Review
Attachment #274687 - Attachment is obsolete: true
Attachment #274999 - Flags: review?(longsonr)
Attachment #274999 - Flags: review?(longsonr) → review+
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.
(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?
Attachment #276533 - Flags: superreview?(roc)
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.
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.
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.
(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).

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+
(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.
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?
Attachment #276655 - Flags: approval1.9?
Attached patch update to tip (obsolete) — Splinter Review
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?
As far as I know it's ready to go.
Attached patch update to tipSplinter Review
This also fixes compilation on Windows where LoadImage is redefined by Windows.h
Attachment #282752 - Attachment is obsolete: true
Attachment #293824 - Flags: approval1.9?
Asking for approval based on comment 18. Note that this is new functionality but it's fairly well isolated as a new filter.
Attachment #293824 - Flags: approval1.9? → approval1.9+
Checked in.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 410602
Depends on: 410659
Depends on: 411120
Blocks: 455986
You need to log in before you can comment on or make changes to this bug.