[Webkit-unassigned] [Bug 47070] [Texmap] [Qt] Texture mapper initial implementation
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 4 05:42:50 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=47070
--- Comment #4 from Kenneth Rohde Christiansen <kenneth at webkit.org> 2010-10-04 05:42:50 PST ---
(From update of attachment 69603)
View in context: https://bugs.webkit.org/attachment.cgi?id=69603&action=review
> WebCore/ChangeLog:9
> + [Texmap] [Qt] Texture mapper initial implementation
> + https://bugs.webkit.org/show_bug.cgi?id=47070
> +
> + No new tests: initial patch.
> +
We need an explanation here what the texture mapper is for.
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:43
> + void glVertexAttribPointer(GLuint, GLint, GLenum, GLboolean, GLsizei, const GLvoid *);
GLvoid*
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:45
> + void glShaderSource(GLuint, GLsizei, const char **, const GLint *);
the * should be left aligned
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:77
> +namespace WTF {
Why are you defining WTF things outside JavaScriptCore/wtf? That doesnt seem right.
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:107
> + qFatal("[TextureMapper GL] Command failed: Line %d\n\toperation: %s, \n\terror: %x\n", line, command, err);
This is not a Qt file, so no qFatal.
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:143
> + friend class TextureMapperGL;
We normally declare friend classes at the bottom, though I do not know if there is a style guide item for that
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:152
> + virtual void setContentsToImage(Image*);
> +private:
Please add newline before private here
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:234
> +#define TEXMAP_GET_SHADER_VAR_LOCATION(type, prog, var) if (gShaderInfo.get##type##Location(TexmapShaderInfo::prog##Program, TexmapShaderInfo::var##Variable, #var) < 0) qWarning("Couldn't find variable "#var" in program "#prog);
> +#define TEXMAP_BUILD_SHADER(program) gShaderInfo.createShaderProgram(vertexShaderSource##program, fragmentShaderSource##program, TexmapShaderInfo::program##Program);
no qWarning here please
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:236
> +TextureMapperGL::TextureMapperGL(GraphicsContext* gc)
use context and not gc... gc normally refers to garbage collector and we normally use context
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:262
> +" gl_FragColor = vec4(color.rgb * o, color.a * o); \n"
> +" } \n";
There seems to be a less spaces before the \n in the last line. It should be this viewer though
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:274
> + const char* vertexShaderSourceOpacityAndMask =
> + OES2_PERCISION_DEFINITIONS
> +" uniform mat4 InMatrix; \n"
> +" attribute vec4 InTexCoordSource, InTexCoordMask, InVertex; \n"
> +" varying highp vec2 OutTexCoordSource, OutTexCoordMask; \n"
> +" void main(void) \n"
> +" { \n"
> +" OutTexCoordSource = vec2(InTexCoordSource); \n"
> +" OutTexCoordMask = vec2(InTexCoordMask); \n"
> +" gl_Position = InMatrix * InVertex; \n"
> +" } \n";
Here as well.
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:361
> +void TextureMapperGL::drawTexture(const BitmapTexture& texture, const IntRect& targetRect, const TransformationMatrix& mvMatrix, float opacity, const BitmapTexture* maskTexture)
mvMatrix? mv == ?
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:430
> +void BitmapTextureGL::reset(const IntSize& newSize, bool opaque)
How is it a reset if it takes new values?
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:460
> +PlatformGraphicsContext* BitmapTextureGL::beginPaint(const IntRect& dirtyRect)
> +
> +{
newline too much here
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:480
> + struct Entry {
> + GLuint texture;
> + int refCount;
> + };
Anyway to avoid manual refcounting?
> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:618
> + // create the model-view-projection matrix to display on screen
Comments start with capital letter, ends with a dot.
> WebCore/platform/graphics/opengl/TextureMapperGL.h:63
> +// next power of two
Comment style... starts with a ...
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:47
> + virtual bool save(const String &);
It is not obvious what the string represents here. also the & should be left algined
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:192
> + return adoptRef(new TextureMapperQt(gc));
context please
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:196
> +PassRefPtr<BitmapTexture> TextureMapperQt::createTexture() { return adoptRef(new BitmapTextureQt()); }
no no, we dont define methods like that... please put the return... on the next line
> WebCore/platform/graphics/qt/TextureMapperQt.cpp:219
> +PassRefPtr<RGBA32PremultimpliedBuffer> RGBA32PremultimpliedBuffer::create() { return adoptRef(new RGBA32PremultimpliedBufferQt()); }
Here as well. We normally only do this in headers
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:39
> +#if PLATFORM(QT)
> +#include "QtCore/qdebug.h"
> +#endif
we really shouldnt use Qt debugginbg here
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:56
> + void mark(BitmapTexture *texture);
* alignment
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:92
> + // needs active context: texture.destroy
comments style
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:128
> + printf("Yaya!! %d [%d] [%d]", index, m_data.size(), m_totalCost);
Yaya? Probably remove this debug output
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:227
> + void notifyTransformAnimationRunning(bool r);
r? write it out.
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:409
> +static int compareGraphicsLayersZValue(const void* pa, const void* pb)
pa, pb? maybe a and b would be better
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:492
> + for (int i = 0; i < size; ++i)
> + if (TextureMapperNode* child = m_children[i])
> + descendantsWithContent += child->countDescendantsWithContent();
> +
The for loop here needs braces
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:515
> + /* 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.
> + */
> +
we normally use // comments
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:554
> + // needs active context
comments style
> WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:612
> + for (int i = 0; i < size; ++i)
> + if (TextureMapperNode* layer = m_children[i])
> + layer->invalidateTransform();
needs braces!
--
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