Use Surfaces on Windows, Linux, and Mac OS

Use Surfaces on Windows, Linux, and Mac OS

Surfaces should be feature-complete, so try enabling them on Windows, Linux, and Mac OS. They're still disabled on Chrome OS because they rely on single-threaded browser compositing, and on Android because they haven't been implemented there yet.

BUG=334090
Committed: https://crrev.com/9d3616042261b9c5aa8f13ba9ef45c1819850559
Cr-Commit-Position: refs/heads/master@{#307130}

[jbauman, 2014-12-04 22:06:57]

jbauman@chromium.org changed reviewers:
+ jamesr@chromium.org, piman@chromium.org

[jbauman, 2014-12-04 22:06:57]

Hopefully this works.

[jamesr, 2014-12-04 22:16:44]

Should work!

lgtm

[piman, 2014-12-05 21:13:15]

LGTM + 1 thing

2 questions:
1- what should we watch out for breakage?
2- what's the plan on Chrome OS and Android?
For Chrome OS, https://codereview.chromium.org/638653003/ just landed, so I
think we're close to being able to go back to single-thread, I guess we just
need to see it through.
For Android are there roadblocks? Will this work for both Chrome and WebView?

https://codereview.chromium.org/756453003/diff/1/content/public/common/conten...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/756453003/diff/1/content/public/common/conten...
content/public/common/content_switches.cc:862: const char kDisableSurfaces[] =
"disable-surfaces";
Mind adding this with other flags in
chrome/browser/chromeos/login/chrome_restart_request.cc ? So that it applies to
guest mode too. I know this doesn't work yet on Chrome OS, but we won't remember
to add flags when we add support.

[jbauman, 2014-12-05 21:57:18]

On 2014/12/05 21:13:15, piman (Very slow to review) wrote:
> LGTM + 1 thing
> 
> 2 questions:
> 1- what should we watch out for breakage?

I'm most worried for Chrome rendering black or hanging under unusual
circumstances - the usual set of possibilities. Also, I'm not certain how the
performance is, so we'll have to see on the bots.

> 2- what's the plan on Chrome OS and Android?
> For Chrome OS, https://codereview.chromium.org/638653003/ just landed, so I
> think we're close to being able to go back to single-thread, I guess we just
> need to see it through.

Right, once single-threaded compositing is enabled, we just need to check that
Chrome OS isn't doing anything unusual and unsupported before enabling this.

> For Android are there roadblocks? Will this work for both Chrome and WebView?
> 
For Android chrome I'm close to having a patch that works, but I want to make
sure our scheduling behavior good enough that it won't cause regressions.

For webview we'll need to add support for SetExternalDrawConstraints for
cc::Display at least. Once we've gotten those idiosyncratic features working,
hopefully it's just a matter of switching the parent compositor to be a display.

>
https://codereview.chromium.org/756453003/diff/1/content/public/common/conten...
> File content/public/common/content_switches.cc (right):
> 
>
https://codereview.chromium.org/756453003/diff/1/content/public/common/conten...
> content/public/common/content_switches.cc:862: const char kDisableSurfaces[] =
> "disable-surfaces";
> Mind adding this with other flags in
> chrome/browser/chromeos/login/chrome_restart_request.cc ? So that it applies
to
> guest mode too. I know this doesn't work yet on Chrome OS, but we won't
remember
> to add flags when we add support.

Ok, will do.

[piman, 2014-12-05 22:37:36]

On Fri, Dec 5, 2014 at 1:57 PM, <jbauman@chromium.org> wrote:

> On 2014/12/05 21:13:15, piman (Very slow to review) wrote:
>
>> LGTM + 1 thing
>>
>
>  2 questions:
>> 1- what should we watch out for breakage?
>>
>
> I'm most worried for Chrome rendering black or hanging under unusual
> circumstances - the usual set of possibilities. Also, I'm not certain how
> the
> performance is, so we'll have to see on the bots.
>
>  2- what's the plan on Chrome OS and Android?
>> For Chrome OS, https://codereview.chromium.org/638653003/ just landed,
>> so I
>> think we're close to being able to go back to single-thread, I guess we
>> just
>> need to see it through.
>>
>
> Right, once single-threaded compositing is enabled, we just need to check
> that
> Chrome OS isn't doing anything unusual and unsupported before enabling
> this.


We should double-check the ReflectorImpl. I think it should mostly work,
but there may be some subtleties around damage.

>
>
>  For Android are there roadblocks? Will this work for both Chrome and
>> WebView?
>>
>
> For Android chrome I'm close to having a patch that works, but I want to
> make
> sure our scheduling behavior good enough that it won't cause regressions.
>
> For webview we'll need to add support for SetExternalDrawConstraints for
> cc::Display at least. Once we've gotten those idiosyncratic features
> working,
> hopefully it's just a matter of switching the parent compositor to be a
> display.


Ok, cool, thanks.

>
>
>
> https://codereview.chromium.org/756453003/diff/1/content/
> public/common/content_switches.cc
>
>> File content/public/common/content_switches.cc (right):
>>
>
>
> https://codereview.chromium.org/756453003/diff/1/content/
> public/common/content_switches.cc#newcode862
>
>> content/public/common/content_switches.cc:862: const char
>> kDisableSurfaces[] =
>> "disable-surfaces";
>> Mind adding this with other flags in
>> chrome/browser/chromeos/login/chrome_restart_request.cc ? So that it
>> applies
>>
> to
>
>> guest mode too. I know this doesn't work yet on Chrome OS, but we won't
>>
> remember
>
>> to add flags when we add support.
>>
>
> Ok, will do.
>
>
> https://codereview.chromium.org/756453003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2014-12-05 23:54:37]

The CQ bit was checked by jbauman@chromium.org

[commit-bot: I haz the power, 2014-12-05 23:55:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/756453003/20001

[commit-bot: I haz the power, 2014-12-06 00:56:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-06 00:57:27]

Patchset 2 (id:??) landed as
https://crrev.com/9d3616042261b9c5aa8f13ba9ef45c1819850559
Cr-Commit-Position: refs/heads/master@{#307130}

[gab, 2014-12-09 15:21:36]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/790823002/ by gab@chromium.org.

The reason for reverting is: Playing whack-a-mole for http://crbug.com/440060
and this CL sounds like it could potentially cause the problems we're seeing on
the Win7 bots (I'll unrevert if not)..