Bug 47070 – [Texmap] [Qt] Texture mapper initial implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Noam Rosenthal]

Provide an initial implementation of texture-mapper, that works with Qt and OpenGL.

[Noam Rosenthal]

Created attachment 69601 [details]
Patch

[Noam Rosenthal]

Created attachment 69602 [details]
Patch

[Noam Rosenthal]

Created attachment 69603 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 69603 [details]
Patch

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!

[Noam Rosenthal]

Created attachment 69623 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 69623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69623&action=review

> WebCore/ChangeLog:7
> +        The idea is that in time this would replace GraphicsLayerQt, and could serve as an implementation for other platforms that don't have a scenegraph library.

with time

> WebCore/ChangeLog:9
> +        an adequate level of stability, we can enable it by default and perhaps have it replace GraphicsLayerQt.

I don't like the 'perhaps' here... It sounds like we are adding code that we are not sure whether we will use or not, or just abandon.

Maybe write: "we will enable it by default and replace GraphicsLayerQt on platforms where that makes most sense.

Try to keep these within 80-100 chars per line

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:59
> +    void glDeleteFramebuffers(GLsizei n, const GLuint *framebuffers);
> +    void glGenFramebuffers(GLsizei n, GLuint *framebuffers);

* alignment

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:61
> +    void glGetFramebufferAttachmentParameteriv(GLenum target, GLenum attachment, GLenum pname, GLint *params);

* alignment

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:77
> +namespace WTF {

Maybe add a comment why you need this for now.

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:107
> +        printf("[TextureMapper GL] Command failed: Line %d\n\toperation: %s, \n\terror: %x\n", line, command, err);

I think webkit has some ways to do debugging.

> WebCore/platform/graphics/qt/TextureMapperQt.cpp:43
> +    virtual bool save(const String&);

Please add a variable like "save(const String& path)"

> WebCore/platform/graphics/qt/TextureMapperQt.cpp:49
> +    GraphicsContext* m_gc;

context

> WebCore/platform/graphics/qt/TextureMapperQt.cpp:127
> +TextureMapperQt::TextureMapperQt(GraphicsContext* gc)

context please, not gc

> WebCore/platform/graphics/qt/TextureMapperQt.cpp:135
> +void TextureMapperQt::bindSurface(BitmapTexture* aSurface)

why aSurface and not just surface?

[Noam Rosenthal]

Created attachment 69729 [details]
Patch

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> Created an attachment (id=69729) [details]
> Patch

Not up for review?

[Noam Rosenthal]

Found some stability issues I wanted to figure out before resubmitting for review.

[Noam Rosenthal]

Created attachment 69811 [details]
Patch

[WebKit Review Bot]

Attachment 69811 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/graphics/opengl/TextureMapperGL.cpp:307:  Extra space before [  [whitespace/braces] [5]
WebCore/platform/graphics/opengl/TextureMapperGL.cpp:574:  Extra space before [  [whitespace/braces] [5]
WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:44:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Attachment 69811 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4141094

[Kenneth Rohde Christiansen]

Comment on attachment 69811 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69811&action=review

> WebCore/WebCore.pro:214
> +QT += opengl

So even if we are not using this we force opengl?

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:81
> +    if (err)
> +        WTFReportError(__FILE__, line, WTF_PRETTY_FUNCTION, "[TextureMapper GL] Command failed: %s (%x)\n", command, err);

we normally do

if (!err)
   return;

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:172
> +//        LOG(infoLog);

We dont want to commit this

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:184
> +TextureMapperGL::TextureMapperGL(GraphicsContext* context)

can't some of these different classes be in different files?

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:479
> +}
> +bool BitmapTextureGL::isValid() const

missing newline here

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:501
> +
> +

excessive newline here

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:538
> +    const BitmapTextureGL& surface = static_cast<const BitmapTextureGL&>(aSurface);
> +    // Create the model-view-projection matrix to display on screen.

I would add a new line before the comment

> WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:698
> +//        if (m_pendingContent.contentType == HTMLContentType)
> +//            recache(m_pendingContent.regionToUpdate);

dont add outcommented code

[Noam Rosenthal]

(In reply to comment #13)
> (From update of attachment 69811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69811&action=review
> 
> > WebCore/WebCore.pro:214
> > +QT += opengl
> 
> So even if we are not using this we force opengl?
>

This went into the patch by mistake... will fix the rest.

[Noam Rosenthal]

Created attachment 69821 [details]
Patch

[Noam Rosenthal]

Created attachment 70130 [details]
Patch

Removed the GL backend from this patch, to make it easier to review - will add it in a separate patch.

[Kenneth Rohde Christiansen]

Comment on attachment 70130 [details]
Patch

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

[Noam Rosenthal]

Created attachment 70147 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 70147 [details]
Patch

Looks fine as an initial commit. Would be nice to have tronical and kling look it over, but that can be done later.

[Noam Rosenthal]

Created attachment 70159 [details]
Patch - OpenGL implementation

[WebKit Commit Bot]

Comment on attachment 70147 [details]
Patch

Rejecting patch 70147 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
l tests successful.
Files=14, Tests=304,  1 wallclock secs ( 0.73 cusr +  0.17 csys =  0.90 CPU)
Running build-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Projects/CommitQueue/LayoutTests
Testing 21484 test cases.
java/lc3/JSObject/ToObject-001.html -> failed

Exiting early after 1 failures. 17535 tests run.
285.23s total testing time

17534 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
28 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4169127

[Adam Barth]

Comment on attachment 70147 [details]
Patch

Flaky?

[WebKit Commit Bot]

Comment on attachment 70147 [details]
Patch

Rejecting patch 70147 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'build-and-test', '--no-clean', '--no-update', '--test', '--quiet', '--non-interactive']" exit_code: 2
Last 500 characters of output:
uild-dumprendertree
Compiling Java tests
make: Nothing to be done for `default'.
Running tests from /Projects/CommitQueue/LayoutTests
Testing 21485 test cases.
http/tests/xmlhttprequest/cross-origin-no-authorization.html -> failed

Exiting early after 1 failures. 21341 tests run.
517.74s total testing time
WARNING:webkitpy.common.system:Called kill_process with a non-existant pid 39828

21340 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
33 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/4243118

[WebKit Commit Bot]

Comment on attachment 70147 [details]
Patch

Clearing flags on attachment: 70147

Committed r69374: <http://trac.webkit.org/changeset/69374>

[Simon Hausmann]

Nice :) At the risk of stating the obvious: I think the chromium layer implementation would be a prime candidate to port to the texture mapper.

[Noam Rosenthal]

Not sure about Chromium, with their GPU process stuff. Also their current stuff is pretty good. 
The first candidates I'm hoping for are Android and the smaller ports, and of course as a replacement for our very own QGraphicsView-based implementation

[Alexis Menard (darktears)]

Comment on attachment 70159 [details]
Patch - OpenGL implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=70159&action=review

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:60
> +    void glGetFramebufferAttachmentParameteriv(GLenum target, GLenum attachment, GLenum pname, GLint *params);

The * should be left aligned.

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:286
> +

Am I missing something or could you return before the cast?

> WebCore/platform/graphics/opengl/TextureMapperGL.h:50
> +    int m_curProgram;

m_currentProgram?

> WebCore/platform/graphics/opengl/TextureMapperGL.h:64
> +static inline int npot(int num)

Could you rename it?

> WebCore/platform/graphics/opengl/TextureMapperGL.h:75
> +static inline IntSize npot(const IntSize& s)

Rename it to size no?

[Noam Rosenthal]

Created attachment 70210 [details]
OpenGL implementation with Alexis' comments

[Noam Rosenthal]

Created attachment 70212 [details]
Patch 1: OpenGL backend

Re-upload, forgot the m_currentProgram thingy ;)

[Simon Hausmann]

(In reply to comment #26)
> Not sure about Chromium, with their GPU process stuff. Also their current stuff is pretty good. 
> The first candidates I'm hoping for are Android and the smaller ports, and of course as a replacement for our very own QGraphicsView-based implementation

AFAIK Chromium is just using GL and their underlying GL library takes care of redirecting the commands to the real process that can access the GPU. Coding wise I think it would be fine.

[Noam Rosenthal]

Created attachment 70361 [details]
Slice 1: allow the media AC integration to compile (doesn't work yet)

[Noam Rosenthal]

Created attachment 70362 [details]
Patch 2: stub for media layer implementation.

[Noam Rosenthal]

Created attachment 70363 [details]
Patch 3: Add an opt-in #define to WebCore

[Noam Rosenthal]

Created attachment 70364 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

[Noam Rosenthal]

Created attachment 70365 [details]
Patch 5: have QWebFrame render the layer it got from TextureMapper

This is the most delicate patch of the 5, because it affects current QWebFrame rendering code, though it doesn't change anything significant outside the opt-in #define.
I preferred to do it like this so that I don't fork the QWebFrame functions completely, causing future pains when trying to remove the opt-in.

[Kenneth Rohde Christiansen]

Comment on attachment 70362 [details]
Patch 2: stub for media layer implementation.

View in context: https://bugs.webkit.org/attachment.cgi?id=70362&action=review

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:620
> +    {
> +        if (!videoItem)
> +            return;
> +
> +    }

what is the difference?

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:625
> +        m_client = client;
> +
> +    }

excessive newline

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:627
> +    virtual void paint(GraphicsContext* gc)

context, and not gc please

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:631
> +            return;
> +        QStyleOptionGraphicsItem opt;

I prefer newline after return

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.h:105
> +    virtual void acceleratedRenderingStateChanged() {}

needs a space in {}, thus { }

[Kenneth Rohde Christiansen]

Comment on attachment 70364 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

View in context: https://bugs.webkit.org/attachment.cgi?id=70364&action=review

> WebCore/platform/qt/QWebPageClient.h:60
> +    virtual void setRootGraphicsLayer(WebCore::TextureMapperPlatformLayer* layer) {}

space within {}, thus { }

> WebCore/platform/qt/QWebPageClient.h:62
> +    virtual void setRootGraphicsLayer(QGraphicsObject* layer) {}

here as well

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:37
> +    PlatformLayerProxyQt(QWebFrame *frame, TextureMapperContentLayer *layer, QWidget *widget = 0, QGraphicsObject* graphicsObject = 0)

wrong placement of *

Can't this be templated instead of can you create two contructors one for QWidget and one for QGraphicsObject? maybe using an init() method?

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:38
> +        : QObject(widget ? (QObject*)widget : (QObject*)graphicsObject)

Is this needed?

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:108
> +void
> +PageClientQWidget::markForSync(bool scheduleSync)

void on same line as PageClientQWidget

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:238
> +    // This is not needed in texture-mapper, because we get the scroll position from the QPainter.
> +    return;

Can't this be refactored to not have a return by just changing the defines?

[Noam Rosenthal]

Created attachment 70374 [details]
Patch 2: stub for media layer implementation.

[Noam Rosenthal]

Did you mean to r- patch 4 instead of patch 3?

[Noam Rosenthal]

Created attachment 70377 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

[Kenneth Rohde Christiansen]

Comment on attachment 70365 [details]
Patch 5: have QWebFrame render the layer it got from TextureMapper

View in context: https://bugs.webkit.org/attachment.cgi?id=70365&action=review

> WebKit/qt/Api/qwebframe.cpp:306
> +    // TextureMapper might use raw OpenGL some other backend. On raster this doesn't have any effect anyway.

raw OpenGL some other... the english sounds a bit strange.

> WebKit/qt/Api/qwebframe.cpp:308
> +    painter->beginNativePainting();
> +    if (rootGraphicsLayer)

do we really need beginNative* if there is no rootGraphicsLayer?

> WebKit/qt/Api/qwebframe.cpp:310
> +    painter->endNativePainting();

I would like a newline after this, as it is a kind of grouping

> WebKit/qt/Api/qwebframe.cpp:360
> +#if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER)

why doesn't texture mapper imply accel. compositing

> WebKit/qt/Api/qwebframe.cpp:361
> +        if (rootGraphicsLayer ) {

remove space after rootGraphicsLayer

[Kenneth Rohde Christiansen]

Comment on attachment 70377 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

View in context: https://bugs.webkit.org/attachment.cgi?id=70377&action=review

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:40
> +    PlatformLayerProxyQt(QWebFrame* frame, TextureMapperContentLayer* layer, QWidget* widget = 0, QGraphicsObject* graphicsObject = 0)
> +        : QObject(widget ? (QObject*)widget : (QObject*)graphicsObject)
> +        , m_widget(widget)
> +        , m_graphicsItem(graphicsObject)

I dont like this very much... it is not clear if you can have widget and graphics object at the same time and in that case, what the widget would be

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:94
> +
> +    QRect m_dirtyRect;
> +    QWidget* m_widget;
> +    QGraphicsItem* m_graphicsItem;
> +    QWebFrame* m_frame;
> +    TextureMapperContentLayer* m_layer;

Why are these public?

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:428
> -    // The sceneRect is a good approximation of the size of the application, independent of the view.
> +    // The sceneRect is a good approximation of the size of the application, in dependent of the view.

This change doesn't seem right

> WebKit/qt/WebCoreSupport/PageClientQt.h:53
> +    PageClientQWidget(QWidget* v, QWebPage *p)
> +        : view(v)
> +        , page(p)

just write out things

[Kenneth Rohde Christiansen]

Comment on attachment 70374 [details]
Patch 2: stub for media layer implementation.

View in context: https://bugs.webkit.org/attachment.cgi?id=70374&action=review

> WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:630
> +        opt.rect = opt.exposedRect.toRect();

Are you setting the flag that makes this not be the boundingrect?

[Noam Rosenthal]

(In reply to comment #43)
> (From update of attachment 70374 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70374&action=review
> 
> > WebCore/platform/graphics/qt/MediaPlayerPrivateQt.cpp:630
> > +        opt.rect = opt.exposedRect.toRect();
> 
> Are you setting the flag that makes this not be the boundingrect?

I'm not sure yet, this is just a stub that makes things compile with texture-mapper + video... let's review it again when it actually works

[Noam Rosenthal]

Created attachment 70401 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

[Noam Rosenthal]

> why doesn't texture mapper imply accel. compositing
I'd rather keep both flags for now - one of them is permanent and one temporary. We'll remove the TEXTURE_MAPPER flag later, and keep the ACCELERATED_COMPOSITING one.

[Noam Rosenthal]

Created attachment 70402 [details]
Patch 5: have QWebFrame render the layer it got from TextureMapper

[Noam Rosenthal]

Comment on attachment 70363 [details]
Patch 3: Add an opt-in #define to WebCore

I think this was r-'ed by mistake - otherwise I see no comments about what's wrong :)

[Kenneth Rohde Christiansen]

Comment on attachment 70402 [details]
Patch 5: have QWebFrame render the layer it got from TextureMapper

Looks fine, just make sure this is very well tested, for the case not using AC

[Noam Rosenthal]

(In reply to comment #49)
> (From update of attachment 70402 [details])
> Looks fine, just make sure this is very well tested, for the case not using AC

I tested it manually and there's no change in rednering - in the non-AC case all I do is move the if() to be outside the for(). 

I tried to keep this change separate in case it does cause a regression that I can't detect by myself.

[Eric Seidel (no email)]

Attachment 70402 [details] was posted by a committer and has review+, assigning to Noam Rosenthal for commit.

[WebKit Commit Bot]

Comment on attachment 70374 [details]
Patch 2: stub for media layer implementation.

Clearing flags on attachment: 70374

Committed r69772: <http://trac.webkit.org/changeset/69772>

[WebKit Commit Bot]

Comment on attachment 70402 [details]
Patch 5: have QWebFrame render the layer it got from TextureMapper

Clearing flags on attachment: 70402

Committed r69777: <http://trac.webkit.org/changeset/69777>

[Noam Rosenthal]

Patch 1 and patch 4 are still awaiting review... any takers?

[Kenneth Rohde Christiansen]

Comment on attachment 70401 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

View in context: https://bugs.webkit.org/attachment.cgi?id=70401&action=review

> WebCore/platform/qt/QWebPageClient.h:63
> +#if USE(TEXTURE_MAPPER)
> +    virtual void setRootGraphicsLayer(WebCore::TextureMapperPlatformLayer* layer) { }
> +#else
> +    virtual void setRootGraphicsLayer(QGraphicsObject* layer) { }
> +#endif

Why not just use NativeLayer instead of QGraphicsObject* etc, now that these typedefs exists?

> WebKit/qt/WebCoreSupport/PageClientQt.cpp:58
> +    void setSizeChanged(const IntSize& newSize)
> +    {
> +    }

Add a comment why this is umimplemented

[Kenneth Rohde Christiansen]

Comment on attachment 70212 [details]
Patch 1: OpenGL backend

Looks fine as an initial commit. We can take it from here in future patches if needed.

[WebKit Commit Bot]

Comment on attachment 70212 [details]
Patch 1: OpenGL backend

Clearing flags on attachment: 70212

Committed r70452: <http://trac.webkit.org/changeset/70452>

[Simon Hausmann]

Comment on attachment 70401 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

View in context: https://bugs.webkit.org/attachment.cgi?id=70401&action=review

> WebCore/platform/qt/QWebPageClient.h:-36
> +#else
> +class QGraphicsObject;
> +#endif
> +
>  QT_BEGIN_NAMESPACE
> -class QGraphicsItem;

Theoretically the QGraphicsObject forward declaration should remain inside QT_BEGIN_NAMESPACE. Why #ifdef the forward declaration anyway? Why not always forward declare QGraphicsObject and TextureMapperPlatformLayer simply? :)

> WebCore/platform/qt/QWebPageClient.h:60
> +    virtual void setRootGraphicsLayer(WebCore::TextureMapperPlatformLayer* layer) { }

On a second thought, why not use the PlatformLayer typedef instead of #ifdef here? Is it a problem to include GraphicsLayer.h here?

[Noam Rosenthal]

Created attachment 71767 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

[Noam Rosenthal]

Created attachment 71768 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

[Kenneth Rohde Christiansen]

Comment on attachment 71768 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

View in context: https://bugs.webkit.org/attachment.cgi?id=71768&action=review

Apart from the ChangeLog, this looks ok.

> WebCore/ChangeLog:13
> +        No new tests. WIP on a new implementation.

What do you mean about this? Are no new tests needed? Are you working on a new implementation? then why should this go in?

> WebCore/platform/qt/QWebPageClient.h:55
> +    virtual void setRootGraphicsLayer(PlatformLayer* layer) { }

Now that is a lot better!

[Noam Rosenthal]

(In reply to comment #61)
> > +        No new tests. WIP on a new implementation.
> 
> What do you mean about this? Are no new tests needed? Are you working on a new implementation? then why should this go in?

The old compositing tests should be sufficient for this. But nothing is testable until the implementation is complete.

[Kenneth Rohde Christiansen]

(In reply to comment #62)
> (In reply to comment #61)
> > > +        No new tests. WIP on a new implementation.
> > 
> > What do you mean about this? Are no new tests needed? Are you working on a new implementation? then why should this go in?
> 
> The old compositing tests should be sufficient for this. But nothing is testable until the implementation is complete.

Now just add that to the ChangeLog!

[Noam Rosenthal]

Created attachment 71780 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

[WebKit Commit Bot]

Comment on attachment 70363 [details]
Patch 3: Add an opt-in #define to WebCore

Clearing flags on attachment: 70363

Committed r70485: <http://trac.webkit.org/changeset/70485>

[WebKit Commit Bot]

Comment on attachment 71780 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper

Clearing flags on attachment: 71780

Committed r70487: <http://trac.webkit.org/changeset/70487>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ariya Hidayat]

Reopened, because it breaks the build. I wonder how this can compile at all. Hint: "Platfrom".

[Noam Rosenthal]

Created attachment 72246 [details]
Build fix

[Noam Rosenthal]

Created attachment 72247 [details]
Build fix

[WebKit Commit Bot]

Comment on attachment 72247 [details]
Build fix

Clearing flags on attachment: 72247

Committed r70819: <http://trac.webkit.org/changeset/70819>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/70819 might have broken GTK Linux 32-bit Release

[Noam Rosenthal]

(In reply to comment #73)
> http://trac.webkit.org/changeset/70819 might have broken GTK Linux 32-bit Release
I doubt it, none of the changed files gets compiled for GTK...

[Noam Rosenthal]

Adding some patches using this bug

[Noam Rosenthal]

Created attachment 72346 [details]
Build fix for X11: Plugins

[Noam Rosenthal]

Created attachment 72347 [details]
Patch: small refactor to allow WebKit2 and remove globals

This is based to feedback from Simon.

[WebKit Review Bot]

Attachment 72347 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe.cpp"
WebCore/platform/graphics/opengl/TextureMapperGL.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/graphics/opengl/TextureMapperGL.cpp:41:  "GL/glx.h" already included at WebCore/platform/graphics/opengl/TextureMapperGL.cpp:37  [build/include] [4]
WebCore/platform/graphics/opengl/TextureMapperGL.cpp:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebCore/platform/graphics/opengl/TextureMapperGL.cpp:553:  Use 0 instead of NULL.  [readability/null] [5]
WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebframe_p.h"
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Noam Rosenthal]

Created attachment 72349 [details]
Patch: small refactor to allow WebKit2 and remove globals

[Early Warning System Bot]

Attachment 72347 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4842073

[Early Warning System Bot]

Attachment 72349 [details] did not build on qt:
Build output: http://queues.webkit.org/results/4748095

[Noam Rosenthal]

Created attachment 72374 [details]
Patch: small refactor to allow WebKit2 and remove globals

Oops, one of the lines was outside the #ifdef

[Andreas Kling]

Comment on attachment 72346 [details]
Build fix for X11: Plugins

r=me

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 72346 [details]:

fast/events/spatial-navigation/snav-clipped-overflowed-content.html
fast/dom/onerror-img.html

Please file bugs against the tests.  These tests were authored by [e-mail] and [e-mail].  The commit-queue is continuing to process your patch.

[Kenneth Rohde Christiansen]

Comment on attachment 72374 [details]
Patch: small refactor to allow WebKit2 and remove globals

View in context: https://bugs.webkit.org/attachment.cgi?id=72374&action=review

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:88
> +typedef GLXContextHandle GLContextHandle;

In WebKit we normally use *Context and not ContextHandle, maybe use GraphicsGLContext or so?

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:113
> +            TargetProgram,
> +            NumPrograms

I would add a new line before NumPrograms. Does this follow webkit naming?

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:154
> +            char infoLog[1024];
> +            int len;
> +            GL_CMD(glGetProgramInfoLog(programID, 1024, &len, infoLog));

You are reintroducing this part. Are you using the log for any thing now?

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:496
> +    GLuint newTextureID = m_textureMapper->pvt->directlyCompositedImages.findOrCreate(nativeImage, found);

I don't think we normally uses "Private" classes in WebCore, and I dont really like the pvt naming. Find out what is done in other cases in WebCore.

[Noam Rosenthal]

Created attachment 72436 [details]
Revised after Kenneth's comments

[Noam Rosenthal]

Created attachment 72437 [details]
Revised after Kenneth's comments

[Kenneth Rohde Christiansen]

Comment on attachment 72437 [details]
Revised after Kenneth's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=72437&action=review

> WebCore/platform/graphics/texmap/TextureMapper.h:1
> -/*
> + /*

Is thi change correct>?

> WebKit/qt/Api/qwebframe.cpp:328
> +#if USE(ACCELERATED_COMPOSITING) && USE(TEXTURE_MAPPER)
> +void QWebFramePrivate::renderCompositedLayers(GraphicsContext* context, const IntRect& clip)
> +{
> +    if (!rootGraphicsLayer)
> +        return;
> +    if (!textureMapper)
> +        textureMapper = TextureMapper::create(context);
> +    textureMapper->setGraphicsContext(context);
> +    textureMapper->setImageInterpolationQuality(context->imageInterpolationQuality());
> +    textureMapper->setTextDrawingMode(context->textDrawingMode());
> +    QPainter* painter = context->platformContext();
> +    FrameView* view = frame->view();
> +    painter->save();
> +    painter->beginNativePainting();
> +    TextureMapperContentLayer::PaintOptions options;
> +    options.visibleRect = clip;
> +    options.targetRect = view->frameRect();
> +    options.viewportSize = view->size();
> +    options.opacity = painter->opacity();
> +    rootGraphicsLayer->paint(textureMapper.get(), options);
> +    painter->endNativePainting();
> +    painter->restore();
> +}
> +#endif

Can't you use newlines to group this more logically... That would make reading it a bit easier... like an empty newline after return

[Noam Rosenthal]

Created attachment 72457 [details]
Same as reviewed patch, fixed the new line and space glitch

[Kenneth Rohde Christiansen]

Comment on attachment 72457 [details]
Same as reviewed patch, fixed the new line and space glitch

View in context: https://bugs.webkit.org/attachment.cgi?id=72457&action=review

> WebCore/platform/graphics/opengl/TextureMapperGL.cpp:497
> +    GLuint newTextureID = m_textureMapper->m_data->directlyCompositedImages.findOrCreate(nativeImage, found);

Btw, I would prefer an inline accessor for m_data. like data(). That is more out way to do things.

Meaning that in the class where m_data is defined m_data-> is fine, but when you use it elsewhere an inline accessor makes sense, also you could make it private and friend if not supposed to be used by others.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 72346 [details]:

webarchive/test-link-rel-icon.html

Please file bugs against the tests.  These tests were authored by [e-mail].  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 72346 [details]
Build fix for X11: Plugins

Clearing flags on attachment: 72346

Committed r71000: <http://trac.webkit.org/changeset/71000>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

Why was this closed? one patch was not committed yet.

[WebKit Review Bot]

Comment on attachment 72457 [details]
Same as reviewed patch, fixed the new line and space glitch

Rejecting patch 72457 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[WebKit Commit Bot]

Comment on attachment 72457 [details]
Same as reviewed patch, fixed the new line and space glitch

Rejecting patch 72457 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', 72457]" exit_code: 2
Last 500 characters of output:
aphics/qt/TextureMapperQt.cpp
patching file WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp
patching file WebCore/platform/graphics/texmap/TextureMapper.h
patching file WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h
patching file WebKit/qt/Api/qwebframe.cpp
patching file WebKit/qt/Api/qwebframe_p.h
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4765106

[WebKit Commit Bot]

Comment on attachment 72457 [details]
Same as reviewed patch, fixed the new line and space glitch

Rejecting patch 72457 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 72457]" exit_code: 2
Last 500 characters of output:
/TextureMapperQt.cpp
patching file WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp
patching file WebCore/platform/graphics/texmap/TextureMapper.h
patching file WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h
patching file WebKit/qt/Api/qwebframe.cpp
patching file WebKit/qt/Api/qwebframe_p.h
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/Users/abarth/git/webkit-queue/WebKitTools/Scripts/svn-apply', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4764117

[Noam Rosenthal]

Created attachment 72483 [details]
Patch

I changed m_data to data(). Can someone r+ it again? For some reason I couldn't do it myself...

[WebKit Commit Bot]

Comment on attachment 72483 [details]
Patch

Rejecting patch 72483 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', 72483]" exit_code: 2
Last 500 characters of output:
bCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp
patching file WebCore/platform/graphics/texmap/TextureMapper.h
patching file WebCore/platform/graphics/texmap/TextureMapperPlatformLayer.h
patching file WebKit/qt/Api/qwebframe.cpp
patching file WebKit/qt/Api/qwebframe_p.h
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Kenneth Rohde Christiansen', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4795130

[Noam Rosenthal]

Created attachment 72762 [details]
Patch

Some patch/diff issues... applying again

[WebKit Commit Bot]

Comment on attachment 72762 [details]
Patch

Clearing flags on attachment: 72762

Committed r71213: <http://trac.webkit.org/changeset/71213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

Reopening to allow some more patches.

[Noam Rosenthal]

Created attachment 73160 [details]
Prepare TextureMapper to be thread safe

This looks like a big patch, but most of it is just moving TextureMapperNode to its own file.

[Kenneth Rohde Christiansen]

Comment on attachment 73160 [details]
Prepare TextureMapper to be thread safe

View in context: https://bugs.webkit.org/attachment.cgi?id=73160&action=review

> WebCore/platform/graphics/qt/TextureMapperQt.h:24
> +#define TextureMapperQt_h
> +namespace WebCore {

Add newline between these

> WebCore/platform/graphics/texmap/TextureMapperNode.cpp:35
> +        inline int calculateCost() const

Rename to computeCost, to follow our nomenclature

> WebCore/platform/graphics/texmap/TextureMapperNode.cpp:81
> +        m_totalCost += m_data[i].calculateCost();
> +
> +
> +    for (int i = size-1; i >= 0 && m_totalCost > TextureMapperCache::MaxCost - TextureMapperCache::PurgeAmount; --i) {

Remove one of these newlines

> WebCore/platform/graphics/texmap/TextureMapperNode.cpp:193
> +        return true;
> +    const int size = m_children.size();

Add newline between these two

> WebCore/platform/graphics/texmap/TextureMapperNode.cpp:207
> +        return 0;
> +    int descendantsWithContent = (m_state.drawsContent || m_currentContent.contentType != HTMLContentType) ? 1 : 0;

here as well

> WebCore/platform/graphics/texmap/TextureMapperNode.cpp:329
> +        return;
> +    m_transforms.forDescendants.setM13(0);

newline between

[Noam Rosenthal]

Created attachment 73244 [details]
Patch

[Noam Rosenthal]

Comment on attachment 73244 [details]
Patch

Committed manually, r71538.

[Eric Seidel (no email)]

This broke the Snow Leopard bot (and thus the commit-queue).

[Noam Rosenthal]

(In reply to comment #108)
> This broke the Snow Leopard bot (and thus the commit-queue).

Temporary fix: 71577. Will re-add the test later.

[Eric Seidel (no email)]

Comment on attachment 72437 [details]
Revised after Kenneth's comments

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 72437 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 72483 [details]
Patch

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 72483 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 73160 [details]
Prepare TextureMapper to be thread safe

Cleared Kenneth Rohde Christiansen's review+ from obsolete attachment 73160 [details] so that this bug does not appear in http://webkit.org/pending-commit.