Bug 77148 - [Qt][Texmap] Refactor TextureMapper API to use ImageBuffers when possible.
Summary: [Qt][Texmap] Refactor TextureMapper API to use ImageBuffers when possible.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
Depends on: 75918 77716
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-26 14:58 PST by Noam Rosenthal
Modified: 2012-02-03 09:39 PST (History)
15 users (show)

See Also:


Attachments
Patch 1: Clean up TextureMapper's content-upload code (35.60 KB, patch)
2012-01-26 15:08 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (36.45 KB, patch)
2012-01-27 08:44 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (55.88 KB, patch)
2012-01-29 22:39 PST, Noam Rosenthal
gns: commit-queue-
Details | Formatted Diff | Diff
Patch (232.77 KB, patch)
2012-01-30 17:45 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (57.22 KB, patch)
2012-01-30 17:51 PST, Noam Rosenthal
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (57.38 KB, patch)
2012-01-30 20:44 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Patch (57.27 KB, patch)
2012-01-31 06:12 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Revised patch based on Martin Robinson's comments. (56.93 KB, patch)
2012-02-02 16:07 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Revised patch based on Martin Robinson's comments. (56.51 KB, patch)
2012-02-02 16:09 PST, Noam Rosenthal
mrobinson: review+
Details | Formatted Diff | Diff
Patch (62.09 KB, patch)
2012-02-02 17:09 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Fixed the non-3d-rendering path (62.12 KB, patch)
2012-02-03 06:08 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff
Attempt at a patch that doesn't break the ARM bot (62.12 KB, patch)
2012-02-03 08:59 PST, Noam Rosenthal
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2012-01-26 14:58:10 PST
TextureMapper's backing-store code (tiling/uploads) is messy and hard to maintain. Time to give it some love.
Comment 1 Noam Rosenthal 2012-01-26 15:08:03 PST
Created attachment 124186 [details]
Patch 1: Clean up TextureMapper's content-upload code
Comment 2 Kenneth Rohde Christiansen 2012-01-26 15:18:03 PST
Comment on attachment 124186 [details]
Patch 1: Clean up TextureMapper's content-upload code

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

It is pretty amazing that you are able to remove so much code

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:615
> +        swizzleRgb(reinterpret_cast<uint32_t*>(qtImage.bits()), qtImage.size());

werent you going to rename this method just jocelyn asked you to?
Comment 3 Noam Rosenthal 2012-01-26 15:57:45 PST
(In reply to comment #2)
> (From update of attachment 124186 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124186&action=review
> 
> It is pretty amazing that you are able to remove so much code
> 
> > Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:615
> > +        swizzleRgb(reinterpret_cast<uint32_t*>(qtImage.bits()), qtImage.size());
> 
> werent you going to rename this method just jocelyn asked you to?

He asked that I add a comment in WebGraphicsLayer what I'm swizzling to/from. Here it should be inferred from the code but I can add a comment as well.
Comment 4 Jocelyn Turcotte 2012-01-27 07:20:47 PST
Comment on attachment 124186 [details]
Patch 1: Clean up TextureMapper's content-upload code

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

Went through it and it looks good, just got two nitpicks.

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:589
> +static void swizzleRgb(uint32_t* data, const IntSize& size)

It's called swizzleRGB with capitals in ShareableBitmap, maybe it's worth to keep the names consistent.

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:123
> +void swizzleRgb(Image*);

It's defined static and used only in TextureMapperGL.cpp.
Does this belong in TextureMapper.h?
Comment 5 Noam Rosenthal 2012-01-27 08:44:26 PST
Created attachment 124325 [details]
Patch
Comment 6 Noam Rosenthal 2012-01-29 22:06:47 PST
After some deeper tests with Instruments on Mac, this seems to still create unnecessary image copies on WebKit2.
I've decided to combine it with another patch that switches TextureMapperQt to a cross-platform implementation that uses ImageBuffer instead of QPixmap; Originally I thought of spliitting it to two patches, but a lot of the changes from the first patch don't make sense when the second patch is there...
So I'm working on a patch which moves things around in TextureMapper API and erases a lot of redundant code, but doesn't change any behavior or creates additional copies.
Comment 7 Noam Rosenthal 2012-01-29 22:39:34 PST
Created attachment 124497 [details]
Patch

Patch might be big but most of it is deletions :)
Comment 8 WebKit Review Bot 2012-01-29 22:42:01 PST
Attachment 124497 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/texmap/TextureMapper.h:59:  The parameter name "t" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:59:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:60:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:86:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Gustavo Noronha (kov) 2012-01-29 22:52:47 PST
Comment on attachment 124497 [details]
Patch

Attachment 124497 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11366468
Comment 10 Early Warning System Bot 2012-01-29 22:59:26 PST
Comment on attachment 124497 [details]
Patch

Attachment 124497 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11367458
Comment 11 Gyuyoung Kim 2012-01-29 23:00:04 PST
Comment on attachment 124497 [details]
Patch

Attachment 124497 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11359783
Comment 12 WebKit Review Bot 2012-01-29 23:01:36 PST
Comment on attachment 124497 [details]
Patch

Attachment 124497 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11354004
Comment 13 Kenneth Rohde Christiansen 2012-01-30 01:34:15 PST
Comment on attachment 124497 [details]
Patch

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

> Source/WebCore/platform/graphics/GraphicsContext.h:420
> +#if PLATFORM(QT)
> +        void concatTransform(const TransformationMatrix&);
> +        void setTransform(const TransformationMatrix&);
> +        TransformationMatrix getTransform() const;
> +#else
> +        void concatTransform(const TransformationMatrix& transform) { concatCTM(transform.toAffine()); }
> +        void setTransform(const TransformationMatrix& transform) { setCTM(transform.toAffine()); }
> +        TransformationMatrix getTransform() const { return TransformationMatrix(getCTM()); }
> +#endif

Maybe a comment here would be nice to know why this is needed for Qt. Isn't it a bit confusing to have both concatTransform and concatCTM?

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:121
> +
> +void BitmapTextureImageBuffer::updateContents(void* data, const IntRect& targetRect, PixelFormat format)
> +{

Maybe a comment here would be helpful for other ports?

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:85
> +    enum AccelerationMode { Accelerated, Unaccelerated };

In the case someone adds native directx support some day, would Software, OpenGL make more sense?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:191
> +    // This suld be a reference copy only.

suld*?
Comment 14 Noam Rosenthal 2012-01-30 17:45:24 PST
Created attachment 124641 [details]
Patch
Comment 15 Noam Rosenthal 2012-01-30 17:51:32 PST
Created attachment 124643 [details]
Patch
Comment 16 WebKit Review Bot 2012-01-30 17:54:22 PST
Attachment 124643 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/texmap/TextureMapper.h:59:  The parameter name "t" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:59:  The parameter name "format" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/texmap/TextureMapper.h:86:  The parameter name "mode" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 WebKit Review Bot 2012-01-30 18:45:35 PST
Comment on attachment 124643 [details]
Patch

Attachment 124643 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11366793
Comment 18 Noam Rosenthal 2012-01-30 20:44:37 PST
Created attachment 124664 [details]
Patch
Comment 19 Kenneth Rohde Christiansen 2012-01-31 01:31:44 PST
Comment on attachment 124664 [details]
Patch

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

Looks good to me, maybe someone else wants to have a look?

> Source/WebCore/ChangeLog:18
> +        Also, Functions that use TransformationMatrix were added to GraphicsContext, to allow for

functions*

> Source/WebCore/platform/graphics/GraphicsContext.cpp:781
> +// These fuctions should be implemented to support software-rendering of 3d transforms.

3D

> Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:590
> -static void swizzleBGRAToRGBA(uint32_t* data, const IntSize& size)
> +static void swizzleRGB(uint32_t* data, const IntSize& size)

wasnt the old name better?
Comment 20 Noam Rosenthal 2012-01-31 06:12:32 PST
Created attachment 124715 [details]
Patch

Addressed Kenneth's comments, fixed warnings.
Comment 21 WebKit Review Bot 2012-01-31 06:15:47 PST
Attachment 124715 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
From git://git.webkit.org/WebKit
   3b3325c..baf92d92 master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 106355 = 3b3325cd5a20bf357b8fdad5c0c1dcb448125ec4
r106356 = baf92d9262faea6b0ee0b2ada47f821fc179c331
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 22 Jocelyn Turcotte 2012-01-31 09:57:32 PST
(In reply to comment #20)
I know it's probably is difficult to split, but this patch is very difficult to follow.
I couldn't find anything to improve after looking at it, though I surely missed something.
Comment 23 Noam Rosenthal 2012-01-31 10:07:32 PST
(In reply to comment #22)
> (In reply to comment #20)
> I know it's probably is difficult to split, but this patch is very difficult to follow.
It's impossible to split without breaking something :(
However, it's a large patch mostly due to deletions. The main thing the patch actually does is move stuff around.
Comment 24 Noam Rosenthal 2012-02-02 07:59:54 PST
Anything I still need to explain/modify about this patch?
Comment 25 Jocelyn Turcotte 2012-02-02 08:12:04 PST
(In reply to comment #23)
> It's impossible to split without breaking something :(
> However, it's a large patch mostly due to deletions. The main thing the patch actually does is move stuff around.

If you say that most of this code was moved, you have my (pretty low value) vote.
I guess that progressively fixing introduced regression, at this stage, would take less efforts than chewing on that patch anyway.
Comment 26 Noam Rosenthal 2012-02-02 08:29:27 PST
> If you say that most of this code was moved, you have my (pretty low value) vote.
Your vote is high-value :)

> I guess that progressively fixing introduced regression, at this stage, would take less efforts than chewing on that patch anyway.

Also, this patch does not at all affect the mainstream code-path that uses GL, only the software fallback.
Comment 27 Martin Robinson 2012-02-02 10:38:37 PST
Comment on attachment 124715 [details]
Patch

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

Just a couple questions before I finish the review. Looks nice! I'm glad that we can share the ImageBuffer code with you.

> Source/WebCore/platform/graphics/GraphicsContext.h:413
> +#if ENABLE(3D_RENDERING)

You might want to have this be ENABLE(3D_RENDERING) && USE(TEXTURE_MAPPER). Then you could move the empty implementation out of Source/WebCore/platform/graphics/GraphicsContext.cpp.

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:193
> +#if ENABLE(3D_RENDERING)
> +    context->concat3DTransform(matrix);
> +#else
> +    context->concatCTM(matrix);
> +#endif

This is repeated a bit. Did you consider moving the #ifdefs into concat3DTransform and the other new methods on GraphicsContext.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:189
> +    {
> +        context->setImageInterpolationQuality(textureMapper->imageInterpolationQuality());
> +        context->setTextDrawingMode(textureMapper->textDrawingMode());
> +        context->translate(-dirtyRect.x(), -dirtyRect.y());
> +        layer->paintGraphicsLayerContents(*context, dirtyRect);
> +        if (m_currentContent.contentType == DirectImageContentType)
> +            context->drawImage(m_currentContent.image.get(), ColorSpaceDeviceRGB, m_state.contentsRect);
> +    }

What is the purpose of this new inner scope?

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:191
> +    // This suld be a reference copy only.

suld -> should

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:192
> +    RefPtr<Image> image = imageBuffer->copyImage();

I believe this is a deep copy on Cairo, at least. You may need to pass a copy behavior here. Looks like Cairo has not implemented the shallow copy behavior.
Comment 28 Noam Rosenthal 2012-02-02 10:45:22 PST
(In reply to comment #27)
> (From update of attachment 124715 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124715&action=review
> 
> Just a couple questions before I finish the review. Looks nice! I'm glad that we can share the ImageBuffer code with you.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:413
> > +#if ENABLE(3D_RENDERING)
> 
> You might want to have this be ENABLE(3D_RENDERING) && USE(TEXTURE_MAPPER). Then you could move the empty implementation out of Source/WebCore/platform/graphics/GraphicsContext.cpp.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:193
> > +#if ENABLE(3D_RENDERING)
> > +    context->concat3DTransform(matrix);
> > +#else
> > +    context->concatCTM(matrix);
> > +#endif
> 
> This is repeated a bit. Did you consider moving the #ifdefs into concat3DTransform and the other new methods on GraphicsContext.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:189
> > +    {
> > +        context->setImageInterpolationQuality(textureMapper->imageInterpolationQuality());
> > +        context->setTextDrawingMode(textureMapper->textDrawingMode());
> > +        context->translate(-dirtyRect.x(), -dirtyRect.y());
> > +        layer->paintGraphicsLayerContents(*context, dirtyRect);
> > +        if (m_currentContent.contentType == DirectImageContentType)
> > +            context->drawImage(m_currentContent.image.get(), ColorSpaceDeviceRGB, m_state.contentsRect);
> > +    }
> 
> What is the purpose of this new inner scope?
>
Not needed, it was there before because we a GraphicsContext on the stack. I'll remove that.

> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:191
> > +    // This suld be a reference copy only.
> 
> suld -> should
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:192
> > +    RefPtr<Image> image = imageBuffer->copyImage();
> 
> I believe this is a deep copy on Cairo, at least. You may need to pass a copy behavior here. Looks like Cairo has not implemented the shallow copy behavior.

Ah, yes. I need to specify DontCopyBackingStore, which is not really implemented in Qt/GTK. I don't think it's in the scope of this patch though, maybe I should add it as a FIXME?
Comment 29 Noam Rosenthal 2012-02-02 10:46:23 PST
(In reply to comment #28)
> (In reply to comment #27)
> > (From update of attachment 124715 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=124715&action=review
> > 
> > Just a couple questions before I finish the review. Looks nice! I'm glad that we can share the ImageBuffer code with you.
> > 
> > > Source/WebCore/platform/graphics/GraphicsContext.h:413
> > > +#if ENABLE(3D_RENDERING)
> > 
> > You might want to have this be ENABLE(3D_RENDERING) && USE(TEXTURE_MAPPER). Then you could move the empty implementation out of Source/WebCore/platform/graphics/GraphicsContext.cpp.

OK, I don't mind - I just thought that concat3DTransform is not inherently specific to TextureMapper, it's just used by TextureMapper.
Comment 30 Noam Rosenthal 2012-02-02 16:07:36 PST
Created attachment 125205 [details]
Revised patch based on Martin Robinson's comments.
Comment 31 Noam Rosenthal 2012-02-02 16:09:32 PST
Created attachment 125206 [details]
Revised patch based on Martin Robinson's comments.
Comment 32 Martin Robinson 2012-02-02 16:39:34 PST
Comment on attachment 125206 [details]
Revised patch based on Martin Robinson's comments.

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

Okay. I think I understand enough of this to give an r+ now. Very nice. The only change I request is to move the ImageBuffer implementations to another file (perhaps TextureMapperImageBuffer.cpp/h), to reduce the number of classes per-file. Eventually I think it would be nice to move the BitmapTexture classes to their own files too.

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:1338
> +#if ENABLE(3D_RENDERING)

I think you are missing a && USE(TEXTURE_MAPPER) here.

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:80
> +class BitmapTextureImageBuffer : public BitmapTexture {
> +    friend class TextureMapperImageBuffer;
> +public:

I think it makes sense to split out the ImageBuffer implementation into a new file.

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:194
> +    context->save();
> +    context->setAlpha(opacity);
> +#if ENABLE(3D_RENDERING)
> +    context->concat3DTransform(matrix);
> +#else
> +    context->concatCTM(matrix);
> +#endif
> +    context->drawImageBuffer(image, ColorSpaceDeviceRGB, targetRect);

It seems that if alpha == 1 and the transform is just a 2D translation we could avoid a copy? Perhaps not, but if so it might be a good fast path to add for the future!
Comment 33 Noam Rosenthal 2012-02-02 17:09:42 PST
Created attachment 125214 [details]
Patch
Comment 34 WebKit Review Bot 2012-02-02 20:00:46 PST
Comment on attachment 125214 [details]
Patch

Clearing flags on attachment: 125214

Committed r106620: <http://trac.webkit.org/changeset/106620>
Comment 35 WebKit Review Bot 2012-02-02 20:00:54 PST
All reviewed patches have been landed.  Closing bug.
Comment 36 Csaba Osztrogonác 2012-02-02 22:34:49 PST
Reopen, because it broke the build:

../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp: In member function 'virtual void WebCore::TextureMapperImageBuffer::beginClip(const WebCore::TransformationMatrix&, const WebCore::FloatRect&)':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:58: error: 'const class WebCore::TransformationMatrix' has no member named 'toAffine'
../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp: In member function 'virtual void WebCore::TextureMapperImageBuffer::drawTexture(const WebCore::BitmapTexture&, const WebCore::FloatRect&, const WebCore::TransformationMatrix&, float, const WebCore::BitmapTexture*)':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:98: error: no matching function for call to 'WebCore::GraphicsContext::concatCTM(const WebCore::TransformationMatrix&)'
../../../../Source/WebCore/platform/graphics/GraphicsContext.h:409: note: candidates are: void WebCore::GraphicsContext::concatCTM(const WebCore::AffineTransform&)
I
Comment 37 Csaba Osztrogonác 2012-02-02 22:40:20 PST
It seems that the false branch of ENABLE(3D_RENDERING) is broken.
( Qt ARM and Windows build is broken because of this bug. )
Comment 38 Csaba Osztrogonác 2012-02-02 23:02:31 PST
Sorry, but I had to roll it out to make builds work again: http://trac.webkit.org/changeset/106630

Please reland with fixed build.
Comment 39 Noam Rosenthal 2012-02-03 06:08:06 PST
Created attachment 125313 [details]
Fixed the non-3d-rendering path
Comment 40 WebKit Review Bot 2012-02-03 06:59:46 PST
Comment on attachment 125313 [details]
Fixed the non-3d-rendering path

Clearing flags on attachment: 125313

Committed r106659: <http://trac.webkit.org/changeset/106659>
Comment 41 WebKit Review Bot 2012-02-03 06:59:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 Csaba Osztrogonác 2012-02-03 07:19:52 PST
Reopen, because the build is still broken:

In file included from ../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:21:0:
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h: In static member function 'static WTF::PassOwnPtr<WebCore::TextureMapper> WebCore::TextureMapper::platformCreateAccelerated()':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:124:16: error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::TextureMapper>' requested
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:125:5: warning: control reaches end of non-void function
Comment 43 Csaba Osztrogonác 2012-02-03 07:42:42 PST
Comment on attachment 125313 [details]
Fixed the non-3d-rendering path

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

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:126
> +#if USE(TEXTURE_MAPPER_GL)
> +    static PassOwnPtr<TextureMapper> platformCreateAccelerated();
> +#else
> +    static PassOwnPtr<TextureMapper> platformCreateAccelerated()
> +    {
> +        return 0;
> +    }
> +#endif

It broke the Qt ARM build:

In file included from ../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:21:
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h: In static member function 'static WTF::PassOwnPtr<WebCore::TextureMapper> WebCore::TextureMapper::platformCreateAccelerated()':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:124: error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::TextureMapper>' requested
In file included from ../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.h:24,
                 from ../../../../Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:21:
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h: In static member function 'static WTF::PassOwnPtr<WebCore::TextureMapper> WebCore::TextureMapper::platformCreateAccelerated()':
../../../../Source/WebCore/platform/graphics/texmap/TextureMapper.h:124: error: conversion from 'int' to non-scalar type 'WTF::PassOwnPtr<WebCore::TextureMapper>' requested
Comment 44 Noam Rosenthal 2012-02-03 08:59:51 PST
Created attachment 125346 [details]
Attempt at a patch that doesn't break the ARM bot
Comment 45 Csaba Osztrogonác 2012-02-03 09:02:14 PST
(In reply to comment #44)
> Created an attachment (id=125346) [details]
> Attempt at a patch that doesn't break the ARM bot

It won't apply to trunk, because your wrong patch is still in it.
Comment 46 Csaba Osztrogonác 2012-02-03 09:04:48 PST
Additionally Balázs landed a speculative fix - http://trac.webkit.org/changeset/106668