Description
Dirk Schulze
2014-03-04 06:52:07 PST
Created attachment 234254 [details]
shape.svg
Created attachment 234255 [details]
sample.html
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. 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. 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.
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 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
Created attachment 241946 [details]
Added new files to Win/GTK/EFL and fixed failing tests
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 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 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 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 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
Created attachment 242208 [details]
Final WIP
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.
Created attachment 242230 [details]
Fixed windows build, cleaned up code and performed some refactoring
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. 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 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. Created attachment 242255 [details]
Addressed some of the comments in the previous review
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.
Created attachment 242496 [details]
Latest patch, EWS testing
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 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 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 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 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 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 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 242564 [details]
EWS testing
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 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 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 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 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 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
Created attachment 242572 [details]
EWS testing
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 242625 [details]
EWS testing after some refactoring
Created attachment 242634 [details]
EWS testing for a simplified version
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. 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 242647 [details]
EWS testing
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 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
Created attachment 242821 [details]
EWS testing
Created attachment 242837 [details]
EWS testing
Created https://bugs.webkit.org/show_bug.cgi?id=139440 and https://bugs.webkit.org/show_bug.cgi?id=139442 for further improvements. Created attachment 243113 [details]
EWS testing
|