Bug 84120 - [chromium] Convert WebPluginContainerImpl over to use WebExternalTextureLayer
Summary: [chromium] Convert WebPluginContainerImpl over to use WebExternalTextureLayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-16 19:24 PDT by James Robinson
Modified: 2012-04-18 19:48 PDT (History)
6 users (show)

See Also:


Attachments
Patch (41.32 KB, patch)
2012-04-16 19:28 PDT, James Robinson
no flags Details | Formatted Diff | Diff
rebased (40.55 KB, patch)
2012-04-16 20:12 PDT, James Robinson
enne: review+
enne: commit-queue-
Details | Formatted Diff | Diff
patch 1 (for reference) (23.71 KB, patch)
2012-04-17 16:25 PDT, James Robinson
no flags Details | Formatted Diff | Diff
patch 2 (for reference) (17.10 KB, patch)
2012-04-17 16:26 PDT, James Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2012-04-16 19:24:51 PDT
[chromium] Convert WebPluginContainerImpl over to use WebExternalTextureLayer
Comment 1 James Robinson 2012-04-16 19:28:28 PDT
Created attachment 137461 [details]
Patch
Comment 2 WebKit Review Bot 2012-04-16 19:33:28 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 3 WebKit Review Bot 2012-04-16 19:33:53 PDT
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.
Comment 4 James Robinson 2012-04-16 20:12:22 PDT
Created attachment 137469 [details]
rebased
Comment 5 James Robinson 2012-04-16 20:13:29 PDT
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.
Comment 6 WebKit Review Bot 2012-04-16 20:15:33 PDT
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.
Comment 7 James Robinson 2012-04-17 16:25:55 PDT
Created attachment 137628 [details]
patch 1 (for reference)
Comment 8 James Robinson 2012-04-17 16:26:16 PDT
Created attachment 137629 [details]
patch 2 (for reference)
Comment 9 Adrienne Walker 2012-04-18 10:37:17 PDT
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?
Comment 10 James Robinson 2012-04-18 19:42:06 PDT
(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.
Comment 11 James Robinson 2012-04-18 19:48:28 PDT
Committed r114604: <http://trac.webkit.org/changeset/114604>