Bug 99887

Created attachment 169946 [details] Patch I'd like some feedback on the use of setTimeout(..., 0) in the test. In DRT, I want to make sure we render the custom filter with its original style once, and then we change the style. I'm not sure about the best way to do this in DRT.

Comment on attachment 169946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169946&action=review Looks good, Max! > LayoutTests/css3/filters/custom/custom-filter-change-blend-mode.html:28 > + document.getElementById("shaded").style.webkitFilter = "custom(none mix(url('../resources/mix-color.fs') normal source-atop), mix_color 0 1 0 1)"; I would use a class name instead, just to keep the CSS code out of JS. > Source/WebCore/ChangeLog:8 > + Before this patch, WebKit would not rerender a custom filter when its blend mode changed. What about using repaint instead of rerender? > Source/WebCore/platform/graphics/filters/CustomFilterProgram.cpp:94 > + if (m_programType == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE && m_mixSettings != o.m_mixSettings) > + return false; > + > + return m_programType == o.m_programType; > +} nit: It looks like you could combine these two in the same line. return (m_programType == o.m_programType) && (m_programType != PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE || m_mixSettings == o.m_mixSettings); > Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:100 > + if (m_programType == PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE && m_mixSettings != o.m_mixSettings) > + return false; > Ditto. > Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.h:61 > + bool operator!=(const CustomFilterProgramMixSettings& o) const > + { > + return !(*this == o); > + } If you do the previous refactorings, you wouldn't need this one anymore.

Created attachment 169984 [details] Patch

Comment on attachment 169946 [details] Patch Thanks for the review, Alex! Regarding the test, I changed it to trigger a style resolution with "targetElement.clientWidth" instead of using setTimeout. A repaint test would have also worked, but this way we can keep it a ref test. Thanks for the tip! View in context: https://bugs.webkit.org/attachment.cgi?id=169946&action=review >> LayoutTests/css3/filters/custom/custom-filter-change-blend-mode.html:28 >> + document.getElementById("shaded").style.webkitFilter = "custom(none mix(url('../resources/mix-color.fs') normal source-atop), mix_color 0 1 0 1)"; > > I would use a class name instead, just to keep the CSS code out of JS. Done. Looks much better. >> Source/WebCore/ChangeLog:8 >> + Before this patch, WebKit would not rerender a custom filter when its blend mode changed. > > What about using repaint instead of rerender? In the patch I just posted, I've changed this to be more even specific: "Before this patch, WebKit would not recompute an element's style when just its custom filter blend mode changed." >> Source/WebCore/platform/graphics/filters/CustomFilterProgram.cpp:94 >> +} > > nit: It looks like you could combine these two in the same line. > > return (m_programType == o.m_programType) && (m_programType != PROGRAM_TYPE_BLENDS_ELEMENT_TEXTURE || m_mixSettings == o.m_mixSettings); Done. >> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.cpp:100 >> > > Ditto. Done. >> Source/WebCore/platform/graphics/filters/CustomFilterProgramInfo.h:61 >> + } > > If you do the previous refactorings, you wouldn't need this one anymore. Yup, I removed it.

Comment on attachment 169984 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169984&action=review > LayoutTests/css3/filters/custom/custom-filter-change-blend-mode.html:47 > + .multiply { > + -webkit-filter: custom(none mix(url('../resources/mix-color.fs') multiply source-atop), mix_color 0 1 0 1); > + } > + .normal { > + -webkit-filter: custom(none mix(url('../resources/mix-color.fs') normal source-atop), mix_color 0 1 0 1); > + } Your description writes that just changing blend mode does not trigger repaint. In this case you set the whole property with all values. How can you make sure that just the blend mode is responsible for the repaint? Is there a way to set just the blend mode explicitly?

Thanks for looking at this, Dirk. (In reply to comment #5) > (From update of attachment 169984 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169984&action=review > > > LayoutTests/css3/filters/custom/custom-filter-change-blend-mode.html:47 > > + .multiply { > > + -webkit-filter: custom(none mix(url('../resources/mix-color.fs') multiply source-atop), mix_color 0 1 0 1); > > + } > > + .normal { > > + -webkit-filter: custom(none mix(url('../resources/mix-color.fs') normal source-atop), mix_color 0 1 0 1); > > + } > > Your description writes that just changing blend mode does not trigger repaint. In this case you set the whole property with all values. Right, I'm just make sure all the new values are the same as the old values, except the blend mode. > How can you make sure that just the blend mode is responsible for the repaint? CustomFilterOperation's equals operator provides the answer: """ virtual bool operator==(const FilterOperation& o) const { if (!isSameType(o)) return false; const CustomFilterOperation* other = static_cast<const CustomFilterOperation*>(&o); return *m_program.get() == *other->m_program.get() && m_meshRows == other->m_meshRows && m_meshColumns == other->m_meshColumns && m_meshBoxType == other->m_meshBoxType && m_meshType == other->m_meshType && customFilterParametersEqual(m_parameters, other->m_parameters); } """ When we change only the blend mode, every condition matches between the old CustomFilterOperation and the new CustomFilterOperation except the first one, which compares the CustomFilterProgram. With this patch, CustomFilterProgram's equals operator considers the mix settings. > Is there a way to set just the blend mode explicitly? Unfortunately no. We haven't defined a CSSOM for the mix function yet.

Comment on attachment 169984 [details] Patch I see. r=me then. :)

(In reply to comment #7) > (From update of attachment 169984 [details]) > I see. r=me then. :) Thanks! :)

Comment on attachment 169984 [details] Patch Clearing flags on attachment: 169984 Committed r132266: <http://trac.webkit.org/changeset/132266>

All reviewed patches have been landed. Closing bug.