Created attachment 188199 [details] Should display a green rect - fails on ToT. The spec is somewhat confusing when it comes to patternUnits == 'objectBoundingBox' vs. elements with an empty object bounding box (see https://bugs.webkit.org/show_bug.cgi?id=109665). But for patternUnits == 'userSpaceOnUse', the whole empty object bounding box logic is not applicable. We have a bug where elements with an empty OBB are not pattern-stroked on the first paint: if (m_attributes.patternUnits() == SVGUnitTypes::SVG_UNIT_TYPE_OBJECTBOUNDINGBOX && objectBoundingBox.isEmpty()) return false; Unfortunately, we are making this decision before updating patternUnits from the element, so we're always seeing SVG_UNIT_TYPE_OBJECTBOUNDINGBOX on the first pass! Later, if a second element gets painted, the pattern attributes get collected and patternUnits is updated to the specified SVG_UNIT_TYPE_USERSPACEONUSE. Then on subsequent repaints the empty bounding box logic no longer kicks in because attributes are up to date.
Created attachment 188202 [details] Patch
Comment on attachment 188202 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188202&action=review We will also need to make this same change in RenderSVGResourceGradient, and possible elsewhere as well. > Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:78 > + FloatRect objectBoundingBox = object->objectBoundingBox(); Is this local variable needed?
(In reply to comment #2) > (From update of attachment 188202 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188202&action=review > > We will also need to make this same change in RenderSVGResourceGradient, and possible elsewhere as well. Gradients are OK and probably all other resource renderers. This was introduced recently, most likely by a change by yours truly :) > > > Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:78 > > + FloatRect objectBoundingBox = object->objectBoundingBox(); > > Is this local variable needed? Nope, we can fold.
Created attachment 188396 [details] Patch Updated per comments.
Comment on attachment 188396 [details] Patch This patch no longer applies cleanly. For that reason, r-.
The attached test case is confusing. It does not display on any browser. The test also includes patternUnits='userSpaceOnUse' while the bug is about patternUnits == 'objectBoundingBox'. It also include mask which is not related to this bug.
Created attachment 274697 [details] test case
The attaches "test case" does not display correctly in WebKit but it does display correctly in FireFox and Chrome.
Created attachment 274705 [details] Patch
I prefer to call collectPatternAttributes() from RenderSVGResourcePattern::applyResource() instead of leaving it in RenderSVGResourcePattern::buildPattern() because I think it is hard to track this side effect of buildPattern(). The function name does not say it is also ensuring the pattern attributes are collected. applyResource() is the only caller to buildPattern(). Also m_attributes is only used by applyResource() and buildPattern().
Comment on attachment 274705 [details] Patch Attachment 274705 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1023244 New failing tests: svg/custom/pattern-units-fill-stroke.svg
Created attachment 274715 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Created attachment 274716 [details] Patch
Comment on attachment 274716 [details] Patch r=me
Comment on attachment 274716 [details] Patch Clearing flags on attachment: 274716 Committed r198574: <http://trac.webkit.org/changeset/198574>
All reviewed patches have been landed. Closing bug.