Bug 129682 - url()'s for background-image, mask-image, border-image, mask-box-image should support SVG resources
Summary: url()'s for background-image, mask-image, border-image, mask-box-image should...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Radu Stavila
URL:
Keywords:
Depends on: 139294 139092
Blocks: 95389 139440 139442
  Show dependency treegraph
 
Reported: 2014-03-04 06:52 PST by Dirk Schulze
Modified: 2015-01-05 01:41 PST (History)
15 users (show)

See Also:


Attachments
shape.svg (1.60 KB, image/svg+xml)
2014-07-02 05:20 PDT, Radu Stavila
no flags Details
sample.html (406 bytes, text/html)
2014-07-02 05:22 PDT, Radu Stavila
no flags Details
WIP (83.56 KB, patch)
2014-10-16 08:06 PDT, Radu Stavila
no flags Details | Formatted Diff | Diff
WIP2 (88.13 KB, patch)
2014-10-24 05:33 PDT, Radu Stavila
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (1.05 MB, application/zip)
2014-10-24 10:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (844.61 KB, application/zip)
2014-10-24 13:01 PDT, Build Bot
no flags Details
New layout tests (15.28 KB, patch)
2014-10-27 08:04 PDT, Radu Stavila
no flags Details | Formatted Diff | Diff
WIP3 (123.79 KB, patch)
2014-11-03 09:14 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
WIP4 (130.58 KB, patch)
2014-11-06 09:07 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (713.03 KB, application/zip)
2014-11-06 10:32 PST, Build Bot
no flags Details
WIP5 - Final WIP before final patch (162.68 KB, patch)
2014-11-20 05:51 PST, Radu Stavila
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (620.52 KB, application/zip)
2014-11-20 07:19 PST, Build Bot
no flags Details
Added new files to Win/GTK/EFL and fixed failing tests (166.56 KB, patch)
2014-11-20 07:41 PST, Radu Stavila
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (600.55 KB, application/zip)
2014-11-20 09:10 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (621.26 KB, application/zip)
2014-11-20 09:14 PST, Build Bot
no flags Details
Final WIP (170.63 KB, patch)
2014-11-25 12:24 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Fixed windows build, cleaned up code and performed some refactoring (171.11 KB, patch)
2014-11-26 08:38 PST, Radu Stavila
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (579.43 KB, application/zip)
2014-11-26 10:07 PST, Build Bot
no flags Details
Addressed some of the comments in the previous review (170.03 KB, patch)
2014-11-27 10:33 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
Latest patch, EWS testing (181.08 KB, patch)
2014-12-03 09:23 PST, Radu Stavila
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (688.39 KB, application/zip)
2014-12-03 11:04 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (1.24 MB, application/zip)
2014-12-03 11:35 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (1.31 MB, application/zip)
2014-12-03 13:29 PST, Build Bot
no flags Details
EWS testing (191.44 KB, patch)
2014-12-04 04:14 PST, Radu Stavila
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (871.56 KB, application/zip)
2014-12-04 05:32 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (781.08 KB, application/zip)
2014-12-04 06:28 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (585.77 KB, application/zip)
2014-12-04 08:26 PST, Build Bot
no flags Details
EWS testing (84.78 KB, patch)
2014-12-04 09:47 PST, Radu Stavila
simon.fraser: review-
Details | Formatted Diff | Diff
EWS testing after some refactoring (84.25 KB, patch)
2014-12-05 03:49 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
EWS testing for a simplified version (93.60 KB, patch)
2014-12-05 09:24 PST, Radu Stavila
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (712.47 KB, application/zip)
2014-12-05 10:48 PST, Build Bot
no flags Details
EWS testing (93.97 KB, patch)
2014-12-05 12:23 PST, Radu Stavila
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (789.36 KB, application/zip)
2014-12-05 17:04 PST, Build Bot
no flags Details
EWS testing (98.19 KB, patch)
2014-12-08 09:30 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
EWS testing (98.19 KB, patch)
2014-12-08 11:50 PST, Radu Stavila
no flags Details | Formatted Diff | Diff
EWS testing (97.71 KB, patch)
2014-12-11 04:07 PST, Radu Stavila
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dirk Schulze 2014-03-04 06:52:07 PST
Starting with mask-image, CSS properties that take and CSS Image value might support SVG resources as well.

mask-image needs to be able to reference <mask> elements and CSS Images. Referencing happens by fragment identifier.

mask-image: url(image.png) - should be a CSSImage
mask-image: url(image.svg) - should be a CSSImage
mask-image: url(#resource) - should be an SVGResource

Sadly we can not identify the type during parsing:

mask-image: url(image.png#frag) - should PROBABLY be an CSSImage (depends if the resource is really an PNG)
mask-image: url(image.svg#frag) -
    * might be a reference to a <mask> element within image.svg
    * might also be a so called SVG Stack where just the active element gets rendered.
    * might reference another SVGResource like a <clipPath> in which case the fragment should be ignored.
    * might reference an SVG paint server like <linearGradient> in which case we want to use it to fill the mask area. (This is the interesting case for background-image, border-image and mask-box image but even more interesting for fill and stroke.)

It might also be a media query: #xywh() or #t() which are reserved and can be identified at parse time. These should be transformed to an CSSImage. SVG has some more reserved fragment identifiers. I don't think we support them from SVG.


mask-image can have the CSSValues: CSSImageValue, CSSImageGeneratedValue and CSSImageSetValue (See diagram https://docs.google.com/a/chromium.org/drawings/d/1FqPsPNvj-mX2HXQHU6lp3Kv8Y2UcEvfhv5D_kOjeDXw/edit)

It might be an idea to have a new CSSValue for all URLs with fragment identifiers. Thankfully this should be very rare on the web for normal images. We probably won't break content if the CSSValue should not be reliable at the beginning, but of course we still need to be caution.

This new CSSValues would be able to fallback to CSSImage values but can handle the different SVGResource types. I did not think about the details yet. I am very open for feedback and discussions.
Comment 1 Radu Stavila 2014-07-02 05:20:55 PDT
Created attachment 234254 [details]
shape.svg
Comment 2 Radu Stavila 2014-07-02 05:22:07 PDT
Created attachment 234255 [details]
sample.html
Comment 3 Alexey Proskuryakov 2014-10-15 14:55:08 PDT
See also: bug 15443.
Comment 4 Radu Stavila 2014-10-16 08:06:05 PDT
Created attachment 239949 [details]
WIP

Attached WIP. In its current state, this patch fixes:
- using URL fragment identifiers to reference <mask> elements in external SVG files
- using URL fragment identifiers to reference <*Gradient> elements in external SVG files
- using URL fragment identifiers to reference inline <mask> elements
- using URL fragment identifiers to reference inline <*Gradient> elements
- using animated masks (some remaining issues for external SVG documents)

Some work still needed for:
- animated masks from external documents
- using SVG stacks (worked in an early prototype but doesn't work anymore after a redesign needed to use inline SVG elements)
- test changing masks properties via script
Comment 5 WebKit Commit Bot 2014-10-16 08:08:09 PDT
Attachment 239949 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.h:1261:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/style/FillLayer.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Dirk Schulze 2014-10-17 01:15:52 PDT
Comment on attachment 239949 [details]
WIP

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

Just some initial thoughts and questions.

> Source/WebCore/css/CSSParser.cpp:9431
> +    for (auto value = valueList->current(); value; value = valueList->next()) {
> +        if (value->unit != CSSPrimitiveValue::CSS_URI)
> +            return false;

Doesn't that exclude all other CSS image values like CSS gradients, cross fade? Why not handle what ever we want to handle in parseFillImage? We'll need the logic there anyway at some point to support gradient and pattern references.

> Source/WebCore/css/StyleResolver.cpp:-3500
> -    if (!state.style() || !state.style()->hasFilter() || state.filtersWithPendingSVGDocuments().isEmpty())

So we don't return earlier for other resources that don't require these steps? Are there any?

> Source/WebCore/css/StyleResolver.cpp:3523
> +            maskImageOperation->getOrCreateCachedSVGDocumentReference()->load(cachedResourceLoader);

Ok, so if we handle it like Filters, don't we run into the same problem that we don't create renderer for the elements in the cached document as it is the case for external filters? If yes, then external masks are pretty much useless, if no, does your patch fix filters as well? Or did you figure out why we never created renderers in the first place?

> Source/WebCore/css/StyleResolver.cpp:3826
> +        case CSSPropertyWebkitMaskImage: { // should go away?

Why should go away? Because we handle it above? But this code seems to handle FillLayers. Don't you use them anymore?

> Source/WebCore/css/WebKitCSSMaskImageValue.h:28
> +#ifndef WebKitCSSMaskImageValue_h
> +#define WebKitCSSMaskImageValue_h

A much more generic version would be great. Maybe share with what we need for clip-path and filter at some point. Gradients and Patterns would make use of it as well. Would be overload to add new CSSValues for each single type.

> Source/WebCore/loader/cache/CachedImage.cpp:341
> +        RefPtr<SVGImage> svgImage = SVGImage::create(this, resourceRequest().url().fragmentIdentifier());

Ok, so you still use CacheImage. Why can't we use the old path then?

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:42
> +    , m_shouldCreateFrameForDocument(true)

Hm. Does that make the document render? Is that the reason why external filters didn't work? If that is the case, maybe we can split the patch and land this part earlier for -webkit-filter and generalize it for all kind of SVG resources.

> Source/WebCore/platform/graphics/MaskImageOperation.cpp:28
> +#include "MaskImageOperation.h"

Is that the pendant to the header for Filter effects? If so we probably want to generalize it.

> Source/WebCore/platform/graphics/MaskImageOperation.h:69
> +    String m_url;
> +    String m_fragment;
> +    std::unique_ptr<CachedSVGDocumentReference> m_cachedSVGDocumentReference;

Looks pretty much identical. We we should reuse the one we have for filters.

> Source/WebCore/rendering/RenderBox.cpp:715
> +FloatRect RenderBox::repaintRectInLocalCoordinates() const
> +{
> +    return borderBoxRect();
> +}
> +
> +FloatRect RenderBox::objectBoundingBox() const
> +{
> +    return borderBoxRect();
> +}

We need to support more boxes than obb for masking. This is a WIP patch, just saying: http://dev.w3.org/fxtf/css-masking-1/#the-mask-clip

> Source/WebCore/rendering/RenderBoxModelObject.cpp:52
> +#include "RenderSVGResourceGradient.h"

Gradients should be used as a type of CSS Image, just like -webkit-filter(), -webkit-crossfade(), linear-gradient() and so on. Definitely a separate patch and needs agreement of the CSS WG as well as SVG WG. I am really excited to see that implemented at some point though! Remember that we have RenderSVGResourcePattern as well.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:889
> +            if (image.get() && image->isSVGImageForContainer()) {
> +                SVGImageForContainer* svgImageForContainer = downcast<SVGImageForContainer>(image.get());
> +                String resourceId = svgImageForContainer->fragmentIdentifier();
> +                if (resourceId.length())
> +                    svgResourceForMasking = svgImageForContainer->getSVGResource(resourceId);
> +            }  else if (bgLayer->hasMaskImage()) {
> +                const RefPtr<MaskImageOperation>& maskImageOperation = bgLayer->maskImage();
> +                CachedSVGDocumentReference* svgDocumentReference = maskImageOperation->cachedSVGDocumentReference();
> +                Element* elementForMasking = nullptr;
> +                if (svgDocumentReference && svgDocumentReference->document()) {
> +                    SVGDocument* svgDocument = svgDocumentReference->document()->document();
> +                    if (svgDocument && svgDocument->rootElement())
> +                        elementForMasking = svgDocument->rootElement()->getElementById(maskImageOperation->fragment());
> +                } else
> +                    elementForMasking = document().getElementById(maskImageOperation->fragment());
> +
> +                if (elementForMasking)
> +                    svgResourceForMasking = elementForMasking->renderer();
> +            }
> +
> +            CompositeOperator compositeOp = (op == CompositeSourceOver ? bgLayer->composite() : op);
> +            if (svgResourceForMasking) {
> +                if (svgResourceForMasking->isRenderSVGResourceMasker()) {
> +                    RenderSVGResourceMasker* masker = toRenderSVGResourceMasker(svgResourceForMasking);
> +                    masker->applySVGMask(*this, context, false);
> +                    MaskerData* maskerData = masker->maskerDataForRenderer(*this);
> +                    ASSERT(maskerData);
> +                    context->drawImageBuffer(maskerData->maskImage.get(), ColorSpaceDeviceRGB, geometry.destRect(), compositeOp);
> +                    didPaintCustomMask = true;
> +                } else if (svgResourceForMasking->isRenderSVGResourceGradient()) {
> +                    RenderSVGResourceGradient* gradient = toRenderSVGResourceGradient(svgResourceForMasking);
> +                    if (gradient->applyResource(*this, style(), context, ApplyToFillMode)) {
> +                        // Create path from the object's box and fill it with the gradient
> +                        Path path;
> +                        path.addRect(objectBoundingBox());
> +                        gradient->postApplyResource(*this, context, ApplyToFillMode, &path, nullptr);
> +                        didPaintCustomMask = true;
> +                    }
> +                }
> +            }

This looks a bit complicated, inline functions can help to sort that out.

> Source/WebCore/rendering/RenderLayerMaskImageInfo.cpp:121
> +} // namespace WebCore

Is that file a duplicate of what we have for Filters?

> Source/WebCore/rendering/RenderLayerMaskImageInfo.h:40
> +class RenderLayer::MaskImageInfo final : public CachedSVGDocumentClient {

Ditto.

> Source/WebCore/rendering/style/FillLayer.h:71
> +    const RefPtr<MaskImageOperation>& maskImage() const { return m_maskImageOperation; }

Yeah, this definitely should be generalized more for all kind of SVG resources. Especially thinking about SVG gradients and SVG Patterns. At the end of the day, all of them are images that just require some changes. Mask URL disables all other mask properties, pattern and gradient won't.

> Source/WebCore/svg/SVGUseElement.cpp:996
> +        // We don't need the SVG document to create a new frame because the new document belongs to the parent UseElement.

Did you try if it does work correctly currently?
Comment 7 Radu Stavila 2014-10-17 07:22:51 PDT
Comment on attachment 239949 [details]
WIP

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

>> Source/WebCore/css/CSSParser.cpp:9431
>> +            return false;
> 
> Doesn't that exclude all other CSS image values like CSS gradients, cross fade? Why not handle what ever we want to handle in parseFillImage? We'll need the logic there anyway at some point to support gradient and pattern references.

I couldn't find a clean way of referencing inline masks, using Image values. Could you please provide an example of the mask-image values you are concerned about?

>> Source/WebCore/css/StyleResolver.cpp:-3500
>> -    if (!state.style() || !state.style()->hasFilter() || state.filtersWithPendingSVGDocuments().isEmpty())
> 
> So we don't return earlier for other resources that don't require these steps? Are there any?

Previously, the only ones having pending SVG documents were filters, so the early return made sense. Was that your question?

>> Source/WebCore/css/StyleResolver.cpp:3523
>> +            maskImageOperation->getOrCreateCachedSVGDocumentReference()->load(cachedResourceLoader);
> 
> Ok, so if we handle it like Filters, don't we run into the same problem that we don't create renderer for the elements in the cached document as it is the case for external filters? If yes, then external masks are pretty much useless, if no, does your patch fix filters as well? Or did you figure out why we never created renderers in the first place?

I overcame that problem, external masks are handled properly. It does not currently fix filters but I think it is possible to port it. The renderers were never created because the SVGDocument was created without a Frame, so style was not applied.

>> Source/WebCore/css/StyleResolver.cpp:3826
>> +        case CSSPropertyWebkitMaskImage: { // should go away?
> 
> Why should go away? Because we handle it above? But this code seems to handle FillLayers. Don't you use them anymore?

Not sure about this one, I left the comment there as a reminder of "stuff to figure out" before finishing the patch.

>> Source/WebCore/css/WebKitCSSMaskImageValue.h:28
>> +#define WebKitCSSMaskImageValue_h
> 
> A much more generic version would be great. Maybe share with what we need for clip-path and filter at some point. Gradients and Patterns would make use of it as well. Would be overload to add new CSSValues for each single type.

Not really sure what you mean. Are you talking about using gradients for -webkit-mask-image?

>> Source/WebCore/loader/cache/CachedImage.cpp:341
>> +        RefPtr<SVGImage> svgImage = SVGImage::create(this, resourceRequest().url().fragmentIdentifier());
> 
> Ok, so you still use CacheImage. Why can't we use the old path then?

This is actually part of the first approach, the one that couldn't handle inline masks. I am no longer using SVGImage at the moment but I didn't rollback all the initial changes just yet.

>> Source/WebCore/loader/cache/CachedSVGDocument.cpp:42
>> +    , m_shouldCreateFrameForDocument(true)
> 
> Hm. Does that make the document render? Is that the reason why external filters didn't work? If that is the case, maybe we can split the patch and land this part earlier for -webkit-filter and generalize it for all kind of SVG resources.

Yes, this is what makes the external SVGDocument attach and generate renderers. I will look into splitting it from the main patch and fix filters first.

>> Source/WebCore/platform/graphics/MaskImageOperation.cpp:28
>> +#include "MaskImageOperation.h"
> 
> Is that the pendant to the header for Filter effects? If so we probably want to generalize it.

Sorry, don't really understand the question.

>> Source/WebCore/platform/graphics/MaskImageOperation.h:69
>> +    std::unique_ptr<CachedSVGDocumentReference> m_cachedSVGDocumentReference;
> 
> Looks pretty much identical. We we should reuse the one we have for filters.

Yes, investigating the possibility of merging these is on my todo list.

>> Source/WebCore/rendering/RenderBox.cpp:715
>> +}
> 
> We need to support more boxes than obb for masking. This is a WIP patch, just saying: http://dev.w3.org/fxtf/css-masking-1/#the-mask-clip

I'll make a note to look into it.

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:52
>> +#include "RenderSVGResourceGradient.h"
> 
> Gradients should be used as a type of CSS Image, just like -webkit-filter(), -webkit-crossfade(), linear-gradient() and so on. Definitely a separate patch and needs agreement of the CSS WG as well as SVG WG. I am really excited to see that implemented at some point though! Remember that we have RenderSVGResourcePattern as well.

-webkit-mask-image: url('file.svg#gradient1');

So you're saying that using a gradient as a mask image like above needs to gradient to be stored as an Image value? If so, why?

>> Source/WebCore/rendering/RenderLayerMaskImageInfo.cpp:121
>> +} // namespace WebCore
> 
> Is that file a duplicate of what we have for Filters?

Pretty much, yes, investigating the possibility of merging these is on my todo list.

>> Source/WebCore/rendering/style/FillLayer.h:71
>> +    const RefPtr<MaskImageOperation>& maskImage() const { return m_maskImageOperation; }
> 
> Yeah, this definitely should be generalized more for all kind of SVG resources. Especially thinking about SVG gradients and SVG Patterns. At the end of the day, all of them are images that just require some changes. Mask URL disables all other mask properties, pattern and gradient won't.

Can you please provide an example for the situations you're thinking about?

>> Source/WebCore/svg/SVGUseElement.cpp:996
>> +        // We don't need the SVG document to create a new frame because the new document belongs to the parent UseElement.
> 
> Did you try if it does work correctly currently?

Yes, it works correctly with this patch.
Comment 8 Radu Stavila 2014-10-24 05:33:08 PDT
Created attachment 240406 [details]
WIP2

WIP2. Difference from first WIP:
- when loading an url for mask-image and finding an invalid SVG mask (not SVG resource at all, SVG resource but no element with the requested ID exists, element inside SVG resource exists but isnt' a valid masking element) it falls back to loading the resource as an image.
- relevant change is in MaskImageOperation::notifyFinished

The following mask-image values are now working properly:
1. url('file.png') => uses the PNG image as a mask
2. url('file.svg#mask') with 'mask' identifying a <mask> element => uses the <mask> element
3. url('file.svg#grad') with 'grad' identifying a <*gradient> element => uses the <*gradient> element as a mask
4. url('file.svg#invalid') => (invalid = non-existing ID or element that can't be used as a mask) fallbacks to using the entire SVG file as a mask (like it happens without the patch)

Moving on, I'll work on using non-url values for mask-image
e.g. -webkit-mask-image: linear-gradient(black 0%, transparent 100%);
Comment 9 WebKit Commit Bot 2014-10-24 05:44:01 PDT
Attachment 240406 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.h:1263:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/style/FillLayer.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 Build Bot 2014-10-24 10:30:33 PDT
Created attachment 240413 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 11 Build Bot 2014-10-24 13:01:02 PDT
Created attachment 240430 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 12 Radu Stavila 2014-10-27 08:04:40 PDT
Created attachment 240481 [details]
New layout tests

Added patch containing new layout tests for anyone looking to play around with the new funtionalities.
Comment 13 Dirk Schulze 2014-10-28 09:58:41 PDT
Comment on attachment 240481 [details]
New layout tests

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

All tests look good to me!

> LayoutTests/css3/masking/mask-svg-invalid-fragmentId.html:14
> +                -webkit-mask-image:url('resources/masks.svg#invalidId');

This is probably the most interesting test, since it does look confusing at first. But it is following the spec and correct.
Comment 14 Dirk Schulze 2014-10-28 10:31:48 PDT
(In reply to comment #13)
> Comment on attachment 240481 [details]
> New layout tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240481&action=review
> 
> All tests look good to me!
> 
> > LayoutTests/css3/masking/mask-svg-invalid-fragmentId.html:14
> > +                -webkit-mask-image:url('resources/masks.svg#invalidId');
> 
> This is probably the most interesting test, since it does look confusing at
> first. But it is following the spec and correct.

After a discussion at TPAC from yesterday, there is one case that is missing: SVG stacks http://simurai.com/blog/2012/04/02/svg-stacks/.

We do support SVG stacks for <img> but not on the CSS side yet and it is not the task of this bug report (or any patch here) to support it. I am adding the comment purely to make sure we have a complete list of everything we want to support in the future.
Comment 15 Radu Stavila 2014-10-28 10:34:41 PDT
I have SVG stacks on my todo list. I actually had them working on the 1st version of the prototype but, after making the necessary changes to support inline SVG masks, it's not working anymore. Are you saying that I shouldn't get them working for this patch?
Comment 16 Dirk Schulze 2014-10-28 10:37:58 PDT
(In reply to comment #15)
> I have SVG stacks on my todo list. I actually had them working on the 1st
> version of the prototype but, after making the necessary changes to support
> inline SVG masks, it's not working anymore. Are you saying that I shouldn't
> get them working for this patch?

It is not a requirement! If you fix it because of the refactoring, this is absolutely fine. IIRC, we have a bug report for this but I am not sure I can find it right now.
Comment 17 Simon Fraser (smfr) 2014-10-28 11:35:18 PDT
Chatting about this at TPAC, we also had issues with the sizing behavior of SVG elements referenced like this. People really want the behavior where the referenced element is sized to fit the background, rather than using the size of the SVG document and just rendering a small part of it.
Comment 18 Dirk Schulze 2014-10-28 11:51:43 PDT
(In reply to comment #17)
> Chatting about this at TPAC, we also had issues with the sizing behavior of
> SVG elements referenced like this. People really want the behavior where the
> referenced element is sized to fit the background, rather than using the
> size of the SVG document and just rendering a small part of it.

I would like to have the discussion about that on a different bug report. SVG Stacks were implemented in Safari 8 and usage rises. I would expect that changes to the current behavior have a high risk of incompatibilities to existing content.
Comment 19 Simon Fraser (smfr) 2014-10-28 12:07:23 PDT
Comment on attachment 240406 [details]
WIP2

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

> Source/WebCore/css/WebKitCSSMaskImageValue.cpp:44
> +    const char* result = "";
> +    return result + CSSValueList::customCSSText();

Why not just return CSSValueList::customCSSText(); ?

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:85
> +            Page::PageClients pageClients;
> +            fillWithEmptyClients(pageClients);
> +            m_page = std::make_unique<Page>(pageClients);
> +            m_page->settings().setMediaEnabled(false);
> +            m_page->settings().setScriptEnabled(false);
> +            m_page->settings().setPluginsEnabled(false);
> +            
> +            Frame& frame = m_page->mainFrame();
> +            frame.setView(FrameView::create(frame));
> +            frame.init();
> +            FrameLoader& loader = frame.loader();
> +            loader.forceSandboxFlags(SandboxAll);
> +            
> +            frame.view()->setCanHaveScrollbars(false);
> +            frame.view()->setTransparent(true);
> +            
> +            ASSERT(loader.activeDocumentLoader()); // DocumentLoader should have been created by frame->init().
> +            loader.activeDocumentLoader()->writer().setMIMEType("image/svg+xml");
> +            loader.activeDocumentLoader()->writer().begin(URL()); // Create the empty document.
> +            loader.activeDocumentLoader()->writer().addData(data->data(), data->size());
> +            loader.activeDocumentLoader()->writer().end();

We should share this stuff with SVGImage.

> Source/WebCore/platform/ScrollView.cpp:1082
> +        // When using a mask from an SVG as an element's maskImage, the visibleContentRect
> +        // of the SVG document will be empty, but the element using it as a mask still
> +        // needs to be repainted.
> +        if (!visibleRect.isEmpty())
> +            paintRect.intersect(visibleRect);

This seems like it would change behavior for non-SVG cases?

> Source/WebCore/platform/graphics/MaskImageOperation.cpp:43
> +    m_url = String(o.m_url);
> +    m_fragment = String(o.m_fragment);

Why the String()?

> Source/WebCore/platform/graphics/MaskImageOperation.cpp:92
> +                CachedImage* newCachedImage = new CachedImage(cachedSVGDocument->resourceRequest(), cachedSVGDocument->sessionID());

This should use unique_ptr<>

> Source/WebCore/platform/graphics/MaskImageOperation.h:50
> +    ~MaskImageOperation() { }

Should be virtual?

> Source/WebCore/rendering/RenderBox.cpp:715
> +FloatRect RenderBox::repaintRectInLocalCoordinates() const
> +{
> +    return borderBoxRect();
> +}
> +
> +FloatRect RenderBox::objectBoundingBox() const
> +{
> +    return borderBoxRect();
> +}

It would seem that these overrides are going to change behavior. If they are only used for SVG, their names should be changed to make that clearer.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:904
> +        if (!geometry.destRect().isEmpty()) {
> +            bool didPaintCustomMask = false;
> +            RenderObject* svgResourceForMasking = nullptr;
> +            if (image.get() && image->isSVGImageForContainer()) {
> +                SVGImageForContainer* svgImageForContainer = downcast<SVGImageForContainer>(image.get());
> +                String resourceId = svgImageForContainer->fragmentIdentifier();
> +                if (resourceId.length())
> +                    svgResourceForMasking = svgImageForContainer->getSVGResource(resourceId);
> +            }  else if (!image.get() && bgLayer->hasMaskImage()) {
> +                const RefPtr<MaskImageOperation>& maskImageOperation = bgLayer->maskImage();
> +                CachedSVGDocumentReference* svgDocumentReference = maskImageOperation->cachedSVGDocumentReference();
> +                Element* elementForMasking = nullptr;
> +                if (svgDocumentReference && svgDocumentReference->document()) {
> +                    SVGDocument* svgDocument = svgDocumentReference->document()->document();
> +                    if (svgDocument && svgDocument->rootElement())
> +                        elementForMasking = svgDocument->rootElement()->getElementById(maskImageOperation->fragment());
> +                } else
> +                    elementForMasking = document().getElementById(maskImageOperation->fragment());
> +
> +                if (elementForMasking)
> +                    svgResourceForMasking = elementForMasking->renderer();
> +            }
> +
> +            CompositeOperator compositeOp = (op == CompositeSourceOver ? bgLayer->composite() : op);
> +            if (svgResourceForMasking) {
> +                if (is<RenderSVGResourceMasker>(svgResourceForMasking)) {
> +                    RenderSVGResourceMasker* masker = downcast<RenderSVGResourceMasker>(svgResourceForMasking);
> +                    masker->applySVGMask(*this, context, false);
> +                    MaskerData* maskerData = masker->maskerDataForRenderer(*this);
> +                    ASSERT(maskerData);
> +                    context->drawImageBuffer(maskerData->maskImage.get(), ColorSpaceDeviceRGB, geometry.destRect(), compositeOp);
> +                    didPaintCustomMask = true;
> +                } else if (is<RenderSVGResourceGradient>(svgResourceForMasking)) {
> +                    RenderSVGResourceGradient* gradient = downcast<RenderSVGResourceGradient>(svgResourceForMasking);
> +                    if (gradient->applyResource(*this, style(), context, ApplyToFillMode)) {
> +                        // Create path from the object's box and fill it with the gradient
> +                        Path path;
> +                        path.addRect(objectBoundingBox());
> +                        gradient->postApplyResource(*this, context, ApplyToFillMode, &path, nullptr);
> +                        didPaintCustomMask = true;
> +                    }
> +                }
> +            }
> +            
> +            if (!didPaintCustomMask && shouldPaintBackgroundImage) {
> +                context->setDrawLuminanceMask(bgLayer->maskSourceType() == MaskLuminance);
> +                bool useLowQualityScaling = shouldPaintAtLowQuality(context, image.get(), bgLayer, geometry.tileSize());
> +                if (image.get())
> +                    image->setSpaceSize(geometry.spaceSize());
> +                context->drawTiledImage(image.get(), style().colorSpace(), geometry.destRect(), geometry.relativePhase(), geometry.tileSize(), ImagePaintingOptions(compositeOp, bgLayer->blendMode(), ImageOrientationDescription(), useLowQualityScaling));

Would be nice to move all this SVG-related code into a separate file.

> Source/WebCore/svg/graphics/SVGImage.cpp:262
> +    if (!m_fragmentIdentifier.isEmpty())
> +        view->scrollToAnchor(m_fragmentIdentifier);

Weird.
Comment 20 Radu Stavila 2014-11-03 09:14:43 PST
Created attachment 240858 [details]
WIP3

WIP3. Difference from previous WIP:
- multiple mask images are now supported
- updating mask-image values from javascript is now supported

Moving on, I'll work on using non-url values for mask-image
e.g. -webkit-mask-image: linear-gradient(black 0%, transparent 100%);
Comment 21 Radu Stavila 2014-11-03 09:17:12 PST
Comment on attachment 240406 [details]
WIP2

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

Thanks a lot for your feedback, I'll be addressing all your comments in the following WIP patch.

>> Source/WebCore/svg/graphics/SVGImage.cpp:262
>> +        view->scrollToAnchor(m_fragmentIdentifier);
> 
> Weird.

This is used for SVG stacks. Why do you find it weird?
Comment 22 Simon Fraser (smfr) 2014-11-03 11:23:30 PST
(In reply to comment #21)
> Comment on attachment 240406 [details]
> WIP2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240406&action=review
> 
> Thanks a lot for your feedback, I'll be addressing all your comments in the
> following WIP patch.
> 
> >> Source/WebCore/svg/graphics/SVGImage.cpp:262
> >> +        view->scrollToAnchor(m_fragmentIdentifier);
> > 
> > Weird.
> 
> This is used for SVG stacks. Why do you find it weird?

It just seems odd to use scrolling as the mechanism by which you render a subset of the SVG, rather than having some notion of the source rect.
Comment 23 Radu Stavila 2014-11-04 02:29:38 PST
(In reply to comment #22)
> (In reply to comment #21)
> > 
> > This is used for SVG stacks. Why do you find it weird?
> 
> It just seems odd to use scrolling as the mechanism by which you render a
> subset of the SVG, rather than having some notion of the source rect.

This was already the way it was done for SVG stacks, it just wasn't working for masks. See the FrameView::scrollToFragment method.
Comment 24 Dirk Schulze 2014-11-04 04:05:58 PST
(In reply to comment #22)
> (In reply to comment #21)
> > Comment on attachment 240406 [details]
> > WIP2
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=240406&action=review
> > 
> > Thanks a lot for your feedback, I'll be addressing all your comments in the
> > following WIP patch.
> > 
> > >> Source/WebCore/svg/graphics/SVGImage.cpp:262
> > >> +        view->scrollToAnchor(m_fragmentIdentifier);
> > > 
> > > Weird.
> > 
> > This is used for SVG stacks. Why do you find it weird?
> 
> It just seems odd to use scrolling as the mechanism by which you render a
> subset of the SVG, rather than having some notion of the source rect.

Could it be renamed to navigateToAnchor?
Comment 25 Radu Stavila 2014-11-04 04:07:23 PST
(In reply to comment #24)
> (In reply to comment #22)
> > (In reply to comment #21)
> > > Comment on attachment 240406 [details]
> > > WIP2
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=240406&action=review
> > > 
> > > Thanks a lot for your feedback, I'll be addressing all your comments in the
> > > following WIP patch.
> > > 
> > > >> Source/WebCore/svg/graphics/SVGImage.cpp:262
> > > >> +        view->scrollToAnchor(m_fragmentIdentifier);
> > > > 
> > > > Weird.
> > > 
> > > This is used for SVG stacks. Why do you find it weird?
> > 
> > It just seems odd to use scrolling as the mechanism by which you render a
> > subset of the SVG, rather than having some notion of the source rect.
> 
> Could it be renamed to navigateToAnchor?

Sure.
Comment 26 Radu Stavila 2014-11-06 09:07:14 PST
Created attachment 241112 [details]
WIP4

Differences from WIP3:

- fixed multiple-image masks repainting
- paint multiple-masks only after all masks have been loaded
- all tests in css3/masking (existing & new) are passing
- painting tiled masks using SVG image is now supported
- gradient masks removed, as requested by Dirk Schulze
- addresses some reviewer comments (some still remain but we're added to the final cleanup task on my todo list)

Moving on, I'll work on using non-url values for mask-image
e.g. -webkit-mask-image: linear-gradient(black 0%, transparent 100%);
Comment 27 WebKit Commit Bot 2014-11-06 09:10:45 PST
Attachment 241112 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.h:1263:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/style/RenderStyle.cpp:833:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/style/RenderStyle.cpp:842:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/style/FillLayer.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/graphics/MaskImageOperation.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 5 in 72 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Build Bot 2014-11-06 10:32:34 PST
Created attachment 241115 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 29 Radu Stavila 2014-11-20 05:51:20 PST
Created attachment 241942 [details]
WIP5 - Final WIP before final patch

Major changes:

- all mask types now supported, including generated images
- properly computes masks geometry, in order to not deform images
- moved as much as possible of the masks painting code from RenderBoxModelObject to mask-specific files
- implemented using animated masks
- properly handles pending images
- implemented keyframe animations for mask-image property
- properly handle setting multiple masks to one element
- updated setters/getters of mask-image property from JS
- implemented setting/reading the shorthand property -webkit-mask
- all test failures from previous WIP are now fixed
Comment 30 WebKit Commit Bot 2014-11-20 05:53:09 PST
Attachment 241942 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderLayer.h:1271:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
ERROR: Source/WebCore/rendering/style/FillLayer.cpp:78:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 84 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 31 Build Bot 2014-11-20 07:19:43 PST
Comment on attachment 241942 [details]
WIP5 - Final WIP before final patch

Attachment 241942 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6480831937773568

New failing tests:
compositing/masks/solid-color-masked.html
fast/css/uri-token-parsing.html
fast/filter-image/parse-filter-image.html
Comment 32 Build Bot 2014-11-20 07:19:52 PST
Created attachment 241945 [details]
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-09  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 33 Radu Stavila 2014-11-20 07:41:57 PST
Created attachment 241946 [details]
Added new files to Win/GTK/EFL and fixed failing tests
Comment 34 WebKit Commit Bot 2014-11-20 07:44:31 PST
Attachment 241946 [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]
ERROR: Source/WebCore/rendering/RenderLayer.h:1271:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 2 in 86 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Build Bot 2014-11-20 09:10:27 PST
Comment on attachment 241946 [details]
Added new files to Win/GTK/EFL and fixed failing tests

Attachment 241946 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5896019325747200

New failing tests:
compositing/masks/solid-color-masked.html
Comment 36 Build Bot 2014-11-20 09:10:34 PST
Created attachment 241950 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 37 Build Bot 2014-11-20 09:14:49 PST
Comment on attachment 241946 [details]
Added new files to Win/GTK/EFL and fixed failing tests

Attachment 241946 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6621511242743808

New failing tests:
compositing/masks/solid-color-masked.html
Comment 38 Build Bot 2014-11-20 09:14:56 PST
Created attachment 241951 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 39 Radu Stavila 2014-11-25 12:24:44 PST
Created attachment 242208 [details]
Final WIP
Comment 40 WebKit Commit Bot 2014-11-25 12:27:34 PST
Attachment 242208 [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]
ERROR: Source/WebCore/rendering/RenderLayer.h:1271:  Please declare enum bitfields as unsigned integral types.  [runtime/enum_bitfields] [5]
Total errors found: 2 in 88 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 41 Radu Stavila 2014-11-26 08:38:42 PST
Created attachment 242230 [details]
Fixed windows build, cleaned up code and performed some refactoring
Comment 42 WebKit Commit Bot 2014-11-26 08:42:11 PST
Attachment 242230 [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 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 43 Dirk Schulze 2014-11-26 09:28:51 PST
Comment on attachment 242230 [details]
Fixed windows build, cleaned up code and performed some refactoring

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

This is an initial review and I did not review it into too much detail. Since you split it into 2 or more patches, there will be more feedback on the individual patches. Especially the new files need a lot more checking.

> Source/WebCore/css/CSSParser.cpp:3369
> +                        // When parsing shorthand, parseMaskImage returns a list with a single CSS value.

I do not understand this comment. Could you elaborate more please?

> Source/WebCore/css/CSSParser.cpp:9464
> +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();

This is called just for mask-image, right? What is the space separated list for?

> Source/WebCore/css/CSSParser.cpp:9468
> +        if (value->id == CSSValueNone || (value->unit == CSSPrimitiveValue::CSS_STRING && equalIgnoringCase(value->string, "none")))

CSSValueNone should cover the "none" case already. What is this string checking for? If that should be necessary then this seems to be a bug.

> Source/WebCore/css/CSSParser.cpp:9474
> +            if (parseFillImage(valueList, fillImageValue))

parseFillImage checks for URL and none as well. Ok, URL has special handling here and I hope we can generalize that in the future. It will be necessary for any <image> value in the future. But at least remove the "none" checking.

> Source/WebCore/css/CSSParser.cpp:9478
> +        if (maskImageValue.get()) {

As described before, I am not sure why you need to differ between the long hand and the shorthand. parseFillImage and other functions don't need to do it.

> Source/WebCore/css/CSSParser.h:266
> +    bool parseMaskImage(CSSParserValueList*, RefPtr<CSSValue>&);

Could you place that above parseFillImage definition and add a comment that we want to combine them in the future please?

> Source/WebCore/css/WebKitCSSMaskImageValue.cpp:50
> +    if (m_innerValue.get()) {

Isn't that missing all things that are supported by parseFillImage? linear-gradient, radial-gradient and so on?

> Source/WebCore/css/WebKitCSSMaskImageValue.h:36
> +class WebKitCSSMaskImageValue : public CSSValue {

This whole class is very generic. That is what I asked for before. Maybe it is time to rename it so that other features can rely on this as well? What about WebKitCSSImageOrSVGResourceValue? (Shorter but descriptive name preferred.) Or, if we can use it for filters as well later, maybe just WebKitCSSResourceValue.

> Source/WebCore/loader/cache/CachedResourceLoader.h:117
> +    void addCachedResource(CachedResource*);

What is this function good for? Are filters and clip-path missing this change and that makes them fail?

> Source/WebCore/loader/cache/CachedSVGDocument.cpp:63
> +        if (m_shouldCreateFrameForDocument) {

Could you add a comment before this "if" explaining what this aims to do please?

> Source/WebCore/page/Page.cpp:300
> +    Page* newPage = new Page(pageConfiguration);
> +    newPage->settings().setMediaEnabled(false);
> +    newPage->settings().setScriptEnabled(false);
> +    newPage->settings().setPluginsEnabled(false);
> +    
> +    Frame& frame = newPage->mainFrame();
> +    frame.setView(FrameView::create(frame));
> +    frame.init();
> +    FrameLoader& loader = frame.loader();
> +    loader.forceSandboxFlags(SandboxAll);
> +    
> +    frame.view()->setCanHaveScrollbars(canHaveScrollbars);
> +    frame.view()->setTransparent(transparent);
> +    
> +    ASSERT(loader.activeDocumentLoader()); // DocumentLoader should have been created by frame->init().
> +    loader.activeDocumentLoader()->writer().setMIMEType(mimeType);
> +    loader.activeDocumentLoader()->writer().begin(URL()); // create the empty document
> +    loader.activeDocumentLoader()->writer().addData(buffer->data(), buffer->size());
> +    loader.activeDocumentLoader()->writer().end();

Some one else should review this as well since this is relevant for security. I do not know what "setMediaEnabled" does. Otherwise looks good to me.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:354
> +        RefPtr<WebKitCSSMaskImageValue> cssMaskImageValue = WebKitCSSMaskImageValue::create(CSSPrimitiveValue::create("", CSSPrimitiveValue::CSS_URI));

Why do you create a MaskImageValue with an URI? This excludes all animations for generated images like gradients, right? (We do not support animation on gradients yet but work is under way.)

> Source/WebCore/platform/ScrollView.cpp:1078
> +        // When using a mask from an SVG as an element's maskImage, the visibleContentRect

Could you give an example? I am not entirely sure what that means.

> Source/WebCore/platform/graphics/MaskImageOperation.cpp:28
> +#include "MaskImageOperation.h"

This file and the header need a more careful review than I am able to right now :P I'll check it later. Is the code familiar to other code paths?

> Source/WebCore/rendering/RenderBox.cpp:705
> +    return borderBoxRect();

could this one-liners be in the header?

> Source/WebCore/rendering/RenderLayerMaskImageInfo.h:41
> +class RenderLayer::MaskImageInfo final {

I wonder if we need to generalize this more later so that we can use it in other places. Or is it to specific to masks? I assume we need this for SVG gradient references as well?

> Source/WebCore/svg/graphics/SVGImage.cpp:-355
> -        m_page->settings().setMediaEnabled(false);

Ahh, you got the security part from here.... would still be interesting what Media does.
Comment 44 Build Bot 2014-11-26 10:07:19 PST
Comment on attachment 242230 [details]
Fixed windows build, cleaned up code and performed some refactoring

Attachment 242230 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5357806437793792

New failing tests:
animations/cross-fade-webkit-mask-image.html
Comment 45 Build Bot 2014-11-26 10:07:36 PST
Created attachment 242232 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 46 Radu Stavila 2014-11-26 10:21:12 PST
Comment on attachment 242230 [details]
Fixed windows build, cleaned up code and performed some refactoring

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

>> Source/WebCore/css/CSSParser.cpp:3369
>> +                        // When parsing shorthand, parseMaskImage returns a list with a single CSS value.
> 
> I do not understand this comment. Could you elaborate more please?

When returning from parseMaskImage, val1 will be of type CSSValueList, with each element in the list being a WebKitCSSMaskImageValue. In the case of shorthand parsing, parseMaskImage stops after the first value because the following values will not be mask-image, but other mask properties.

>> Source/WebCore/css/CSSParser.cpp:9464
>> +    RefPtr<CSSValueList> list = CSSValueList::createSpaceSeparated();
> 
> This is called just for mask-image, right? What is the space separated list for?

As discussed on irc, I'll change it to read one value at a time.

>> Source/WebCore/css/CSSParser.cpp:9468
>> +        if (value->id == CSSValueNone || (value->unit == CSSPrimitiveValue::CSS_STRING && equalIgnoringCase(value->string, "none")))
> 
> CSSValueNone should cover the "none" case already. What is this string checking for? If that should be necessary then this seems to be a bug.

I saw it done the same in parseAnimationName. I'll remove the text check and see if anything breaks.

>> Source/WebCore/css/CSSParser.cpp:9474
>> +            if (parseFillImage(valueList, fillImageValue))
> 
> parseFillImage checks for URL and none as well. Ok, URL has special handling here and I hope we can generalize that in the future. It will be necessary for any <image> value in the future. But at least remove the "none" checking.

Basically, parseFillImage will only used for generated images. Otherwise, I want it to go on the new path.

>> Source/WebCore/css/CSSParser.cpp:9478
>> +        if (maskImageValue.get()) {
> 
> As described before, I am not sure why you need to differ between the long hand and the shorthand. parseFillImage and other functions don't need to do it.

I was needed to only parse one value for the shorthand scenario. It will no longer be necessary once parseMaskImage is updated.

>> Source/WebCore/css/CSSParser.h:266
>> +    bool parseMaskImage(CSSParserValueList*, RefPtr<CSSValue>&);
> 
> Could you place that above parseFillImage definition and add a comment that we want to combine them in the future please?

Sure.

>> Source/WebCore/css/WebKitCSSMaskImageValue.cpp:50
>> +    if (m_innerValue.get()) {
> 
> Isn't that missing all things that are supported by parseFillImage? linear-gradient, radial-gradient and so on?

No. For that case, isURI will return false and it will "return m_innerValue->cssText()"

>> Source/WebCore/css/WebKitCSSMaskImageValue.h:36
>> +class WebKitCSSMaskImageValue : public CSSValue {
> 
> This whole class is very generic. That is what I asked for before. Maybe it is time to rename it so that other features can rely on this as well? What about WebKitCSSImageOrSVGResourceValue? (Shorter but descriptive name preferred.) Or, if we can use it for filters as well later, maybe just WebKitCSSResourceValue.

I'll think about it.

>> Source/WebCore/loader/cache/CachedResourceLoader.h:117
>> +    void addCachedResource(CachedResource*);
> 
> What is this function good for? Are filters and clip-path missing this change and that makes them fail?

Normally, CachedResource objects are created by the CachedResourceLoader. However, when MaskImageOperation decides that the resource it downloaded is not a valid SVG (or the specified fragment identifier is not valid), it switches back to using a CachedImage. However, because it was not created by the CachedResourceLoader, the only one keeping a handle to it is the MaskImageOperation object (via RenderStyle->...->FillLayer) and it ends up being destroyed at the wrong time in some scenarios. Adding it to the resource loader ensures that it is destroyed at the correct time, along with all the other cached resources.

>> Source/WebCore/loader/cache/CachedSVGDocument.cpp:63
>> +        if (m_shouldCreateFrameForDocument) {
> 
> Could you add a comment before this "if" explaining what this aims to do please?

Will do.

>> Source/WebCore/page/Page.cpp:300
>> +    loader.activeDocumentLoader()->writer().end();
> 
> Some one else should review this as well since this is relevant for security. I do not know what "setMediaEnabled" does. Otherwise looks good to me.

It was copied from SVGImage and moved to a new method so I can also use it in CachedSVGDocument without duplicating so much code.

>> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:354
>> +        RefPtr<WebKitCSSMaskImageValue> cssMaskImageValue = WebKitCSSMaskImageValue::create(CSSPrimitiveValue::create("", CSSPrimitiveValue::CSS_URI));
> 
> Why do you create a MaskImageValue with an URI? This excludes all animations for generated images like gradients, right? (We do not support animation on gradients yet but work is under way.)

It's actually not used during the animation phase, I just added it because it's mandatory for MaskImageOperation's constructor. Would you prefer it to be CSSValueNone?

>> Source/WebCore/platform/ScrollView.cpp:1078
>> +        // When using a mask from an SVG as an element's maskImage, the visibleContentRect
> 
> Could you give an example? I am not entirely sure what that means.

In some situations when using a <mask> element from an external SVG, visibleContentRect will be empty.

>> Source/WebCore/platform/graphics/MaskImageOperation.cpp:28
>> +#include "MaskImageOperation.h"
> 
> This file and the header need a more careful review than I am able to right now :P I'll check it later. Is the code familiar to other code paths?

This is new code.

>> Source/WebCore/rendering/RenderBox.cpp:705
>> +    return borderBoxRect();
> 
> could this one-liners be in the header?

Sure.

>> Source/WebCore/rendering/RenderLayerMaskImageInfo.h:41
>> +class RenderLayer::MaskImageInfo final {
> 
> I wonder if we need to generalize this more later so that we can use it in other places. Or is it to specific to masks? I assume we need this for SVG gradient references as well?

I thought about it and I think that for the time being, it's better to remain separate. I think it might be possible at some point to unite it with RenderLayer::FilterInfo but it's not trivial.
Comment 47 Radu Stavila 2014-11-27 10:33:29 PST
Created attachment 242255 [details]
Addressed some of the comments in the previous review
Comment 48 WebKit Commit Bot 2014-11-27 10:36:21 PST
Attachment 242255 [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 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 49 Radu Stavila 2014-12-03 09:23:19 PST
Created attachment 242496 [details]
Latest patch, EWS testing
Comment 50 WebKit Commit Bot 2014-12-03 09:26:15 PST
Attachment 242496 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParser.cpp:3369:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 1 in 87 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 51 Build Bot 2014-12-03 11:04:06 PST
Comment on attachment 242496 [details]
Latest patch, EWS testing

Attachment 242496 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6275309699596288

New failing tests:
css3/masking/mask-repeat-space-padding.html
fast/masking/parsing-mask-source-type.html
fast/masking/parsing-mask.html
fast/backgrounds/multiple-backgrounds-computed-style.html
css3/masking/mask-base64.html
css3/masking/mask-multiple-values.html
Comment 52 Build Bot 2014-12-03 11:04:12 PST
Created attachment 242504 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 53 Build Bot 2014-12-03 11:35:50 PST
Comment on attachment 242496 [details]
Latest patch, EWS testing

Attachment 242496 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5665564466872320

New failing tests:
css3/masking/mask-repeat-space-padding.html
css3/masking/mask-repeat-space-border.html
fast/masking/parsing-mask-source-type.html
fast/masking/parsing-mask.html
css3/masking/mask-multiple-values.html
css3/masking/mask-base64.html
fast/backgrounds/multiple-backgrounds-computed-style.html
Comment 54 Build Bot 2014-12-03 11:35:56 PST
Created attachment 242508 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 55 Build Bot 2014-12-03 13:28:55 PST
Comment on attachment 242496 [details]
Latest patch, EWS testing

Attachment 242496 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5902387352961024

New failing tests:
css3/masking/mask-repeat-space-padding.html
css3/masking/mask-repeat-space-border.html
fast/masking/parsing-mask-source-type.html
fast/masking/parsing-mask.html
fast/backgrounds/multiple-backgrounds-computed-style.html
css3/masking/mask-base64.html
css3/masking/mask-multiple-values.html
Comment 56 Build Bot 2014-12-03 13:29:01 PST
Created attachment 242518 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 57 Radu Stavila 2014-12-04 04:14:02 PST
Created attachment 242564 [details]
EWS testing
Comment 58 Build Bot 2014-12-04 05:32:11 PST
Comment on attachment 242564 [details]
EWS testing

Attachment 242564 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5184591816032256

New failing tests:
animations/cross-fade-webkit-mask-image.html
fast/backgrounds/multiple-backgrounds-computed-style.html
Comment 59 Build Bot 2014-12-04 05:32:17 PST
Created attachment 242566 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 60 Build Bot 2014-12-04 06:28:51 PST
Comment on attachment 242564 [details]
EWS testing

Attachment 242564 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5375539284017152

New failing tests:
fast/backgrounds/multiple-backgrounds-computed-style.html
Comment 61 Build Bot 2014-12-04 06:28:57 PST
Created attachment 242568 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 62 Build Bot 2014-12-04 08:25:58 PST
Comment on attachment 242564 [details]
EWS testing

Attachment 242564 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5625044671660032

New failing tests:
fast/backgrounds/multiple-backgrounds-computed-style.html
Comment 63 Build Bot 2014-12-04 08:26:07 PST
Created attachment 242570 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 64 Radu Stavila 2014-12-04 09:47:15 PST
Created attachment 242572 [details]
EWS testing
Comment 65 Simon Fraser (smfr) 2014-12-04 13:52:01 PST
Comment on attachment 242572 [details]
EWS testing

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

> LayoutTests/compositing/masks/become-tiled-mask.html:44
> +                }, 100);

Why 100ms? that's too long for a test.

> LayoutTests/compositing/masks/cease-tiled-mask.html:44
> +                }, 100);

Ditto.

> Source/WebCore/css/CSSParser.cpp:9449
> +    return (outValue.get());

No need for the extra parens.

> Source/WebCore/platform/ScrollView.cpp:1082
> +        // When using a mask from an SVG as an element's maskImage, the visibleContentRect
> +        // of the SVG document will be empty, but the element using it as a mask still
> +        // needs to be repainted.
> +        if (!containsSVGDocument() || !visibleRect.isEmpty())
> +            paintRect.intersect(visibleRect);

It's a bit weird to do this here, and ScrollView really shouldn't know anything about SVG documents. Why not just set paintsEntireContents() on the mask document? I think this needs to be cleaned up before the patch lands.

> Source/WebCore/rendering/RenderBoxModelObject.cpp:858
> +            image = bgImage->image(backgroundObject ? backgroundObject : this, geometry.tileSize());
> +        
> +        if (!image.get() && bgLayer->hasMaskImage()) {
> +            const RefPtr<MaskImageOperation>& maskImageOperation = bgLayer->maskImage();
> +            if (StyleImage* styleImage = maskImageOperation->image())
> +                image = styleImage->image(backgroundObject ? backgroundObject : this, geometry.tileSize());
> +

I think this could be simplified by looking at the painting phase and only consulting the mask image during the mask phase. Even better would be to pull the mask-painting into a separate function.

> Source/WebCore/rendering/RenderElement.cpp:335
> +        if (StyleImage* image = currNew->image())
> +            image->addClient(this);
> +        else if (currNew->maskImage().get())
> +            currNew->maskImage()->addRendererImageClient(this);

Can't a FillLayer have both an image and a mask image?
Comment 66 Radu Stavila 2014-12-04 14:07:14 PST
Comment on attachment 242572 [details]
EWS testing

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

>> Source/WebCore/rendering/RenderBoxModelObject.cpp:858
>> +
> 
> I think this could be simplified by looking at the painting phase and only consulting the mask image during the mask phase. Even better would be to pull the mask-painting into a separate function.

I guess that could work, but I believe it shouldn't be totally separated because much of the code is shared. Basically, when the mask can't find a valid fragment id in an SVG, it defaults to using a normal image in which case, the painting is identical to the non-masking phase. The custom painting of <mask> elements from SVGs is already separated in the MaskImageOperation::drawMask method. I will look into how the code in paintFillLayerExtended could be simplified further.

>> Source/WebCore/rendering/RenderElement.cpp:335
>> +            currNew->maskImage()->addRendererImageClient(this);
> 
> Can't a FillLayer have both an image and a mask image?

No, it cannot. As we discussed on IRC, I'll add a FIXME for this in FillLayer.h.
Comment 67 Radu Stavila 2014-12-05 03:49:33 PST
Created attachment 242625 [details]
EWS testing after some refactoring
Comment 68 Radu Stavila 2014-12-05 09:24:04 PST
Created attachment 242634 [details]
EWS testing for a simplified version
Comment 69 Build Bot 2014-12-05 10:48:47 PST
Comment on attachment 242634 [details]
EWS testing for a simplified version

Attachment 242634 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5377890241740800

Number of test failures exceeded the failure limit.
Comment 70 Build Bot 2014-12-05 10:48:55 PST
Created attachment 242644 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5
Comment 71 Radu Stavila 2014-12-05 12:23:02 PST
Created attachment 242647 [details]
EWS testing
Comment 72 Build Bot 2014-12-05 17:04:18 PST
Comment on attachment 242647 [details]
EWS testing

Attachment 242647 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6069438495326208

New failing tests:
animations/cross-fade-webkit-mask-image.html
Comment 73 Build Bot 2014-12-05 17:04:26 PST
Created attachment 242684 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5
Comment 74 Radu Stavila 2014-12-08 09:30:32 PST
Created attachment 242821 [details]
EWS testing
Comment 75 Radu Stavila 2014-12-08 11:50:27 PST
Created attachment 242837 [details]
EWS testing
Comment 76 Radu Stavila 2014-12-09 08:15:14 PST
Created https://bugs.webkit.org/show_bug.cgi?id=139440 and https://bugs.webkit.org/show_bug.cgi?id=139442 for further improvements.
Comment 77 Radu Stavila 2014-12-11 04:07:09 PST
Created attachment 243113 [details]
EWS testing