Summary: | The SVGDocument of an SVGImage should not perform any additional actions when the SVGImage is being destroyed | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Radu Stavila <stavila> | ||||||
Component: | CSS | Assignee: | Radu Stavila <stavila> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | commit-queue, d-r, esprehn+autocc, fmalita, gyuyoung.kim, japhet, kangil.han, koivisto, mihnea, pdr, schenney, sergio, simon.fraser | ||||||
Priority: | P2 | Keywords: | AdobeTracked | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 139294, 139592 | ||||||||
Attachments: |
|
Description
Radu Stavila
2014-12-15 05:24:54 PST
Created attachment 243294 [details]
Patch
Comment on attachment 243294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243294&action=review > Source/WebCore/dom/Document.h:534 > + inline bool isBeingDestroyed() const { return m_isBeingDestroyed; } > + inline void setIsBeingDestroyed() { m_isBeingDestroyed = true; } This sounds very generic yet it is only ever set for SVG images. Can we just have isSVGImage() or similar? Other places that check for that seem to use frame()->page()->chrome().client().isSVGImageChromeClient(). Comment on attachment 243294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243294&action=review >> Source/WebCore/dom/Document.h:534 >> + inline void setIsBeingDestroyed() { m_isBeingDestroyed = true; } > > This sounds very generic yet it is only ever set for SVG images. > > Can we just have isSVGImage() or similar? Other places that check for that seem to use frame()->page()->chrome().client().isSVGImageChromeClient(). I can do that but since the actions that are taken when the flag is set are not SVG-specific but rather specific for any document being destroyed, isn't this more consistent? Another thing would be that I would like to only affect the particular scenario of the SVGImage being destroyed, I don't want to change it for all SVGImages, in any scenario. (In reply to comment #4) > Another thing would be that I would like to only affect the particular > scenario of the SVGImage being destroyed, I don't want to change it for all > SVGImages, in any scenario. What are those scenarios and why don't you want to change them? Because I don't know all the cases when closeURL might be called on SVGDocuments attached to SVGImages and I feel keeping this restricted to only the problem at hand is the safest way to avoid unforeseen efects. Comment on attachment 243294 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=243294&action=review >>> Source/WebCore/dom/Document.h:534 >>> + inline void setIsBeingDestroyed() { m_isBeingDestroyed = true; } >> >> This sounds very generic yet it is only ever set for SVG images. >> >> Can we just have isSVGImage() or similar? Other places that check for that seem to use frame()->page()->chrome().client().isSVGImageChromeClient(). > > I can do that but since the actions that are taken when the flag is set are not SVG-specific but rather specific for any document being destroyed, isn't this more consistent? I don't like having both this and RenderObject::documentBeingDestroyed() (which calls Document::renderTreeBeingDestroyed()) which means something different. Either they should be true at the same time, or they need names that distinguish them. Created attachment 243355 [details]
Implemented reviewer feedback
Comment on attachment 243355 [details] Implemented reviewer feedback Clearing flags on attachment: 243355 Committed r177441: <http://trac.webkit.org/changeset/177441> All reviewed patches have been landed. Closing bug. *** Bug 139592 has been marked as a duplicate of this bug. *** |