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!
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?
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.
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
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
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.
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
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
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
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?
(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.
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.
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?
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
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
(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
> 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.
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 :)
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
(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.
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?
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!
(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.
(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!
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.
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 tonikitoo@webkit.org and vestbo@webkit.org. The commit-queue is continuing to process your patch.
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.
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.
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 ddkilzer@webkit.org. The commit-queue is continuing to process your patch.
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
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
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.
2010-10-03 23:18 PDT, Noam Rosenthal
2010-10-04 00:22 PDT, Noam Rosenthal
2010-10-04 00:25 PDT, Noam Rosenthal
2010-10-04 06:06 PDT, Noam Rosenthal
2010-10-04 19:01 PDT, Noam Rosenthal
2010-10-05 11:03 PDT, Noam Rosenthal
2010-10-05 11:49 PDT, Noam Rosenthal
2010-10-07 12:33 PDT, Noam Rosenthal
2010-10-07 14:00 PDT, Noam Rosenthal
2010-10-07 15:15 PDT, Noam Rosenthal
2010-10-08 01:53 PDT, Noam Rosenthal
2010-10-08 01:59 PDT, Noam Rosenthal
2010-10-09 12:30 PDT, Noam Rosenthal
2010-10-09 12:37 PDT, Noam Rosenthal
2010-10-09 12:40 PDT, Noam Rosenthal
2010-10-09 12:46 PDT, Noam Rosenthal
2010-10-09 12:54 PDT, Noam Rosenthal
2010-10-09 14:42 PDT, Noam Rosenthal
2010-10-09 15:25 PDT, Noam Rosenthal
2010-10-10 07:15 PDT, Noam Rosenthal
2010-10-10 07:20 PDT, Noam Rosenthal
2010-10-25 10:42 PDT, Noam Rosenthal
2010-10-25 10:44 PDT, Noam Rosenthal
2010-10-25 12:19 PDT, Noam Rosenthal
2010-10-28 15:30 PDT, Noam Rosenthal
2010-10-28 15:32 PDT, Noam Rosenthal
2010-10-29 10:30 PDT, Noam Rosenthal
2010-10-29 10:43 PDT, Noam Rosenthal
2010-10-29 10:54 PDT, Noam Rosenthal
2010-10-29 12:51 PDT, Noam Rosenthal
2010-10-30 08:29 PDT, Noam Rosenthal
2010-10-30 08:33 PDT, Noam Rosenthal
2010-10-31 08:39 PDT, Noam Rosenthal
2010-10-31 20:21 PDT, Noam Rosenthal
2010-11-02 16:21 PDT, Noam Rosenthal
2010-11-05 20:30 PDT, Noam Rosenthal
2010-11-08 08:59 PST, Noam Rosenthal