Simplify IOSurface CoreAnimation code: Part 2

Simplify IOSurface CoreAnimation code: Part 2

It isn't necessary to mess around with sharing CGLContextObjs anymore,
they were necessary only to share data between the frame subscriber
and the RenderWidgetHostViewMac. Just give each IOSurfaceLayer its
own CGLContextObj, and draw using immediate mode (using shaders
resulted in an increase in startup time).

BUG=314190
TBR=pfeldman@chromium.org
R=kbr@chromium.org

Committed: https://crrev.com/4040f1e2376a76ca4582cefe26b484a6ea2c5b8d
Cr-Commit-Position: refs/heads/master@{#293457}

[ccameron, 2014-09-04 09:32:48]

Going to try this again -- I'll see if I can repro some of the failures from
yesterday first.

https://codereview.chromium.org/541663002/diff/1/content/browser/compositor/i...
File content/browser/compositor/io_surface_layer_mac.mm (right):

https://codereview.chromium.org/541663002/diff/1/content/browser/compositor/i...
content/browser/compositor/io_surface_layer_mac.mm:286: -
(void)releaseCGLContext:(CGLContextObj)glContext {
This is the bit that is different this time -- we just note that the context is
being destroyed, and mark that we need to re-create the resources.

If this context were in a sharegroup (fortunately it's not), we'd have to worry
about cleaning up resources so that the sharegroup doesn't become cluttered.

[ccameron, 2014-09-04 21:28:38]

Okay, this definitely fixed the double-free issue
(TabCaptureApiPixelTest.EndToEnd crashed within a few iterations on the last
patch, ran 100 iterations with no issues on this patch).

[Ken Russell (switch to Gerrit), 2014-09-04 22:20:36]

LGTM with a couple of comments.

https://codereview.chromium.org/541663002/diff/1/content/browser/compositor/i...
File content/browser/compositor/io_surface_layer_mac.mm (right):

https://codereview.chromium.org/541663002/diff/1/content/browser/compositor/i...
content/browser/compositor/io_surface_layer_mac.mm:283: return
CGLRetainPixelFormat(pixel_format);
You're sure the contract is to retain these pixel format objects? It isn't clear
from the documentation. Also looks like you should override
releaseCGLPixelFormat: if you're overriding this.

https://codereview.chromium.org/541663002/diff/1/content/browser/devtools/ren...
File content/browser/devtools/renderer_overrides_handler_browsertest.cc (right):

https://codereview.chromium.org/541663002/diff/1/content/browser/devtools/ren...
content/browser/devtools/renderer_overrides_handler_browsertest.cc:128:
EXPECT_LE(std::abs(0x12-(int)SkColorGetR(color)), 1);
Needs to be resynced? These changes look unrelated.

[ccameron, 2014-09-04 23:01:01]

Thanks!

https://codereview.chromium.org/541663002/diff/1/content/browser/compositor/i...
File content/browser/compositor/io_surface_layer_mac.mm (right):

https://codereview.chromium.org/541663002/diff/1/content/browser/compositor/i...
content/browser/compositor/io_surface_layer_mac.mm:283: return
CGLRetainPixelFormat(pixel_format);
On 2014/09/04 22:20:36, Ken Russell wrote:
> You're sure the contract is to retain these pixel format objects? It isn't
clear
> from the documentation. Also looks like you should override
> releaseCGLPixelFormat: if you're overriding this.

Oh -- pixel_format is a ScopedTypeRef, so there is an implicit CFRelease when it
falls out of scope -- the retain is to ensure it has a ref when it is returned.
I've just gotten in the habit of always using ScopedTypeRef -- in this instance
it may cause more confusion than good (though if someone adds an early-out
somewhere, these things leak easily).

The default implementation of releaseCGLPixelFormat does the corresponding
CFRelease (same deal was in our override of releaseCGLContext -- the super call
does the release).

https://codereview.chromium.org/541663002/diff/1/content/browser/devtools/ren...
File content/browser/devtools/renderer_overrides_handler_browsertest.cc (right):

https://codereview.chromium.org/541663002/diff/1/content/browser/devtools/ren...
content/browser/devtools/renderer_overrides_handler_browsertest.cc:128:
EXPECT_LE(std::abs(0x12-(int)SkColorGetR(color)), 1);
On 2014/09/04 22:20:36, Ken Russell wrote:
> Needs to be resynced? These changes look unrelated.

Oh -- this was a test that was failing with this test (a while ago), and it was
much more useful to use EXPECT_LE (to see what the actual pixel values were).

[ccameron, 2014-09-04 23:01:05]

The CQ bit was checked by ccameron@chromium.org

[commit-bot: I haz the power, 2014-09-04 23:02:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ccameron@chromium.org/541663002/1

[commit-bot: I haz the power, 2014-09-05 04:34:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-05 04:34:20]

Try jobs failed on following builders:
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ccameron, 2014-09-05 04:40:57]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2014-09-05 04:41:23]

On 2014/09/05 04:34:20, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_swarming on tryserver.chromium.mac
>
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ RUN      ] ExtensionServiceTest.ExternalInstallMultiple
[2293:1799:0904/210648:2327765677105:FATAL:power_monitor.cc(38)] Check failed:
obs->power_monitor_thread_checker_.

Not my problem, try again.

[commit-bot: I haz the power, 2014-09-05 04:43:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ccameron@chromium.org/541663002/1

[commit-bot: I haz the power, 2014-09-05 05:54:02]

Committed patchset #1 (id:1) as d139a285dbef4bac42deb98796049128ed80b503

[commit-bot: I haz the power, 2014-09-10 03:37:25]

Patchset 1 (id:??) landed as
https://crrev.com/4040f1e2376a76ca4582cefe26b484a6ea2c5b8d
Cr-Commit-Position: refs/heads/master@{#293457}