Bug 61925 – Add layer factory to GraphicsLayer for creating non-default layer type.

• Component: WebKit2
• Status: RESOLVED FIXED

[Viatcheslav Ostapenko]

Webkit2 Qt implementation will use this to create graphics layers.

[Viatcheslav Ostapenko]

Created attachment 95761 [details]
Layer factory method implementation.

[WebKit Review Bot]

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.

[Viatcheslav Ostapenko]

Created attachment 95762 [details]
Fix style error

[Kenneth Rohde Christiansen]

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

[Kenneth Rohde Christiansen]

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?

[Noam Rosenthal]

(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.

[Kenneth Rohde Christiansen]

(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 :-)

[Noam Rosenthal]

(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.

[Noam Rosenthal]

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

[Noam Rosenthal]

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.

[Noam Rosenthal]

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

[Simon Fraser (smfr)]

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?

[Noam Rosenthal]

(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.

[Noam Rosenthal]

(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.

[Simon Fraser (smfr)]

Comment on attachment 96543 [details]
Patch

Are you going to convert GraphicsLayer::create() to go through the factory for everyone, or just Qt?

[Noam Rosenthal]

(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.

[Noam Rosenthal]

Created attachment 96586 [details]
Patch

Revised the patch, added more information in the ChangeLog.

[Simon Fraser (smfr)]

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

[Noam Rosenthal]

(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.

[Noam Rosenthal]

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

[Kenneth Rohde Christiansen]

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.

[Noam Rosenthal]

Created attachment 96733 [details]
Patch with an additional comment

[WebKit Review Bot]

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: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Review Bot]

Comment on attachment 96733 [details]
Patch with an additional comment

Clearing flags on attachment: 96733

Committed r88541: <http://trac.webkit.org/changeset/88541>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Noam Rosenthal]

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

[WebKit Review Bot]

Comment on attachment 96911 [details]
Patch: Use GraphicsLayerFactory in TextureMapper

Clearing flags on attachment: 96911

Committed r88643: <http://trac.webkit.org/changeset/88643>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.