[Webkit-unassigned] [Bug 47070] [Texmap] [Qt] Texture mapper initial implementation
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Oct 7 12:50:42 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=47070
--- Comment #17 from Kenneth Rohde Christiansen <kenneth at webkit.org> 2010-10-07 12:50:42 PST ---
(From update of attachment 70130)
View in context: https://bugs.webkit.org/attachment.cgi?id=70130&action=review
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:21
> +#include "texmap/TextureMapper.h"
so this lives in WebCore/platform/graphics/texmap/ ? This seems like a layering violation then.
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:23
> +#include "QtCore/qdebug.h"
Shouldn't we include these as <QtCore/qdebug.h>
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:28
> +# include "opengl/TextureMapperGL.h"
why the space before include?
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:59
> + virtual const char* type() const { return "Qt"; }
why Qt here. that doesn't seem descriptive
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:62
> + static void initialize(QPainter *painter)
* is placed wrongly
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:64
> + painter->setRenderHints(QPainter::Antialiasing | QPainter::SmoothPixmapTransform, false);
A comment would be good here
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:160
> + const BitmapTextureQt* maskQt = static_cast<const BitmapTextureQt*>(maskTexture);
Why not just 'mask'?
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:201
> + m_image = QImage(rect.size().width(), rect.size().height(), QImage::Format_ARGB32_Premultiplied);
Are you sure we are never overriding an existing m_image here?
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:36
> +#define DEBUG_TEXMAP_FPS 1
Should we commit with debug enabled?
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:43
> + GraphicsContext* gc;
I prefer context to gc
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:62
> + return sz.width() * sz.height() * 4;
add comment about the 4
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:87
> + // We need an active GL context, because we might call glDeleteTextures.
Isn't this the non GL impl?
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:109
> +// LOG("[TextureMapper] Purged %2.4fK of textures(%d) [%2.4fK/%d]\n", float(before - m_totalCost) / 1024,
> +// int(size-m_data.size()), float(m_totalCost) / 1024, m_data.size());
remove before committing
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:112
> +void TextureMapperCache::mark(BitmapTexture *texture)
* placement
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:233
> + TransformData() : dirty(true), localDirty(true), perspectiveDirty(true)
> + {
> + }
Make that TransformData() : dirty(true), localDirty(true), perspectiveDirty(true { }
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:272
> + inline IntRect fullRect() const
entireRect?
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:318
> + bool dirty;
> + bool tiled;
I believe we would normally call these isDirty, isTiled etc
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:382
> + return int(((*nodeA)->m_transforms.centerZ - (*nodeB)->m_transforms.centerZ) * 1000);
1000? comment
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:392
> +{
> +
> + if (m_layerType == ClipLayer || m_layerType == TransparencyLayer || m_state.replicaLayer)
unneeded newline
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:398
> + for (int i = 0; i < size; ++i)
> + if (TextureMapperNode* child = m_children[i])
> + if (child->hasSurfaceDescendants())
> + return true;
You are missing braces here for the 'for' and the first 'if'
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:403
> +void TextureMapperNode::paint(GraphicsContext* gc, const IntSize& size, const IntRect& targetRect, const IntRect& exposedRect, const TransformationMatrix& transform, float opacity)
s/gc/context
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:405
> + // needs active context
comments starts with capital, ends with dot
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:483
> + // calculate layer type. A layer can be one of the following:
> + // RootLayer: the top level. Draws to a framebuffer, and the target texture draws into the viewport.
> + // only one layer is the root layer.
> + // ScissorLayer: draws to the current framebuffer, and applies an extra scissor before drawing its children.
> + // A scissor layer is a layer with children that masks to bounds, is not a transparency layer, and has a rectangular clip.
> + // ClipLayer: creates a new framebuffer, the size of the layer, and then paints it to the enclosing BitmapTexture with the layer's transform/opacity.
> + // A clip layer is a layer that masks to bounds, doesn't preserve 3D, has children, and has a transparency/mask or a non-rectangular transform.
> + // TransparencyLayer: creates a new framebuffer idetical in size to the current framebuffer. Then draws the fb's texture to the current framebuffer with identity transform.
> + // Used for layers with children and transparency/mask that preserve 3D or don't mask to bounds.
> + // DefaultLayer: draws itself and its children directly to the current framebuffer.
> + // any layer that doesn't conform to the other rules is a DefaultLayer.
> +
Can you keep this comment within 100 chars?
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:524
> +#if 0
I dont like adding code if 0'ed
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:686
> + // needs active context
wrong comment style
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:744
> +void TextureMapperNode::paintSelf(TextureMapper* textureMapper, float opacity, BitmapTexture* maskTexture, BitmapTexture* replicaMaskTexture, bool isSurface)
huge method, can't it be split up?
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:763
> + // needs active context
comment style
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:1053
> + if (m_changeMask & MaskLayerChange)
> + if (TextureMapperNode* layer = toTextureMapperNode(m_layer->maskLayer()))
> + layer->m_effectTarget = this;
needs braces!
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:1057
> + if (m_changeMask & ReplicaLayerChange)
> + if (TextureMapperNode* layer = toTextureMapperNode(m_layer->replicaLayer()))
> + layer->m_effectTarget = this;
also here
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list