Because the patch in https://bugs.webkit.org/show_bug.cgi?id=129682 is quite large, I will break it into two smaller patches. This is the first one and it contains all the new classes created for this patch but does not affect anything in the current functionality. The following patch will link all the pieces together.
Created attachment 242271 [details] Patch
Attachment 242271 [details] did not pass style-queue: ERROR: Source/WebCore/rendering/style/FillLayer.cpp:78: Wrong number of spaces before statement. (expected: 8) [whitespace/indent] [4] Total errors found: 1 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 242271 [details] Patch @Dirk, Simon: Please review.
Comment on attachment 242271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242271&action=review Really great patch! You did not add the copyright in all files that you touched. Inline more comments and questions. I can review the patch as a whole but would like to have a second pair of eyes to look at the image loader and caching part. Simon, could you take a look please? > Source/WebCore/ChangeLog:31 > + It only adds support for referencing <mask> elements for the -webkit-mask-image There is nothing about that that couldn't be tested? > Source/WebCore/ChangeLog:86 > + (WebCore::MaskImageOperation::notifyFinished): This is the method that gets called when the document has finished downloading and checks if it can find a valid <mask> element. Add a line break here. > Source/WebCore/ChangeLog:89 > + * rendering/RenderBoxModelObject.cpp: The BackgroundImageGeometry class was moved out of RenderBoxModelObject in order to be used as a parameter for other methods. This was necessary to avoid having methods with very many parameters. Ditto. This could have been the first patch, just moving out BackgroundImageGeometry. > Source/WebCore/ChangeLog:135 > + * rendering/RenderLayerMaskImageInfo.cpp: Added. For this file, you need to add per line comments what MaskImageInfor is doing in each method. > Source/WebCore/css/StyleResolver.cpp:3629 > + outOperations.clear(); If you create the mask image operation, shouldn't this be empty? Or are you reusing it? If it should be empty, assert instead. > Source/WebCore/css/StyleResolver.cpp:3638 > + CSSPrimitiveValue& primitiveValue = downcast<CSSPrimitiveValue>(*inValue); > + if (primitiveValue.getValueID() == CSSValueNone) > + return true; why not return false if is not 'none'? I wonder if it should be: return (downcast<CSSPrimitiveValue>(*inValue).getValueID() == CSSValueNone); > Source/WebCore/css/StyleResolver.cpp:3642 > + if (!is<CSSValueList>(*inValue)) > + return false; Ok, you would fall out here, still. > Source/WebCore/css/StyleResolver.cpp:3655 > + newMaskImage = MaskImageOperation::create(&maskImageValue, "", "", false, m_state.document().cachedResourceLoader()); Maybe you want to add a send create() function to avoid these passes. I wonder why you still pass a loader even though there is nothing to load. > Source/WebCore/css/StyleResolver.cpp:3672 > + if (newMaskImage.get()) > + outOperations.append(newMaskImage); If one of the above operations fails and we do not end up with a newMaskImage, don't we have a list of less mask operations than the user specified? To know the order of the applied resources is important for mask-composite. > Source/WebCore/loader/cache/CachedSVGDocument.h:40 > + virtual bool canReuse(const ResourceRequest&) const override { return m_canReuseResource; } Why not call it canReuseResource() as well? Sounds better. > Source/WebCore/loader/cache/CachedSVGDocument.h:41 > + void setCanReuse(bool canReuseResource) { m_canReuseResource = canReuseResource; }; Ditto with set* > Source/WebCore/platform/graphics/MaskImageOperation.cpp:48 > +PassRefPtr<MaskImageOperation> MaskImageOperation::create(PassRefPtr<StyleImage> generatedImage) As mentioned above, maybe you can add a create() function for the none case. Also, it seems that they are short enough to put them into the header. > Source/WebCore/platform/graphics/MaskImageOperation.cpp:81 > + return m_cssMaskImageValue->isCSSValueNone(); Shouldn't there always be a valid m_cssMaskImageValue? Should this be an assertion instead? > Source/WebCore/platform/graphics/MaskImageOperation.cpp:104 > + if (image()) > + return (image()->cachedImage() && image()->cachedImage()->image()); What about generated images? For those, cachedImage() would return 0 and therefore false. Pending image resources would return 0 as well, but that is probably correct here. > Source/WebCore/platform/graphics/MaskImageOperation.cpp:114 > +void MaskImageOperation::setRenderLayerImageClient(CachedImageClient* client) I think in these methods it is correct not to look at generated images. > Source/WebCore/platform/graphics/MaskImageOperation.cpp:226 > + // Identify the element referenced by the fragmentId. > + CachedSVGDocumentReference* svgDocumentReference = cachedSVGDocumentReference(); > + Element* elementForMasking = nullptr; > + RenderObject* svgResourceForMasking = nullptr; > + if (svgDocumentReference && svgDocumentReference->document()) { > + SVGDocument* svgDocument = svgDocumentReference->document()->document(); > + if (svgDocument && svgDocument->rootElement()) > + elementForMasking = svgDocument->rootElement()->getElementById(fragment()); > + } else > + elementForMasking = renderer.document().getElementById(fragment()); This function just gets the element, could you put the code into an inline function with a descriptive name?
Comment on attachment 242271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242271&action=review >> Source/WebCore/ChangeLog:31 >> + It only adds support for referencing <mask> elements for the -webkit-mask-image > > There is nothing about that that couldn't be tested? Unfortunately no, all the code added here is "dead", it's not called anywhere so there's nothing to test. I looked at multiple ways of splitting this patch and having some intermediary tests but there are so many existing tests for -webkit-mask-image that it's impossible to change something without breaking existing tests, unless I change everything all the way. >> Source/WebCore/css/StyleResolver.cpp:3638 >> + return true; > > why not return false if is not 'none'? > > I wonder if it should be: return (downcast<CSSPrimitiveValue>(*inValue).getValueID() == CSSValueNone); The code that will call this method (part2 of the patch) will only apply it to the style if this returns true. As such, I want it to return true for none so it would apply it and clear any old masks. >> Source/WebCore/loader/cache/CachedSVGDocument.h:40 >> + virtual bool canReuse(const ResourceRequest&) const override { return m_canReuseResource; } > > Why not call it canReuseResource() as well? Sounds better. It's not a new method, it's an override of an existing method (defined in CachedResource).
Comment on attachment 242271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242271&action=review >>> Source/WebCore/css/StyleResolver.cpp:3638 >>> + return true; >> >> why not return false if is not 'none'? >> >> I wonder if it should be: return (downcast<CSSPrimitiveValue>(*inValue).getValueID() == CSSValueNone); > > The code that will call this method (part2 of the patch) will only apply it to the style if this returns true. As such, I want it to return true for none so it would apply it and clear any old masks. Don't you return false in the next if clause anyway? Because if it is a primitive value and not none, you still fail on the value list check. >> Source/WebCore/css/StyleResolver.cpp:3672 >> + outOperations.append(newMaskImage); > > If one of the above operations fails and we do not end up with a newMaskImage, don't we have a list of less mask operations than the user specified? To know the order of the applied resources is important for mask-composite. I'd still be interested in this question.
(In reply to comment #6) > Comment on attachment 242271 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=242271&action=review > >> Source/WebCore/css/StyleResolver.cpp:3672 > >> + outOperations.append(newMaskImage); > > > > If one of the above operations fails and we do not end up with a newMaskImage, don't we have a list of less mask operations than the user specified? To know the order of the applied resources is important for mask-composite. > > I'd still be interested in this question. I'm addressing that in the upcoming new patch.
Comment on attachment 242271 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=242271&action=review >> Source/WebCore/platform/graphics/MaskImageOperation.cpp:104 >> + return (image()->cachedImage() && image()->cachedImage()->image()); > > What about generated images? For those, cachedImage() would return 0 and therefore false. Pending image resources would return 0 as well, but that is probably correct here. It will not get here for generated image because of the previous if statement. m_isExternalDocument will be false for generated images.
Created attachment 242495 [details] Patch integrating Dirk's feedback
Comment on attachment 242495 [details] Patch integrating Dirk's feedback View in context: https://bugs.webkit.org/attachment.cgi?id=242495&action=review > Source/WebCore/css/StyleResolver.cpp:3477 > + State& state = m_state; Why do you need this variable? > Source/WebCore/css/StyleResolver.cpp:3489 > + RefPtr<MaskImageOperation> newMaskImage = nullptr; No need to initialize to nullptr. > Source/WebCore/css/StyleResolver.cpp:3502 > + state.maskImagesWithPendingSVGDocuments().append(newMaskImage); m_state > Source/WebCore/css/StyleResolver.h:241 > bool createFilterOperations(CSSValue* inValue, FilterOperations& outOperations); > + bool createMaskImageOperations(CSSValue* inValue, Vector<RefPtr<MaskImageOperation>>& outOperations); It's a bit odd that we have FilterOperations, but not MaskImageOperations. > Source/WebCore/loader/cache/CachedSVGDocument.h:53 > + unsigned m_shouldCreateFrameForDocument : 1; > + unsigned m_canReuseResource : 1; I don't think it's worth using bit flags here. > Source/WebCore/page/Page.cpp:279 > +Page* Page::createPageFromBuffer(PageConfiguration& pageConfiguration, const SharedBuffer* buffer, const String& mimeType, bool canHaveScrollbars, bool transparent) This should return a unique_ptr<> > Source/WebCore/platform/ScrollView.h:413 > + virtual bool isSVGDocument() const { return false; } This seems like the wrong level of abstraction. The scrollview contains a document, it is not a document. > Source/WebCore/platform/graphics/MaskImageOperation.h:73 > + CachedSVGDocumentReference* getOrCreateCachedSVGDocumentReference(); Maybe ensureCachedSVGDocumentReference()? > Source/WebCore/platform/graphics/MaskImageOperation.h:75 > + virtual void notifyFinished(CachedResource*); override? > Source/WebCore/rendering/RenderLayerMaskImageInfo.cpp:102 > + CachedSVGDocument* cachedSVGDocument = documentReference ? documentReference->document() : 0; nullptr > Source/WebCore/rendering/style/FillLayer.h:185 > + RefPtr<MaskImageOperation> m_maskImageOperation; I'm a little concerned about increasing the size of FillLayer for something that is very rarely used. > Source/WebCore/rendering/svg/RenderSVGResourceMasker.h:56 > + MaskerData* maskerDataForRenderer(RenderObject& renderer) { return m_masker.get(&renderer); } Could this return a const *?
Comment on attachment 242495 [details] Patch integrating Dirk's feedback View in context: https://bugs.webkit.org/attachment.cgi?id=242495&action=review >> Source/WebCore/rendering/svg/RenderSVGResourceMasker.h:56 >> + MaskerData* maskerDataForRenderer(RenderObject& renderer) { return m_masker.get(&renderer); } > > Could this return a const *? Not really because it's member maskImage is then used as a parameter to GraphicsContext::drawImageBuffer, which doesn't take it as const.
Created attachment 242563 [details] Integrated Simon's feedback
Created attachment 242569 [details] Added 'reviewed by' in ChangeLog
Comment on attachment 242569 [details] Added 'reviewed by' in ChangeLog Clearing flags on attachment: 242569 Committed r176798: <http://trac.webkit.org/changeset/176798>
(In reply to comment #13) > Created attachment 242569 [details] > Added 'reviewed by' in ChangeLog Commit queue will do that for you.
(In reply to comment #15) > (In reply to comment #13) > > Created attachment 242569 [details] > > Added 'reviewed by' in ChangeLog > > Commit queue will do that for you. Ha, good to know. Is this a recent change in the bots? I remember when I forgot to add it some time ago and it didn't like the fact that it found "OOPS" in the CL.
Reverted in bug 146653
<rdar://problem/114683744>