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.
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
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 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 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.
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%);
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.
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
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
Created attachment 240481[details]
New layout tests
Added patch containing new layout tests for anyone looking to play around with the new funtionalities.
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.
(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.
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?
(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.
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.
(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.
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 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?
(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.
(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.
(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?
(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.
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%);
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.
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
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
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.
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
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.
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
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
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.
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 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.
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 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.
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.
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.
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
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
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
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
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
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 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 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.
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
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
2014-07-02 05:20 PDT, Radu Stavila
2014-07-02 05:22 PDT, Radu Stavila
2014-10-16 08:06 PDT, Radu Stavila
2014-10-24 05:33 PDT, Radu Stavila
2014-10-24 10:30 PDT, Build Bot
2014-10-24 13:01 PDT, Build Bot
2014-10-27 08:04 PDT, Radu Stavila
2014-11-03 09:14 PST, Radu Stavila
2014-11-06 09:07 PST, Radu Stavila
2014-11-06 10:32 PST, Build Bot
2014-11-20 05:51 PST, Radu Stavila
2014-11-20 07:19 PST, Build Bot
2014-11-20 07:41 PST, Radu Stavila
2014-11-20 09:10 PST, Build Bot
2014-11-20 09:14 PST, Build Bot
2014-11-25 12:24 PST, Radu Stavila
2014-11-26 08:38 PST, Radu Stavila
2014-11-26 10:07 PST, Build Bot
2014-11-27 10:33 PST, Radu Stavila
2014-12-03 09:23 PST, Radu Stavila
2014-12-03 11:04 PST, Build Bot
2014-12-03 11:35 PST, Build Bot
2014-12-03 13:29 PST, Build Bot
2014-12-04 04:14 PST, Radu Stavila
2014-12-04 05:32 PST, Build Bot
2014-12-04 06:28 PST, Build Bot
2014-12-04 08:26 PST, Build Bot
2014-12-04 09:47 PST, Radu Stavila
2014-12-05 03:49 PST, Radu Stavila
2014-12-05 09:24 PST, Radu Stavila
2014-12-05 10:48 PST, Build Bot
2014-12-05 12:23 PST, Radu Stavila
2014-12-05 17:04 PST, Build Bot
2014-12-08 09:30 PST, Radu Stavila
2014-12-08 11:50 PST, Radu Stavila
2014-12-11 04:07 PST, Radu Stavila