Bug 82360 – pass alpha directly to player, rather than creating a layer (for performance)

• Component: New Bugs
• Status: RESOLVED FIXED

[Mike Reed]

pass alpha directly to player, rather than creating a layer (for performance)

[Mike Reed]

Created attachment 134105 [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]

Comment on attachment 134105 [details]
Patch

Attachment 134105 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12143684

[Mike Reed]

Created attachment 134119 [details]
Patch

[Mike Reed]

need to wait for webkit's DEPS on chrome to update to get the api change already landed there...

[WebKit Review Bot]

Comment on attachment 134119 [details]
Patch

Attachment 134119 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12148612

[Mike Reed]

Created attachment 134290 [details]
Patch

[Mike Reed]

Comment on attachment 134290 [details]
Patch

mis-fired on the +. Just meant to say ?

[Mike Reed]

PTAL

[Darin Fisher (:fishd, Google)]

Comment on attachment 134290 [details]
Patch

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

> Source/WebKit/chromium/public/WebMediaPlayer.h:119
> +    virtual void paint(WebCanvas* canvas, const WebRect& rect)

nit: Can you delete this old method and just fix-up the WEBKIT_USING_CG path to pass 0xFF?

[James Robinson]

Can we just delete the WEBKIT_USING_CG code from **/chromium/** completely? We don't compile in this mode and are unlikely to start doing so spontaneously.

[Adam Barth]

> Can we just delete the WEBKIT_USING_CG code from **/chromium/** completely? We don't compile in this mode and are unlikely to start doing so spontaneously.

My understanding is that we now delete WEBKIT_USING_CG.  It's untested, therefore it's broken.

[Mike Reed]

I would prefer to delete dead code unrelated to this change, in a separate CL, so I don't conflate the two ideas (and slightly complicate testing and revert possibilities). However, if deleting that code in just these files will get me approved, then I'll do it :)

[Mike Reed]

Created attachment 134565 [details]
Patch

[Mike Reed]

this patch also deletes (dead) code behind WEBKIT_USING_CG, and assumes WEBKIT_USING_SKIA

[Mike Reed]

PTAL

[James Robinson]

Comment on attachment 134565 [details]
Patch

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

I don't know enough about what this does to review it. You will need to find someone who is familiar with this code.

> Source/WebKit/chromium/src/WebMediaPlayerClientImpl.cpp:45
>  // WebCommon.h defines WEBKIT_USING_SKIA so this has to be included last.
> -#if WEBKIT_USING_SKIA
>  #include "PlatformContextSkia.h"

now that you've removed the #ifdef you should delete the comment and put this in the normal include location (sorted with other includes)

[Andrew Scherkus]

jamesr: I've been working with reed on the chromium-side changes

patch is r+ to me

[James Robinson]

Comment on attachment 134565 [details]
Patch

OK, that works for me (and would have been useful information to have on this bug earlier)

[Mike Reed]

Created attachment 134785 [details]
Patch

[Mike Reed]

#include moved into upper section with comment removed.

PTAL

[Stephen White]

Comment on attachment 134785 [details]
Patch

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

Looks good.  r=me

> Source/WebKit/chromium/ChangeLog:3
> +        pass alpha directly to player, rather than creating a layer (for performance)

Super-nit:  This should probably be a real sentence.

[WebKit Review Bot]

Comment on attachment 134785 [details]
Patch

Clearing flags on attachment: 134785

Committed r112682: <http://trac.webkit.org/changeset/112682>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.