Bug 84120 – [chromium] Convert WebPluginContainerImpl over to use WebExternalTextureLayer

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Convert WebPluginContainerImpl over to use WebExternalTextureLayer

[James Robinson]

Created attachment 137461 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 137461 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/src/WebExternalTextureLayer.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Created attachment 137469 [details]
rebased

[James Robinson]

Stylebot is wrong about the #include order, it doesn't realize that:

#include <public/WebExternalTextureLayer.h>

in WebExternalTextureLayer.cpp is #including the header for that class.

I'll try to get this up on github to make it easier to tell the code changes apart from the file moves, but it's currently giving me errors running remote hooks.

[WebKit Review Bot]

Attachment 137469 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebKit/chromium/src/WebExternalTextureLayer.cpp:29:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Created attachment 137628 [details]
patch 1 (for reference)

[James Robinson]

Created attachment 137629 [details]
patch 2 (for reference)

[Adrienne Walker]

Comment on attachment 137469 [details]
rebased

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

Thanks for splitting this up into two patches.  That made it way easier to review.

> Source/Platform/chromium/public/WebExternalTextureLayer.h:38
>  class WebExternalTextureLayerImpl;

Remove me.

> Source/Platform/chromium/public/WebExternalTextureLayer.h:59
>      // Sets the texture id that represents the layer, in the namespace of the
>      // compositor context.
>      WEBKIT_EXPORT void setTextureId(unsigned);
> -    WEBKIT_EXPORT unsigned textureId() const;
> +
> +    // Sets the IO surface id that represents this layer. setTextureId() and setIOSurfaceProperties() are mutually
> +    // exclusive - a layer can be either backed by a texture or an IO surface, but not both.
> +    WEBKIT_EXPORT void setIOSurfaceProperties(const WebSize&, unsigned ioSurfaceId);

A random design aside.  It's always struck me as a little bit of a wart that we have this class that gets used in these two modes, and these modes infect its usage all the way down, even though one path (seemingly?) is only used by npapi plugins and the other only by ppapi.  I kind of wish it wouldn't cost so much boilerplate to have them be two separate classes.  It doesn't have to be fixed here, but do you have any thoughts on how to make this better?

[James Robinson]

(In reply to comment #9)
> > Source/Platform/chromium/public/WebExternalTextureLayer.h:59
> >      // Sets the texture id that represents the layer, in the namespace of the
> >      // compositor context.
> >      WEBKIT_EXPORT void setTextureId(unsigned);
> > -    WEBKIT_EXPORT unsigned textureId() const;
> > +
> > +    // Sets the IO surface id that represents this layer. setTextureId() and setIOSurfaceProperties() are mutually
> > +    // exclusive - a layer can be either backed by a texture or an IO surface, but not both.
> > +    WEBKIT_EXPORT void setIOSurfaceProperties(const WebSize&, unsigned ioSurfaceId);
> 
> A random design aside.  It's always struck me as a little bit of a wart that we have this class that gets used in these two modes, and these modes infect its usage all the way down, even though one path (seemingly?) is only used by npapi plugins and the other only by ppapi.  I kind of wish it wouldn't cost so much boilerplate to have them be two separate classes.  It doesn't have to be fixed here, but do you have any thoughts on how to make this better?

I think it's a problem.  I think the first step to fixing things is to figure out how many of the features (meaning options like flipped, uvrect, premultiplied, etc) on this class are actually being used by the IOSurface path.  If it's a small number, then I think having a dedicated type for IOSurface backed layers would make more sense than having them share the texture type.  If IOSurface-backed layers can do pretty much everything that texture layers can do, then I'd like to continue to share as much code as possible.  I haven't been able to do this investigation yet but would like to soon - of course if anyone else gets to it first that'd be great.

[James Robinson]

Committed r114604: <http://trac.webkit.org/changeset/114604>