Bug 139092 – [SVG Masking] Add support for referencing <mask> elements from -webkit-mask-image

• Component: CSS
• Status: REOPENED

[Radu Stavila]

Because the patch in https://bugs.webkit.org/show_bug.cgi?id=129682 is quite large, I will break it into two smaller patches. This is the first one and it contains all the new classes created for this patch but does not affect anything in the current functionality. The following patch will link all the pieces together.

[Radu Stavila]

Created attachment 242271 [details]
Patch

[WebKit Commit Bot]

Attachment 242271 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/style/FillLayer.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radu Stavila]

Comment on attachment 242271 [details]
Patch

@Dirk, Simon: Please review.

[Dirk Schulze]

Comment on attachment 242271 [details]
Patch

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

Really great patch! You did not add the copyright in all files that you touched. Inline more comments and questions. I can review the patch as a whole but would like to have a second pair of eyes to look at the image loader and caching part. Simon, could you take a look please?

> Source/WebCore/ChangeLog:31
> +        It only adds support for referencing <mask> elements for the -webkit-mask-image 

There is nothing about that that couldn't be tested?

> Source/WebCore/ChangeLog:86
> +        (WebCore::MaskImageOperation::notifyFinished): This is the method that gets called when the document has finished downloading and checks if it can find a valid <mask> element.

Add a line break here.

> Source/WebCore/ChangeLog:89
> +        * rendering/RenderBoxModelObject.cpp: The BackgroundImageGeometry class was moved out of RenderBoxModelObject in order to be used as a parameter for other methods. This was necessary to avoid having methods with very many parameters.

Ditto. This could have been the first patch, just moving out BackgroundImageGeometry.

> Source/WebCore/ChangeLog:135
> +        * rendering/RenderLayerMaskImageInfo.cpp: Added.

For this file, you need to add per line comments what MaskImageInfor is doing in each method.

> Source/WebCore/css/StyleResolver.cpp:3629
> +    outOperations.clear();

If you create the mask image operation, shouldn't this be empty? Or are you reusing it? If it should be empty, assert instead.

> Source/WebCore/css/StyleResolver.cpp:3638
> +        CSSPrimitiveValue& primitiveValue = downcast<CSSPrimitiveValue>(*inValue);
> +        if (primitiveValue.getValueID() == CSSValueNone)
> +            return true;

why not return false if is not 'none'?

I wonder if it should be: return (downcast<CSSPrimitiveValue>(*inValue).getValueID() == CSSValueNone);

> Source/WebCore/css/StyleResolver.cpp:3642
> +    if (!is<CSSValueList>(*inValue))
> +        return false;

Ok, you would fall out here, still.

> Source/WebCore/css/StyleResolver.cpp:3655
> +                newMaskImage = MaskImageOperation::create(&maskImageValue, "", "", false, m_state.document().cachedResourceLoader());

Maybe you want to add a send create() function to avoid these passes. I wonder why you still pass a loader even though there is nothing to load.

> Source/WebCore/css/StyleResolver.cpp:3672
> +        if (newMaskImage.get())
> +            outOperations.append(newMaskImage);

If one of the above operations fails and we do not end up with a  newMaskImage, don't we have a list of less mask operations than the user specified? To know the order of the applied resources is important for mask-composite.

> Source/WebCore/loader/cache/CachedSVGDocument.h:40
> +    virtual bool canReuse(const ResourceRequest&) const override { return m_canReuseResource; }

Why not call it canReuseResource() as well? Sounds better.

> Source/WebCore/loader/cache/CachedSVGDocument.h:41
> +    void setCanReuse(bool canReuseResource) { m_canReuseResource = canReuseResource; };

Ditto with set*

> Source/WebCore/platform/graphics/MaskImageOperation.cpp:48
> +PassRefPtr<MaskImageOperation> MaskImageOperation::create(PassRefPtr<StyleImage> generatedImage)

As mentioned above, maybe you can add a create() function for the none case. Also, it seems that they are short enough to put them into the header.

> Source/WebCore/platform/graphics/MaskImageOperation.cpp:81
> +        return m_cssMaskImageValue->isCSSValueNone();

Shouldn't there always be a valid m_cssMaskImageValue? Should this be an assertion instead?

> Source/WebCore/platform/graphics/MaskImageOperation.cpp:104
> +    if (image())
> +        return (image()->cachedImage() && image()->cachedImage()->image());

What about generated images? For those, cachedImage() would return 0 and therefore false. Pending image resources would return 0 as well, but that is probably correct here.

> Source/WebCore/platform/graphics/MaskImageOperation.cpp:114
> +void MaskImageOperation::setRenderLayerImageClient(CachedImageClient* client)

I think in these methods it is correct not to look at generated images.

> Source/WebCore/platform/graphics/MaskImageOperation.cpp:226
> +    // Identify the element referenced by the fragmentId.
> +    CachedSVGDocumentReference* svgDocumentReference = cachedSVGDocumentReference();
> +    Element* elementForMasking = nullptr;
> +    RenderObject* svgResourceForMasking = nullptr;
> +    if (svgDocumentReference && svgDocumentReference->document()) {
> +        SVGDocument* svgDocument = svgDocumentReference->document()->document();
> +        if (svgDocument && svgDocument->rootElement())
> +            elementForMasking = svgDocument->rootElement()->getElementById(fragment());
> +    } else
> +        elementForMasking = renderer.document().getElementById(fragment());

This function just gets the element, could you put the code into an inline function with a descriptive name?

[Radu Stavila]

Comment on attachment 242271 [details]
Patch

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

>> Source/WebCore/ChangeLog:31
>> +        It only adds support for referencing <mask> elements for the -webkit-mask-image 
> 
> There is nothing about that that couldn't be tested?

Unfortunately no, all the code added here is "dead", it's not called anywhere so there's nothing to test. I looked at multiple ways of splitting this patch and having some intermediary tests but there are so many existing tests for -webkit-mask-image that it's impossible to change something without breaking existing tests, unless I change everything all the way.

>> Source/WebCore/css/StyleResolver.cpp:3638
>> +            return true;
> 
> why not return false if is not 'none'?
> 
> I wonder if it should be: return (downcast<CSSPrimitiveValue>(*inValue).getValueID() == CSSValueNone);

The code that will call this method (part2 of the patch) will only apply it to the style if this returns true. As such, I want it to return true for none so it would apply it and clear any old masks.

>> Source/WebCore/loader/cache/CachedSVGDocument.h:40
>> +    virtual bool canReuse(const ResourceRequest&) const override { return m_canReuseResource; }
> 
> Why not call it canReuseResource() as well? Sounds better.

It's not a new method, it's an override of an existing method (defined in CachedResource).

[Dirk Schulze]

Comment on attachment 242271 [details]
Patch

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

>>> Source/WebCore/css/StyleResolver.cpp:3638
>>> +            return true;
>> 
>> why not return false if is not 'none'?
>> 
>> I wonder if it should be: return (downcast<CSSPrimitiveValue>(*inValue).getValueID() == CSSValueNone);
> 
> The code that will call this method (part2 of the patch) will only apply it to the style if this returns true. As such, I want it to return true for none so it would apply it and clear any old masks.

Don't you return false in the next if clause anyway? Because if it is a primitive value and not none, you still fail on the value list check.

>> Source/WebCore/css/StyleResolver.cpp:3672
>> +            outOperations.append(newMaskImage);
> 
> If one of the above operations fails and we do not end up with a  newMaskImage, don't we have a list of less mask operations than the user specified? To know the order of the applied resources is important for mask-composite.

I'd still be interested in this question.

[Radu Stavila]

(In reply to comment #6)
> Comment on attachment 242271 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242271&action=review

> >> Source/WebCore/css/StyleResolver.cpp:3672
> >> +            outOperations.append(newMaskImage);
> > 
> > If one of the above operations fails and we do not end up with a  newMaskImage, don't we have a list of less mask operations than the user specified? To know the order of the applied resources is important for mask-composite.
> 
> I'd still be interested in this question.

I'm addressing that in the upcoming new patch.

[Radu Stavila]

Comment on attachment 242271 [details]
Patch

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

>> Source/WebCore/platform/graphics/MaskImageOperation.cpp:104
>> +        return (image()->cachedImage() && image()->cachedImage()->image());
> 
> What about generated images? For those, cachedImage() would return 0 and therefore false. Pending image resources would return 0 as well, but that is probably correct here.

It will not get here for generated image because of the previous if statement. m_isExternalDocument will be false for generated images.

[Radu Stavila]

Created attachment 242495 [details]
Patch integrating Dirk's feedback

[Simon Fraser (smfr)]

Comment on attachment 242495 [details]
Patch integrating Dirk's feedback

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

> Source/WebCore/css/StyleResolver.cpp:3477
> +    State& state = m_state;

Why do you need this variable?

> Source/WebCore/css/StyleResolver.cpp:3489
> +        RefPtr<MaskImageOperation> newMaskImage = nullptr;

No need to initialize to nullptr.

> Source/WebCore/css/StyleResolver.cpp:3502
> +                    state.maskImagesWithPendingSVGDocuments().append(newMaskImage);

m_state

> Source/WebCore/css/StyleResolver.h:241
>      bool createFilterOperations(CSSValue* inValue, FilterOperations& outOperations);
> +    bool createMaskImageOperations(CSSValue* inValue, Vector<RefPtr<MaskImageOperation>>& outOperations);

It's a bit odd that we have FilterOperations, but not MaskImageOperations.

> Source/WebCore/loader/cache/CachedSVGDocument.h:53
> +    unsigned m_shouldCreateFrameForDocument : 1;
> +    unsigned m_canReuseResource : 1;

I don't think it's worth using bit flags here.

> Source/WebCore/page/Page.cpp:279
> +Page* Page::createPageFromBuffer(PageConfiguration& pageConfiguration, const SharedBuffer* buffer, const String& mimeType, bool canHaveScrollbars, bool transparent)

This should return a unique_ptr<>

> Source/WebCore/platform/ScrollView.h:413
> +    virtual bool isSVGDocument() const { return false; }

This seems like the wrong level of abstraction. The scrollview contains a document, it is not a document.

> Source/WebCore/platform/graphics/MaskImageOperation.h:73
> +    CachedSVGDocumentReference* getOrCreateCachedSVGDocumentReference();

Maybe ensureCachedSVGDocumentReference()?

> Source/WebCore/platform/graphics/MaskImageOperation.h:75
> +    virtual void notifyFinished(CachedResource*);

override?

> Source/WebCore/rendering/RenderLayerMaskImageInfo.cpp:102
> +        CachedSVGDocument* cachedSVGDocument = documentReference ? documentReference->document() : 0;

nullptr

> Source/WebCore/rendering/style/FillLayer.h:185
> +    RefPtr<MaskImageOperation> m_maskImageOperation;

I'm a little concerned about increasing the size of FillLayer for something that is very rarely used.

> Source/WebCore/rendering/svg/RenderSVGResourceMasker.h:56
> +    MaskerData* maskerDataForRenderer(RenderObject& renderer) { return m_masker.get(&renderer); }

Could this return a const *?

[Radu Stavila]

Comment on attachment 242495 [details]
Patch integrating Dirk's feedback

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

>> Source/WebCore/rendering/svg/RenderSVGResourceMasker.h:56
>> +    MaskerData* maskerDataForRenderer(RenderObject& renderer) { return m_masker.get(&renderer); }
> 
> Could this return a const *?

Not really because it's member maskImage is then used as a parameter to GraphicsContext::drawImageBuffer, which doesn't take it as const.

[Radu Stavila]

Created attachment 242563 [details]
Integrated Simon's feedback

[Radu Stavila]

Created attachment 242569 [details]
Added 'reviewed by' in ChangeLog

[WebKit Commit Bot]

Comment on attachment 242569 [details]
Added 'reviewed by' in ChangeLog

Clearing flags on attachment: 242569

Committed r176798: <http://trac.webkit.org/changeset/176798>

[Simon Fraser (smfr)]

(In reply to comment #13)
> Created attachment 242569 [details]
> Added 'reviewed by' in ChangeLog

Commit queue will do that for you.

[Radu Stavila]

(In reply to comment #15)
> (In reply to comment #13)
> > Created attachment 242569 [details]
> > Added 'reviewed by' in ChangeLog
> 
> Commit queue will do that for you.

Ha, good to know. Is this a recent change in the bots? I remember when I forgot to add it some time ago and it didn't like the fact that it found "OOPS" in the CL.

[Tim Nguyen (:ntim)]

Reverted in bug 146653

[Radar WebKit Bug Importer]

<rdar://problem/114683744>