Summary: | Enhance ShadowBlur to tile inset box shadows | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Component: | Platform | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ariya.hidayat, eric, krit, mrobinson, sam, simon.fraser | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | OS X 10.5 | ||||||||||
Bug Depends on: | 51312 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2010-12-23 15:44:33 PST
Created attachment 77424 [details]
Part 1: new method on GraphicsContext.
Comment on attachment 77424 [details]
Part 1: new method on GraphicsContext.
LGTM. I'm not superhappy with the name fillRectWithRoundedHole but I also can't come up with a clearer one. I think this one will do just fine.
Did this land? Not yet. Comment on attachment 77424 [details] Part 1: new method on GraphicsContext. Landed in http://trac.webkit.org/changeset/77106 Created attachment 80619 [details]
Patch
Comment on attachment 80619 [details] Patch http://trac.webkit.org/changeset/77110 Keep bug open for tiling optimization. In drawInsetShadow(), rect can also be very large. We could be smarter about limiting the size of the layer, but we need to take the shadow offsets into account too. Another enhancement (for all shadows) would be to remember the parameters for the last shadow rendered into the scratch buffer, and just re-use the buffer if the current parameters are the same. That would pay off on pages with consecutive shadowed boxes of the same size. Created attachment 81673 [details]
Patch
Comment on attachment 81673 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81673&action=review r=me but with comments. > Source/WebCore/platform/graphics/ShadowBlur.cpp:410 > + leftSlice= twiceRadius + max(radii.topLeft().width(), radii.bottomLeft().width()); missing space after leftSlice > Source/WebCore/platform/graphics/ShadowBlur.cpp:-505 > - 1 | 2 | 3 > - ----------- > - 4 | | 5 > - ----------- > - 6 | 7 | 8 > - > - The corners are directly copied from the template rectangle to the > - real one and the side tiles are 1 pixel width, we use them as > - > - tiles to cover the destination side. The corner tiles are bigger > - than just the side of the rounded corner, we need to increase it > - because the modifications caused by the corner over the blur > - effect. We fill the central part with solid color to complete the > - shadow. I think this comment is useful. I wouldn't remove it. > Source/WebCore/platform/graphics/ShadowBlur.cpp:539 > + boundingRect.move(m_offset.width(), m_offset.height()); boundingRect.move(m_offset); > Source/WebCore/platform/graphics/ShadowBlur.cpp:542 > + destHoleRect.move(m_offset.width(), m_offset.height()); ditto > Source/WebCore/platform/graphics/ShadowBlur.cpp:607 > + const int templateSideLength = 1; You are using this at least twice. Can you add a static const at the beginning of the document? > Source/WebCore/platform/graphics/ShadowBlur.cpp:686 > + { > + IntRect blurRect(IntPoint(), templateSize); > + RefPtr<ByteArray> layerData = m_layerImage->getUnmultipliedImageData(blurRect); > + blurLayerImage(layerData->data(), blurRect.size(), blurRect.width() * 4); > + m_layerImage->putUnmultipliedImageData(layerData.get(), blurRect.size(), blurRect, IntPoint()); > + } The braces seem useless to me. > Source/WebCore/platform/graphics/ShadowBlur.cpp:692 > + shadowContext->fillRect(FloatRect(0, 0, templateSize.width(), templateSize.height())); argh. Why don't we have a ctor FloatRect(IntPoint, IntSize), you're using this multiple times :-) |