Bug 102967 – Make CachedSVGDocumentReference independent of FilterOperation

• Component: CSS
• Status: RESOLVED FIXED

[Dirk Schulze]

Make CachedSVGDocumentReference independent of FilterOperation to reuse it on clip-path and masking.

[Dirk Schulze]

Created attachment 175495 [details]
Patch

[Antti Koivisto]

Comment on attachment 175495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review

r=me

> Source/WebCore/platform/graphics/filters/FilterOperation.h:-169
> -    class Data {
> -    public:
> -        virtual ~Data() { }
> -    };

Huh. Good to kill this.

> Source/WebCore/rendering/FilterEffectRenderer.cpp:125
>  PassRefPtr<FilterEffect> FilterEffectRenderer::buildReferenceFilter(Document* document, PassRefPtr<FilterEffect> previousEffect, ReferenceFilterOperation* op)

'filterOperation' would be a better name than 'op'

[Dirk Schulze]

Committed r135455: <http://trac.webkit.org/changeset/135455>

[Stephen White]

Comment on attachment 175495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review

>> Source/WebCore/platform/graphics/filters/FilterOperation.h:-169
>> -    };
> 
> Huh. Good to kill this.

The reason there was this done this way in the first place was to avoid a layering violation, by having platform depend on css or SVG.  This change adds a dependency on loader/, which seems to have lots of dependencies on SVG.  So this definitely seems like a layering violation too.

That being said, it looks like platform/ -> loader/ are pretty common, so this is already a pretty leaky boat.  :(

[Sam Weinig]

(In reply to comment #4)
> (From update of attachment 175495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review
> 
> >> Source/WebCore/platform/graphics/filters/FilterOperation.h:-169
> >> -    };
> > 
> > Huh. Good to kill this.
> 
> The reason there was this done this way in the first place was to avoid a layering violation, by having platform depend on css or SVG.  This change adds a dependency on loader/, which seems to have lots of dependencies on SVG.  So this definitely seems like a layering violation too.
> 
> That being said, it looks like platform/ -> loader/ are pretty common, so this is already a pretty leaky boat.  :(

Please do not add to this! There should be no new layering violations please.

[Dirk Schulze]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 175495 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review
> > 
> > >> Source/WebCore/platform/graphics/filters/FilterOperation.h:-169
> > >> -    };
> > > 
> > > Huh. Good to kill this.
> > 
> > The reason there was this done this way in the first place was to avoid a layering violation, by having platform depend on css or SVG.  This change adds a dependency on loader/, which seems to have lots of dependencies on SVG.  So this definitely seems like a layering violation too.
> > 
> > That being said, it looks like platform/ -> loader/ are pretty common, so this is already a pretty leaky boat.  :(
> 
> Please do not add to this! There should be no new layering violations please.

There was already a violation before. I prefer readable code before code that just hides this issue. Can you open a new bug report for fixing the violation in a proper way please?

[Sam Weinig]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 175495 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/filters/FilterOperation.h:-169
> > > >> -    };
> > > > 
> > > > Huh. Good to kill this.
> > > 
> > > The reason there was this done this way in the first place was to avoid a layering violation, by having platform depend on css or SVG.  This change adds a dependency on loader/, which seems to have lots of dependencies on SVG.  So this definitely seems like a layering violation too.
> > > 
> > > That being said, it looks like platform/ -> loader/ are pretty common, so this is already a pretty leaky boat.  :(
> > 
> > Please do not add to this! There should be no new layering violations please.
> 
> There was already a violation before. I prefer readable code before code that just hides this issue. Can you open a new bug report for fixing the violation in a proper way please?

Can you explain what the existing issue was, as I don't see it.

[Dirk Schulze]

Comment on attachment 175495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review

> Source/WebCore/css/CachedSVGDocumentReference.h:-37
> -class CachedSVGDocumentReference : public ReferenceFilterOperation::Data, public CachedSVGDocumentClient {

CachedSVGDocumentReference that inherits from Data, defined in ReferenceFilterOperation and CachedSVGDocumentClient, but is stored in a member variable of ReferenceFilterOperation in comparison to CachedSVGDocumentReference which just inherits from CachedSVGDocumentClient and is still stored in a member variable in ReferenceFilterOperation? Not much of a difference, with the exception that this looks a lot cleaner IMO.

This makes it possible to reuse this class for ReferenceMaskOperation and ReferenceClipPathOperation. Alternatives could be:

class CachedSVGDocumentReference : public ReferenceFilterOperation::Data, public ReferenceMaskOperation::Data, public ReferenceClipPathOperation::Data, public CachedSVGDocumentClient ...

, or create a base class and add multiple inheritances to the former named classes.

Of course you could see the old CachedSVGDocumentReference as some kind of behavioral design pattern, but in general it makes it harder to understand the code. I am happy to find a proper solution and don't think that RenferenceFilterOperation should have access to the document at all IMO.

I would prefer a new bug report to fix this issue since I plan other changes over the next weeks and months anyway. But if you feel strongly about it, we can roll the patch out for now.

[Sam Weinig]

(In reply to comment #8)
> (From update of attachment 175495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review
> 
> > Source/WebCore/css/CachedSVGDocumentReference.h:-37
> > -class CachedSVGDocumentReference : public ReferenceFilterOperation::Data, public CachedSVGDocumentClient {
> 
> CachedSVGDocumentReference that inherits from Data, defined in ReferenceFilterOperation and CachedSVGDocumentClient, but is stored in a member variable of ReferenceFilterOperation in comparison to CachedSVGDocumentReference which just inherits from CachedSVGDocumentClient and is still stored in a member variable in ReferenceFilterOperation? Not much of a difference, with the exception that this looks a lot cleaner IMO.


You misunderstand, the issue is that platform/ cannot #include non-platform/, not the other way around.

[Dirk Schulze]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 175495 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review
> > 
> > > Source/WebCore/css/CachedSVGDocumentReference.h:-37
> > > -class CachedSVGDocumentReference : public ReferenceFilterOperation::Data, public CachedSVGDocumentClient {
> > 
> > CachedSVGDocumentReference that inherits from Data, defined in ReferenceFilterOperation and CachedSVGDocumentClient, but is stored in a member variable of ReferenceFilterOperation in comparison to CachedSVGDocumentReference which just inherits from CachedSVGDocumentClient and is still stored in a member variable in ReferenceFilterOperation? Not much of a difference, with the exception that this looks a lot cleaner IMO.
> 
> 
> You misunderstand, the issue is that platform/ cannot #include non-platform/, not the other way around.

Yes, I think I am confused when I look at it. The files are not dependent on content in platform/ and have nothing platform dependent, neither CachedSVGDocument nor CachedSVGDocumentClient. But I didn't check if there are inheriting classes from CachedResource. Why should CachedSVGDocument be platform dependent?

[Simon Fraser (smfr)]

Comment on attachment 175495 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review

> Source/WebCore/platform/graphics/filters/FilterOperation.h:31
> +#include "CachedSVGDocumentReference.h"

Here's your layering violation right here.

[Simon Fraser (smfr)]

I agree with Sam: this patch is adding a new layering violation.

[Dirk Schulze]

(In reply to comment #11)
> (From update of attachment 175495 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review
> 
> > Source/WebCore/platform/graphics/filters/FilterOperation.h:31
> > +#include "CachedSVGDocumentReference.h"
> 
> Here's your layering violation right here.

Ah sorry, that is right. Thought that the file is in WebCore/css/style where it belongs to IMO. There is no reason why this file is in platform. Same for FilterOperations. Would you be ok to move these files to the proper locations?

[Simon Fraser (smfr)]

Aren't they in platform/ because GraphicsLayer needs them?

[Stephen White]

(In reply to comment #13)
> (In reply to comment #11)
> > (From update of attachment 175495 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review
> > 
> > > Source/WebCore/platform/graphics/filters/FilterOperation.h:31
> > > +#include "CachedSVGDocumentReference.h"
> > 
> > Here's your layering violation right here.
> 
> Ah sorry, that is right. Thought that the file is in WebCore/css/style where it belongs to IMO. There is no reason why this file is in platform. Same for FilterOperations. Would you be ok to move these files to the proper locations?

That doesn't work either, because lots of files in platform/ depend on these files, e.g., platform/graphics/GraphicsLayer.h
platform/graphics/ca/PlatformCAAnimation.h
platform/graphics/texmap/TextureMapper.h
etc.

So then those would become layer violations.

[Dirk Schulze]

(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > (From update of attachment 175495 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review
> > > 
> > > > Source/WebCore/platform/graphics/filters/FilterOperation.h:31
> > > > +#include "CachedSVGDocumentReference.h"
> > > 
> > > Here's your layering violation right here.
> > 
> > Ah sorry, that is right. Thought that the file is in WebCore/css/style where it belongs to IMO. There is no reason why this file is in platform. Same for FilterOperations. Would you be ok to move these files to the proper locations?
> 
> That doesn't work either, because lots of files in platform/ depend on these files, e.g., platform/graphics/GraphicsLayer.h
> platform/graphics/ca/PlatformCAAnimation.h
> platform/graphics/texmap/TextureMapper.h
> etc.
> 
> So then those would become layer violations.

Right, let me think about that. Then the code is actually not that 
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > (From update of attachment 175495 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=175495&action=review
> > > 
> > > > Source/WebCore/platform/graphics/filters/FilterOperation.h:31
> > > > +#include "CachedSVGDocumentReference.h"
> > > 
> > > Here's your layering violation right here.
> > 
> > Ah sorry, that is right. Thought that the file is in WebCore/css/style where it belongs to IMO. There is no reason why this file is in platform. Same for FilterOperations. Would you be ok to move these files to the proper locations?
> 
> That doesn't work either, because lots of files in platform/ depend on these files, e.g., platform/graphics/GraphicsLayer.h
> platform/graphics/ca/PlatformCAAnimation.h
> platform/graphics/texmap/TextureMapper.h
> etc.
> 
> So then those would become layer violations.

Ah right, there is a bad dependency graph on FilterOperations. Hm... I'll look into it tomorrow again.