Bug 61925

Created attachment 95761 [details] Layer factory method implementation.

Attachment 95761 [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/GraphicsLayerTextureMapper.h:94: The parameter name "factory" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:428: Missing space before ( in if( [whitespace/parens] [5] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 95762 [details] Fix style error

As always would be nice with No'am's input

Comment on attachment 95762 [details] Fix style error View in context: https://bugs.webkit.org/attachment.cgi?id=95762&action=review > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:421 > +void GraphicsLayerTextureMapper::setGraphicsLayerFactory(GraphicsLayerFactory factory) What is this set method needed for?

(In reply to comment #4) > As always would be nice with No'am's input I'm working closely with Slava on all of these patches.

(In reply to comment #6) > (In reply to comment #4) > > As always would be nice with No'am's input > > I'm working closely with Slava on all of these patches. Having noted in the bug report that you are ok with the change would be nice :-)

(In reply to comment #5) > (From update of attachment 95762 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95762&action=review > > > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:421 > > +void GraphicsLayerTextureMapper::setGraphicsLayerFactory(GraphicsLayerFactory factory) > > What is this set method needed for? The WebProcess uses a different GraphicsLayer implementation than the UI process or WebKit1. So the idea is that the WebProcess would call this function on its startup to override the default GraphicsLayer::create.

I feel like the factory function should be in GraphicsLayer and not in GraphicsLayerTextureMapper. I'll post a patch soon.

Created attachment 96543 [details] Patch Moved the implementation from GraphicsLayerTextureMapper to GraphicsLayer. This will allow other ports that want to use GraphicsLayer across processes to use this factory, without relying on TextureMapper.

The build failure of windows is a problem with the test bot.

Comment on attachment 96543 [details] Patch Can't you just declare a static GraphicsLayer::create() method, and have the implementation in each platform's impl file?

(In reply to comment #12) > (From update of attachment 96543 [details]) > Can't you just declare a static GraphicsLayer::create() method, and have the implementation in each platform's impl file? It's a bit more complicated... We need a different implementation if we're in the web process vs. if we are in the UI process.

(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 96543 [details] [details]) > > Can't you just declare a static GraphicsLayer::create() method, and have the implementation in each platform's impl file? > > It's a bit more complicated... > We need a different implementation if we're in the web process vs. if we are in the UI process. I mean, a different implementation if we're in the web process vs. if we're in WebKit1. This is the only way we can use GraphicsLayers in this way with WebCore being compiled once for WebKit2 and WebKit1.

Comment on attachment 96543 [details] Patch Are you going to convert GraphicsLayer::create() to go through the factory for everyone, or just Qt?

(In reply to comment #15) > (From update of attachment 96543 [details]) > Are you going to convert GraphicsLayer::create() to go through the factory for everyone, or just Qt? Just Qt. I considered doing that for everyone but it's too complicated, since people might call GraphicsLayer::create from within their code.

Created attachment 96586 [details] Patch Revised the patch, added more information in the ChangeLog.

I think the factory stuff should be #ifdeffed for Qt then.

(In reply to comment #18) > I think the factory stuff should be #ifdeffed for Qt then. OK, I'll do that, and if other ports want to use the factory they can add it to the #ifdef.

Created attachment 96693 [details] Patch based on smfr's comments

Comment on attachment 96693 [details] Patch based on smfr's comments View in context: https://bugs.webkit.org/attachment.cgi?id=96693&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:372 > + typedef PassOwnPtr<GraphicsLayer> GraphicsLayerFactory(GraphicsLayerClient*); > + static void setGraphicsLayerFactory(GraphicsLayerFactory); We need a comment here explaining what it is for as that is not totally obvious - at least to me.

Created attachment 96733 [details] Patch with an additional comment

The commit-queue encountered the following flaky tests while processing attachment 96733 [details]: http/tests/local/stylesheet-and-script-load-order.html bug 62450 (author: koivisto@iki.fi) The commit-queue is continuing to process your patch.

Comment on attachment 96733 [details] Patch with an additional comment Clearing flags on attachment: 96733 Committed r88541: <http://trac.webkit.org/changeset/88541>

All reviewed patches have been landed. Closing bug.

Created attachment 96911 [details] Patch: Use GraphicsLayerFactory in TextureMapper

Comment on attachment 96911 [details] Patch: Use GraphicsLayerFactory in TextureMapper Clearing flags on attachment: 96911 Committed r88643: <http://trac.webkit.org/changeset/88643>

All reviewed patches have been landed. Closing bug.