Closed
Bug 390379
Opened 17 years ago
Closed 16 years ago
Implement feImage
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: tor, Assigned: tor)
References
(Blocks 2 open bugs)
Details
Attachments
(2 files, 6 obsolete files)
27.11 KB,
patch
|
damons
:
approval1.9+
|
Details | Diff | Splinter Review |
27.26 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Comment 1•17 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).
Attachment #274687 -
Attachment is obsolete: true
Attachment #274999 -
Flags: review?(longsonr)
Updated•17 years ago
|
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?
let's see how #2 looks?
Attachment #276533 -
Flags: superreview?(roc)
Comment 7•17 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.
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•17 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•17 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).
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•17 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•17 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?
Attachment #276655 -
Flags: approval1.9?
Assignee | ||
Comment 17•17 years ago
|
||
Attachment #276655 -
Attachment is obsolete: true
Updated•17 years ago
|
Assignee: nobody → tor
Comment 18•17 years ago
|
||
Given the state of things now, I think we should take this bug. What's the current status?
Comment 19•17 years ago
|
||
As far as I know it's ready to go.
Comment 20•17 years ago
|
||
This also fixes compilation on Windows where LoadImage is redefined by Windows.h
Attachment #282752 -
Attachment is obsolete: true
Updated•17 years ago
|
Attachment #293824 -
Flags: approval1.9?
Comment 21•17 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•17 years ago
|
Attachment #293824 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 22•16 years ago
|
||
Assignee | ||
Comment 23•16 years ago
|
||
Checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•