|
Description
Noam Rosenthal
2010-10-03 23:14:29 PDT
Created attachment 69601 [details]
Patch
Created attachment 69602 [details]
Patch
Created attachment 69603 [details]
Patch
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! Created attachment 69623 [details]
Patch
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? Created attachment 69729 [details]
Patch
(In reply to comment #7) > Created an attachment (id=69729) [details] > Patch Not up for review? Found some stability issues I wanted to figure out before resubmitting for review. Created attachment 69811 [details]
Patch
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.
Attachment 69811 [details] did not build on qt: Build output: http://queues.webkit.org/results/4141094 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 (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. Created attachment 69821 [details]
Patch
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.
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 Created attachment 70147 [details]
Patch
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.
Created attachment 70159 [details]
Patch - OpenGL implementation
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
Flaky?
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 Comment on attachment 70147 [details] Patch Clearing flags on attachment: 70147 Committed r69374: <http://trac.webkit.org/changeset/69374> 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. 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? Created attachment 70210 [details]
OpenGL implementation with Alexis' comments
Created attachment 70212 [details]
Patch 1: OpenGL backend
Re-upload, forgot the m_currentProgram thingy ;)
(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 70361 [details]
Slice 1: allow the media AC integration to compile (doesn't work yet)
Created attachment 70362 [details]
Patch 2: stub for media layer implementation.
Created attachment 70363 [details]
Patch 3: Add an opt-in #define to WebCore
Created attachment 70364 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper
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 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 { } 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? Created attachment 70374 [details]
Patch 2: stub for media layer implementation.
Did you mean to r- patch 4 instead of patch 3? Created attachment 70377 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper
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 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? (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 Created attachment 70401 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper
> 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.
Created attachment 70402 [details]
Patch 5: have QWebFrame render the layer it got from TextureMapper
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. Attachment 70402 [details] was posted by a committer and has review+, assigning to Noam Rosenthal for commit.
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> 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> Patch 1 and patch 4 are still awaiting review... any takers? 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 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.
Comment on attachment 70212 [details] Patch 1: OpenGL backend Clearing flags on attachment: 70212 Committed r70452: <http://trac.webkit.org/changeset/70452> 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? Created attachment 71767 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper
Created attachment 71768 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper
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! Created attachment 71780 [details]
Patch 4: glue layer (PageClientQt) opt-in for TextureMapper
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> 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> All reviewed patches have been landed. Closing bug. Reopened, because it breaks the build. I wonder how this can compile at all. Hint: "Platfrom". Created attachment 72246 [details]
Build fix
Created attachment 72247 [details]
Build fix
Comment on attachment 72247 [details] Build fix Clearing flags on attachment: 72247 Committed r70819: <http://trac.webkit.org/changeset/70819> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/70819 might have broken GTK Linux 32-bit Release (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... Adding some patches using this bug Created attachment 72346 [details]
Build fix for X11: Plugins
Created attachment 72347 [details]
Patch: small refactor to allow WebKit2 and remove globals
This is based to feedback from Simon.
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.
Created attachment 72349 [details]
Patch: small refactor to allow WebKit2 and remove globals
Attachment 72347 [details] did not build on qt: Build output: http://queues.webkit.org/results/4842073 Attachment 72349 [details] did not build on qt: Build output: http://queues.webkit.org/results/4748095 Created attachment 72374 [details]
Patch: small refactor to allow WebKit2 and remove globals
Oops, one of the lines was outside the #ifdef
Comment on attachment 72346 [details]
Build fix for X11: Plugins
r=me
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. Created attachment 72436 [details]
Revised after Kenneth's comments
Created attachment 72437 [details]
Revised after Kenneth's comments
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 Created attachment 72457 [details]
Same as reviewed patch, fixed the new line and space glitch
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 72346 [details] Build fix for X11: Plugins Clearing flags on attachment: 72346 Committed r71000: <http://trac.webkit.org/changeset/71000> All reviewed patches have been landed. Closing bug. Why was this closed? one patch was not committed yet. Comment on attachment 72457 [details] Same as reviewed patch, fixed the new line and space glitch Rejecting patch 72457 from review queue. noam.rosenthal@nokia.com 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. 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 72483 [details]
Patch
I changed m_data to data(). Can someone r+ it again? For some reason I couldn't do it myself...
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 Created attachment 72762 [details]
Patch
Some patch/diff issues... applying again
Comment on attachment 72762 [details] Patch Clearing flags on attachment: 72762 Committed r71213: <http://trac.webkit.org/changeset/71213> All reviewed patches have been landed. Closing bug. Reopening to allow some more patches. 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.
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 Created attachment 73244 [details]
Patch
Comment on attachment 73244 [details] Patch Committed manually, r71538. This broke the Snow Leopard bot (and thus the commit-queue). (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. 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. 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. 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. |