Bug 75779

Created attachment 121562 [details] Patch

Comment on attachment 121562 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121562&action=review > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:45 > + if i understood correctly, if (!newTexture) will be true just in the first loop iteration, so we could move this if out of for loop. Doing something like: RefPtr<BitmapTexture> newTexture = m_texturePool[0]; int index = 0; for (size_t i = 1 ....) what do you think?

(In reply to comment #2) > (From update of attachment 121562 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121562&action=review > > > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:45 > > + > > if i understood correctly, if (!newTexture) will be true just in the first loop iteration, so we could move this if out of for loop. > Doing something like: > > RefPtr<BitmapTexture> newTexture = m_texturePool[0]; > int index = 0; > for (size_t i = 1 ....) > > what do you think? Yes, sounds less yucky. Will revise :)

Created attachment 121564 [details] Patch

Created attachment 121565 [details] Patch

Comment on attachment 121565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121565&action=review > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:461 > bool TextureMapperNode::paintReflection(const TextureMapperPaintOptions& options > > // The mask has to be adjusted to target coordinates. > if (maskTexture) { > - maskSurface = options.surfaceManager->getIntermediateSurface(); > + maskSurface = options.textureMapper->acquireTextureFromPool(options.textureMapper->viewportSize()); btw, why doesn't the textureMapperNode have a ref to the textureMapper? instead of passing it thru the options? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:28 > + > +PassRefPtr<BitmapTexture> TextureMapper::acquireTextureFromPool(const IntSize& size) > +{ Why make this part of the TextureMapper? And not a singleton TexturePool? Is it not singleton in nature? > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:51 > + // We choose the smallest texture that is equal or larger than the required size. > + if ((textureSize.width() < size.width() || textureSize.height() < size.height()) > + || (newTextureSize.width() >= size.width() > + && newTextureSize.height() >= size.height() > + && newTextureSize.width() * newTextureSize.height() <= textureSize.width() * textureSize.height())) { > + continue; > + } Couldnt this be styled a bit nicer?

Comment on attachment 121565 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121565&action=review Okay! Please fix the style nit before landing. > Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:136 > + : m_parent(0), m_effectTarget(0), m_opacity(1.0), m_textureMapper(0) { } This should probably be: TextureMapperNode() : m_parent(0) , m_effectTarget(0) , m_opacity(1), , m_textureMapper(0) { } Note the style guide suggests remove the .0 from floats when it isn't important for arithmetic.

(In reply to comment #6) > (From update of attachment 121565 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121565&action=review Whoops, sorry Noam. Kenneth left these comments while I was doing my review. I'm also interested in the answers to his questions, so maybe you might wait untli a nod from him to land this?

(In reply to comment #6) > btw, why doesn't the textureMapperNode have a ref to the textureMapper? instead of passing it thru the options? Because TextureMappers might be changed... they're tied to the painting context. You shouldn't really use TexftureMapper, except for during paint. > > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:28 > > + > > +PassRefPtr<BitmapTexture> TextureMapper::acquireTextureFromPool(const IntSize& size) > > +{ > > Why make this part of the TextureMapper? And not a singleton TexturePool? Is it not singleton in nature? No, it has to be cleaned up together with other TextureMapper-bound resources. > > > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:51 > > + // We choose the smallest texture that is equal or larger than the required size. > > + if ((textureSize.width() < size.width() || textureSize.height() < size.height()) > > + || (newTextureSize.width() >= size.width() > > + && newTextureSize.height() >= size.height() > > + && newTextureSize.width() * newTextureSize.height() <= textureSize.width() * textureSize.height())) { > > + continue; > > + } > > Couldnt this be styled a bit nicer? Will do.

Created attachment 121599 [details] Patch Addressed some of the comments with the patch; Other comments I addressed in previous reply.

Comment on attachment 121599 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121599&action=review > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:53 > + bool selectedTextureContainsSize = FitsSize?

Created attachment 121636 [details] Patch

Comment on attachment 121636 [details] Patch Rejecting attachment 121636 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: apply', u'--force']" exit_code: 9 Error: the Git diff contains a binary file without the binary data in line: "Binary files /dev/null and b/LayoutTests/platform/qt/css3/filters/add-filter-rendering-expected.png differ". Be sure to use the --binary flag when invoking "git diff" with diffs containing binary files. at /mnt/git/webkit-commit-queue/Tools/Scripts/VCSUtils.pm line 676, <ARGV> line 58. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 9 Full output: http://queues.webkit.org/results/11183429

Created attachment 121637 [details] Patch

Comment on attachment 121637 [details] Patch Clearing flags on attachment: 121637 Committed r104447: <http://trac.webkit.org/changeset/104447>

All reviewed patches have been landed. Closing bug.