Status

()

defect
RESOLVED FIXED
12 years ago
11 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

12 years ago
Posted patch feImage for raster images (obsolete) — Splinter Review
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

12 years ago
Posted patch use ALL (obsolete) — Splinter Review
Attachment #274687 - Attachment is obsolete: true
Attachment #274999 - Flags: review?(longsonr)
Attachment #274999 - Flags: review?(longsonr) → review+
(Assignee)

Updated

12 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

12 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?
(Assignee)

Comment 6

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

Comment 8

12 years ago
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

12 years ago
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

12 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

12 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

12 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

12 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

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

Comment 17

12 years ago
Posted 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.
Posted 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+
(Assignee)

Comment 22

12 years ago
(Assignee)

Comment 23

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

Updated

12 years ago
Depends on: 410602

Updated

12 years ago
Depends on: 410659
Depends on: 411120

Updated

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