As of http://trac.webkit.org/changeset/103990, rendering accelerated content in a browser window on the Aura desktop is broken. Instead of displaying the content, a uniform grey background is displayed instead. To reproduce, compile chromium with "use_aura=1", and attempt to view any accelerated content (e.g. poster circle).
As a starting point, reverting the change to WebExternalTextureLayerImpl.cpp:54, so that it is just return !!textureId(); instead of return !!textureId() && LayerChromium::drawsContent(); causes the problem to go away *in Aura* (I haven't tested this elsewhere). It seems to suggest that this class is not correctly informing the base class about the drawing of content.
Created attachment 121117 [details] Patch
It seems that PluginLayerChromium never calls setIsDrawable(), leaving a default of false when you call LayerChromium::drawsContent(). Shawn, does this look reasonable?
(In reply to comment #3) > It seems that PluginLayerChromium never calls setIsDrawable(), leaving a default of false when you call LayerChromium::drawsContent(). Shawn, does this look reasonable? Yeah, setIsDrawable needs to be explicitly called for any layers that intend to display stuff. good catch =) However, I don't think layers should set their own isDrawable by calling it. that should happen somewhere by whoever owns the reference to this layer. For example, in webcore this occurs in GraphicsLayer (i.e. GraphicsLayerChromium). So, I would guess that wherever aura creates its layers that end up being PluginLayerChromium types, that's where this initialization should occur.
Comment on attachment 121117 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121117&action=review Tests please. > Source/WebCore/ChangeLog:8 > + No new tests. (OOPS!) Nope, this will fail an SVN presubmit check. You need to remove this line and ideally write some tests covering this code change.
(In reply to comment #5) > (From update of attachment 121117 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121117&action=review > > Tests please. > > > Source/WebCore/ChangeLog:8 > > + No new tests. (OOPS!) > > Nope, this will fail an SVN presubmit check. You need to remove this line and ideally write some tests covering this code change. Tests for sure, but I wanted some feedback on the approach first :-)
Created attachment 121127 [details] Patch
Comment on attachment 121127 [details] Patch Does this look OK? If so, will a simple unit test suffice that creates a WebExternalTextureLayer and sets the drawability then checks it was set ok suffice?
Comment on attachment 121127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121127&action=review Is this what you meant, Shawn? > Source/WebKit/chromium/src/WebExternalTextureLayer.cpp:49 > +void WebExternalTextureLayer::setIsDrawable(bool drawsContent) why isn't this bool called "drawable"?
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
What I was expecting the following: WebExternalTextureLayer owns a WebExternalTextureLayerImpl that is essentially a PluginLayerChromium. So, if we want the WebExternalTextureLayer to assume that its Impl always drawable, it would unwrap the PluginLayerChromium and call setIsDrawable in the constructor of WebExternalTextureLayer. I wouldn't expect that we need to modify the public API. I'm also not clear how setIsDrawable ever gets called, anyway, with this patch? But maybe I'm completely missing something here, since I'm not entirely familiar with this part of code =) wjmaclean, will it be reasonable to call setIsDrawable in the WebExternalTextureLayer constructor?
(In reply to comment #11) > What I was expecting the following: WebExternalTextureLayer owns a WebExternalTextureLayerImpl that is essentially a PluginLayerChromium. So, if we want the WebExternalTextureLayer to assume that its Impl always drawable, it would unwrap the PluginLayerChromium and call setIsDrawable in the constructor of WebExternalTextureLayer. > > I wouldn't expect that we need to modify the public API. I'm also not clear how setIsDrawable ever gets called, anyway, with this patch? But maybe I'm completely missing something here, since I'm not entirely familiar with this part of code =) In ui/gfx/compositor/layer.cc . > wjmaclean, will it be reasonable to call setIsDrawable in the WebExternalTextureLayer constructor? Sure, I just though you wanted the layer to be set isDrawable in Aura (as per your comment #4), which is on the "chromium side of the fence", hence the modification to the public API. So it's OK just to have the layer to default to isDrawable() in the contructor?
(In reply to comment #12) > (In reply to comment #11) > > What I was expecting the following: WebExternalTextureLayer owns a WebExternalTextureLayerImpl that is essentially a PluginLayerChromium. So, if we want the WebExternalTextureLayer to assume that its Impl always drawable, it would unwrap the PluginLayerChromium and call setIsDrawable in the constructor of WebExternalTextureLayer. > > > > I wouldn't expect that we need to modify the public API. I'm also not clear how setIsDrawable ever gets called, anyway, with this patch? But maybe I'm completely missing something here, since I'm not entirely familiar with this part of code =) > > In ui/gfx/compositor/layer.cc . > > > wjmaclean, will it be reasonable to call setIsDrawable in the WebExternalTextureLayer constructor? > > Sure, I just though you wanted the layer to be set isDrawable in Aura (as per your comment #4), which is on the "chromium side of the fence", hence the modification to the public API. > > So it's OK just to have the layer to default to isDrawable() in the contructor? Yeah, sorry if I was unclear, as far as I see I think that's the right place. I'm making the distinction between WebExternalTextureLayer and the WebExternalTextureLayerImpl class, since one owns the other, very much like how GraphicsLayerChromium owns various LayerChromiums.
(In reply to comment #13) > > > Sure, I just though you wanted the layer to be set isDrawable in Aura (as per your comment #4), which is on the "chromium side of the fence", hence the modification to the public API. > > > > So it's OK just to have the layer to default to isDrawable() in the contructor? > > Yeah, sorry if I was unclear, as far as I see I think that's the right place. I'm making the distinction between WebExternalTextureLayer and the WebExternalTextureLayerImpl class, since one owns the other, very much like how GraphicsLayerChromium owns various LayerChromiums. OK, new patch in a few minutes ...
Created attachment 121136 [details] Patch
How does this look?
Comment on attachment 121136 [details] Patch Looks good to me; one very trivial nit. View in context: https://bugs.webkit.org/attachment.cgi?id=121136&action=review > Source/WebKit/chromium/tests/WebLayerTest.cpp:81 > + : WebExternalTextureLayer(node) style nit: this should probably be indented by 4 more spaces.
(In reply to comment #17) > (From update of attachment 121136 [details]) > Looks good to me; one very trivial nit. > > View in context: https://bugs.webkit.org/attachment.cgi?id=121136&action=review > > > Source/WebKit/chromium/tests/WebLayerTest.cpp:81 > > + : WebExternalTextureLayer(node) > > style nit: this should probably be indented by 4 more spaces. Sure, np! Before I resubmit the patch, did you have anything James?
Comment on attachment 121136 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121136&action=review Nope, I think this looks good except for style nits > Source/WebKit/chromium/tests/WebLayerTest.cpp:30 > #include "CompositorFakeWebGraphicsContext3D.h" > +#include "WebExternalTextureLayerImpl.h" > #include "WebCompositor.h" this doesn't look sorted correctly > Source/WebKit/chromium/tests/WebLayerTest.cpp:67 > +class WebExternalTextureLayerImplTest: public WebExternalTextureLayerImpl { space before the : > Source/WebKit/chromium/tests/WebLayerTest.cpp:72 > +class WebExternalTextureLayerTest: public WebExternalTextureLayer { space before the : > Source/WebKit/chromium/tests/WebLayerTest.cpp:79 > +private: newline before private: please > Source/WebKit/chromium/tests/WebLayerTest.cpp:80 > + WebExternalTextureLayerTest(const PassRefPtr<WebExternalTextureLayerImpl>& node) explicit, please
Created attachment 121146 [details] Patch
(In reply to comment #19) > (From update of attachment 121136 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121136&action=review > > Nope, I think this looks good except for style nits > > > Source/WebKit/chromium/tests/WebLayerTest.cpp:30 > > #include "CompositorFakeWebGraphicsContext3D.h" > > +#include "WebExternalTextureLayerImpl.h" > > #include "WebCompositor.h" > > this doesn't look sorted correctly Done. > > Source/WebKit/chromium/tests/WebLayerTest.cpp:67 > > +class WebExternalTextureLayerImplTest: public WebExternalTextureLayerImpl { > > space before the : Done. > > Source/WebKit/chromium/tests/WebLayerTest.cpp:72 > > +class WebExternalTextureLayerTest: public WebExternalTextureLayer { > > space before the : Done. > > Source/WebKit/chromium/tests/WebLayerTest.cpp:79 > > +private: > > newline before private: please Done. > > Source/WebKit/chromium/tests/WebLayerTest.cpp:80 > > + WebExternalTextureLayerTest(const PassRefPtr<WebExternalTextureLayerImpl>& node) > > explicit, please Done.
Comment on attachment 121146 [details] Patch R=me
Comment on attachment 121146 [details] Patch Attachment 121146 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11110285
Created attachment 121275 [details] Patch
(In reply to comment #23) > (From update of attachment 121146 [details]) > Attachment 121146 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/11110285 This compiles and runs fine when I use "make webkit_unit_tests" on my development machine, but it looks as if the EWS bot is perhaps expecting WEBKIT_IMPLEMENTATION to be defined? The error Source/WebKit/chromium/tests/WebLayerTest.cpp:82: error: no matching function for call to 'WebKit::WebExternalTextureLayer::WebExternalTextureLayer(const WTF::PassRefPtr<WebKit::WebExternalTextureLayerImpl>&)' suggests it's not seeing the definition inside the #if WEBKIT_IMPLEMENTATION in WebExternalTextureLayer.h, but the corresponding definition inside WebExternalTextureLayer.cpp isn't inside the same #if. Also, I've updated the patch to be a bit more explicit about the distinction between WebExternalTextureLayerImpl* and WebExternalTextureLayerImplTest*.
Created attachment 121306 [details] Patch
Revised approach, with impl-specific test.
Comment on attachment 121306 [details] Patch R=me - looks great
Comment on attachment 121306 [details] Patch Rejecting attachment 121306 [details] from commit-queue. New failing tests: http/tests/inspector/network/download.html Full output: http://queues.webkit.org/results/11145068
Comment on attachment 121306 [details] Patch Looks like flake, trying again
Comment on attachment 121306 [details] Patch Clearing flags on attachment: 121306 Committed r104209: <http://trac.webkit.org/changeset/104209>
All reviewed patches have been landed. Closing bug.