Mac: Kill lots of AcceleratedWidget code (with fire)

Mac: Remove lots of AcceleratedWidget code

In the non-remote-CALayer model, we render to an IOSurface and pass it
to the browser process, where we draw that into a CAOpenGLLayer. Delete
all of the code to do that, and just set the contents of a CALayer to
be that IOSurface.

While we're in the neighborhood, change the software path to render into
an IOSurface, and use that same path to draw it. Double-buffer the
software output surface in SoftwareOutputDeviceMac, to avoid writing the
next frame with the CPU while the WindowServer is drawing it (it appears
that the WindowServer does not respect the IOSurface locks).

BUG=546691
Committed: https://crrev.com/3c29f968998edc5aca9f8b228196b29268938deb
Cr-Commit-Position: refs/heads/master@{#356129}

[ccameron, 2015-10-22 06:09:01]

Description was changed from

==========
Mac: Kill lots of AcceleratedWidget code (with fire)

In the non-remote-CALayer model, we render to an IOSurface and pass it
to the browser process, where we draw that into a CAOpenGLLayer. Delete
all of the code to do that, and just set the contents of a CALayer to
be that IOSurface.

While we're in the neighborhood, change the software path to render into
an IOSurface, and use that same path to draw it. Double-buffer the
software output surface in SoftwareOutputDeviceMac, to avoid writing the
next frame with the CPU wile the WindowServer is drawing it (it appears
that the WindowServer does not respect the IOSurface locks).

BUG=
==========

to

==========
Mac: Kill lots of AcceleratedWidget code (with fire)

In the non-remote-CALayer model, we render to an IOSurface and pass it
to the browser process, where we draw that into a CAOpenGLLayer. Delete
all of the code to do that, and just set the contents of a CALayer to
be that IOSurface.

While we're in the neighborhood, change the software path to render into
an IOSurface, and use that same path to draw it. Double-buffer the
software output surface in SoftwareOutputDeviceMac, to avoid writing the
next frame with the CPU while the WindowServer is drawing it (it appears
that the WindowServer does not respect the IOSurface locks).

BUG=
==========

[ccameron, 2015-10-22 06:09:27]

ccameron@chromium.org changed reviewers:
+ tapted@chromium.org

[ccameron, 2015-10-22 06:09:28]

(with fire)

https://codereview.chromium.org/1420533005/diff/1/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_iosurface_mac.cc (right):

https://codereview.chromium.org/1420533005/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_iosurface_mac.cc:15: const uint32
kIOSurfaceDimensionRoundup = 1;
This is needed so that the IOSurface will always be the size of the content that
is intended to be drawn. This code is going to be destroyed in a follow-up CL,
and the regression in memory usage is limited to when people are rapidly
resizing windows on 10.6-10.8.

https://codereview.chromium.org/1420533005/diff/1/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_iosurface_mac.cc:68:
AddIntegerValue(properties, kIOSurfacePixelFormat, pixel_format);
This property is needed for -[CALayer setContents:] to work.

[tapted, 2015-10-22 08:26:50]

this looks really neat! I started skimming, but then realised this will need
more brainpower than I have right now (I'm at a team offsite..), so all I have
are a few nits that I haven't really thought through yet. I'll have a proper
review waiting for you Monday next week (but it's OK if you want an alternate
set of eyes too :).

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:54: LOG(ERROR) <<
"Failed to lock previous IOSurface " << io_result;
nit: DLOG? (i.e. is this likely? There's some resistance to putting error
strings in the release build since it's so unlikely anyone will see them) More
elsewhere.

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:76: if (io_result) {
nit: curlies not needed

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:95:
io_surfaces_[i].reset(IOSurfaceCreate((CFDictionaryRef)options));
CFCast<CFDictionaryRef>(options) ? (from foundation_util.h)

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:130: if (io_result) {
nit: curlies not needed

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:140: current_index_ +=
1;
current_index_ = !current_index?

https://codereview.chromium.org/1420533005/diff/20001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_iosurface_mac.cc (right):

https://codereview.chromium.org/1420533005/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_iosurface_mac.cc:62: int pixel_format
= 'BGRA';
nit: unsigned?

https://codereview.chromium.org/1420533005/diff/20001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/20001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:19: @interface CALayer
(Private)
nit: Private -> PrivateAPI

("Private" could also imply it's our own private interface, that we define
ourselves, rather than exposing a private method that Apple implement)

[ccameron, 2015-10-22 19:19:00]

Description was changed from

==========
Mac: Kill lots of AcceleratedWidget code (with fire)

In the non-remote-CALayer model, we render to an IOSurface and pass it
to the browser process, where we draw that into a CAOpenGLLayer. Delete
all of the code to do that, and just set the contents of a CALayer to
be that IOSurface.

While we're in the neighborhood, change the software path to render into
an IOSurface, and use that same path to draw it. Double-buffer the
software output surface in SoftwareOutputDeviceMac, to avoid writing the
next frame with the CPU while the WindowServer is drawing it (it appears
that the WindowServer does not respect the IOSurface locks).

BUG=
==========

to

==========
Mac: Kill lots of AcceleratedWidget code (with fire)

In the non-remote-CALayer model, we render to an IOSurface and pass it
to the browser process, where we draw that into a CAOpenGLLayer. Delete
all of the code to do that, and just set the contents of a CALayer to
be that IOSurface.

While we're in the neighborhood, change the software path to render into
an IOSurface, and use that same path to draw it. Double-buffer the
software output surface in SoftwareOutputDeviceMac, to avoid writing the
next frame with the CPU while the WindowServer is drawing it (it appears
that the WindowServer does not respect the IOSurface locks).

BUG=546691
==========

[ccameron, 2015-10-22 19:26:11]

Thanks!

Adding kbr@ to get the review a bit sooner (I'm building another patch to remove
ImageTransportSurfaceFBO and friends next).

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:54: LOG(ERROR) <<
"Failed to lock previous IOSurface " << io_result;
On 2015/10/22 08:26:49, tapted wrote:
> nit: DLOG? (i.e. is this likely? There's some resistance to putting error
> strings in the release build since it's so unlikely anyone will see them) More
> elsewhere.

Done.

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:76: if (io_result) {
On 2015/10/22 08:26:49, tapted wrote:
> nit: curlies not needed

Done.

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:95:
io_surfaces_[i].reset(IOSurfaceCreate((CFDictionaryRef)options));
On 2015/10/22 08:26:49, tapted wrote:
> CFCast<CFDictionaryRef>(options) ? (from foundation_util.h)

Oops -- forgot to remove the C cast. Done.

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:130: if (io_result) {
On 2015/10/22 08:26:49, tapted wrote:
> nit: curlies not needed

Done.

https://codereview.chromium.org/1420533005/diff/20001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:140: current_index_ +=
1;
On 2015/10/22 08:26:49, tapted wrote:
> current_index_ = !current_index?

Done.

https://codereview.chromium.org/1420533005/diff/20001/content/common/gpu/imag...
File content/common/gpu/image_transport_surface_iosurface_mac.cc (right):

https://codereview.chromium.org/1420533005/diff/20001/content/common/gpu/imag...
content/common/gpu/image_transport_surface_iosurface_mac.cc:62: int pixel_format
= 'BGRA';
On 2015/10/22 08:26:50, tapted wrote:
> nit: unsigned?

Done.

https://codereview.chromium.org/1420533005/diff/20001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/20001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:19: @interface CALayer
(Private)
On 2015/10/22 08:26:50, tapted wrote:
> nit: Private -> PrivateAPI
> 
> ("Private" could also imply it's our own private interface, that we define
> ourselves, rather than exposing a private method that Apple implement)

Done.

[ccameron, 2015-10-23 17:46:15]

ccameron@chromium.org changed reviewers:
+ avi@chromium.org

[ccameron, 2015-10-23 17:46:15]

Adding avi@ (cause the bulk of the change is to CoreAnimation code).

[Avi (use Gerrit), 2015-10-23 19:41:58]

I'm willing to stamp, but I'm not qualified to review the graphics changes.

Also, the formatting... :(

https://codereview.chromium.org/1420533005/diff/40001/content/browser/composi...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/40001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:93:
kIOSurfaceBytesPerElement : @(bytesPerElement),
What is this formatting? Please don't tell me that clang format did it, because
I will force you to file a bug against it, and manually-format this before
committing it.

[ccameron, 2015-10-23 19:50:29]

I think I may have to wait for tapted then :(

https://codereview.chromium.org/1420533005/diff/40001/content/browser/composi...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/40001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:93:
kIOSurfaceBytesPerElement : @(bytesPerElement),
On 2015/10/23 19:41:58, Avi wrote:
> What is this formatting? Please don't tell me that clang format did it,
because
> I will force you to file a bug against it, and manually-format this before
> committing it.

I think it might have been "git cl format" ... but I can't reproduce it.

[tapted, 2015-10-26 02:51:42]

very cool change. Sorry for the delay in review. mostly just nits, just one
concern in the last comment about last_swap_size_dip_ and sofware frames.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
File content/browser/compositor/software_output_device_mac.h (right):

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.h:40: float scale_factor_;
this shadows a protected: member in cc::SoftwareOutputDevice .. it looks like
SoftwareOutputDeviceWin just writes into that in its overrides. Of course.. that
goes against style guides, so I think what you've done is right.

Really I think what's needed is a cc::SoftwareOutputDevice and a
cc::SoftwareOutputDeviceImpl or similar. So I guess this is fine as-is -- it
wouldn't need to change

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.h:49: gfx::Rect
previous_buffer_damage_rect_;
Could this be an SkRegion? might avoid a few conversions -- looks like the
SkRegion::op(..) overloads all redirect to one that works with SkRegions. Small
gains though, so I'll leave it up to you

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:7: #include
"content/browser/compositor/software_output_device_mac.h"
nit (while you're here): this line should be swapped with #import Cocoa

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:44:
previous_buffer_damage_rect_ = new_damage_rect;
e.g., with regions, this could be 

previous_buffer_damage_region_.swap(new_damage_region);

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:52:
io_surfaces_[!current_index_],
optional: this comes up a lot - maybe a private helper function like 

IOSurfaceRef SoftwareOutputDeviceMac::BackBuffer() { .. }

or similar. Also for FrontBuffer I guess. But not sure if this really adds
much.. up to you. I guess CopyPreviousBufferDamage could also take an
IOSurfaceRef argument

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:60: uint8_t* pixels =
reinterpret_cast<uint8_t*>(
static_cast? (pretty sure void* static casts to any pointer type)

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:62: size_t pitch =
IOSurfaceGetBytesPerRow(io_surfaces_[!current_index_]);
optional: pitch -> stride? (I'd actually never seen pitch for this before.. -
looks like they're interchangeable, and both used, but stride more common in
Chrome)

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:84: for (int i = 0; i <
2; ++i) {
Since it's a bit orthogonal to the core purpose, perhaps a helper function, and
here do, e.g.

if (!EnsureBuffersExist())
  return nullptr;

(should this even be instead in the EnsureBackbuffer() override ?)

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:96:
base::mac::CFCast<CFDictionaryRef>(options)));
whoops (my bad) - this should be NSToCFCast instead of CFCast (equivalent I
guess, but NSToCFCast is more constrained and doesn't need the "strict" version
with a runtime check)

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:99: DLOG(ERROR) <<
"Failed to allocateIOSurface";
nit: space after allocate

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:114: SkPMColor* pixels
= reinterpret_cast<SkPMColor*>(
static_cast

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:116: size_t pitch =
IOSurfaceGetBytesPerRow(io_surfaces_[current_index_]);
(optional: stride?)

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/BUILD.gn (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/BUILD.gn:34: # Required by io_surface_texture.mm.
This comment needs updating - perhaps call out io_surface_context.mm instead,
since I think that still needs the OpenGL refs.

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.gyp (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.gyp:40: # Required by
io_surface_texture.mm.
(here too)

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.h (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.h:171: bool flip_y);
is flip_y needed here? (i.e. would it be correct to require it to be false at
this point?)

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:110: int
AcceleratedWidgetMac::GetRendererID() const {
should this just be removed?

I guess it would get removed from
GpuProcessHostUIShim::OnAcceleratedSurfaceBuffersSwapped() and
AcceleratedSurfaceMsg_BufferPresented_Params as well, so that's a long thread to
pull - maybe a good follow-up

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:130: void
AcceleratedWidgetMac::BeginPumpingFrames() {
looks like these methods could be removed too (and might be a smaller thread -
should just touch render_widget_host_view_mac a bit, which is already in the CL
[since.. I never got around to poking this missing piece for MacViews so, yay,
one less thing \o/])

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:236: id new_contents =
io_surface.get() ? static_cast<id>(io_surface.get()) : nil;
is the ternary op needed?

(does `id new_contents = static_cast<id>(io_surface.get());` work?)

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:247: if ([local_layer_
respondsToSelector:(@selector(contentsScale))] &&
comment about this? (e.g. when would it be false - just snow leopard?)

Perhaps even `else { DCHECK(base::mac::IsOSSnowLeopard(); }` so we know that we
can remove the check when the IsOSSnowLeopard function is no longer required for
Chrome generally

https://codereview.chromium.org/1420533005/diff/80001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_unittest.mm:72: false);
Sadly the NativeWidgetMacTest.InvalidateShadow views_unittest fails with only
this (sorry - haven't yet made views_unittests part of the required tryjob
tasks). 

The problem is that previously AcceleratedWidgetMac::GotSoftwareFrame() would
update AcceleratedWidgetMac::last_swap_size_dip_ but now that only happens via
AcceleratedWidgetMac::GotAcceleratedFrame()

Maybe last_swap_size_dip_ isn't really needed for software frames... in which
case it's just the test that's broken - i.e. SimulateFrameSwap should simulate
an accelerated frame rather than a software frame [although I couldn't figure
out how to make this work in a unit test when I first made this - it's probably
fine to leave the test failing until I can figure it out]

But if last_swap_size_dip_ _should_ be updated for software frames then, one
option: the line that updates it in GotAcceleratedFrame() could be moved and
copied to GotAcceleratedCAContextFrame() and GotIOSurfaceFrame() [I've verified
that would fix the test]

[ccameron, 2015-10-26 06:40:34]

Thanks!! Updated.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
File content/browser/compositor/software_output_device_mac.h (right):

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.h:40: float scale_factor_;
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> this shadows a protected: member in cc::SoftwareOutputDevice .. it looks like
> SoftwareOutputDeviceWin just writes into that in its overrides. Of course..
that
> goes against style guides, so I think what you've done is right.
> 
> Really I think what's needed is a cc::SoftwareOutputDevice and a
> cc::SoftwareOutputDeviceImpl or similar. So I guess this is fine as-is -- it
> wouldn't need to change

Agree -- I think that cc::SoftwareOutputDevice needs a refactor to get rid of
the somewhat nasty inheritance patterns.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.h:49: gfx::Rect
previous_buffer_damage_rect_;
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> Could this be an SkRegion? might avoid a few conversions -- looks like the
> SkRegion::op(..) overloads all redirect to one that works with SkRegions.
Small
> gains though, so I'll leave it up to you

Yes, that simplifies things -- I've changed it (and the arg to the copy
function) to SkRegion.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:7: #include
"content/browser/compositor/software_output_device_mac.h"
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> nit (while you're here): this line should be swapped with #import Cocoa

Done.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:44:
previous_buffer_damage_rect_ = new_damage_rect;
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> e.g., with regions, this could be 
> 
> previous_buffer_damage_region_.swap(new_damage_region);

Done.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:52:
io_surfaces_[!current_index_],
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> optional: this comes up a lot - maybe a private helper function like 
> 
> IOSurfaceRef SoftwareOutputDeviceMac::BackBuffer() { .. }
> 
> or similar. Also for FrontBuffer I guess. But not sure if this really adds
> much.. up to you. I guess CopyPreviousBufferDamage could also take an
> IOSurfaceRef argument

Created a local variable for this (we don't use the previous buffer elsewhere).

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:60: uint8_t* pixels =
reinterpret_cast<uint8_t*>(
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> static_cast? (pretty sure void* static casts to any pointer type)

Done.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:62: size_t pitch =
IOSurfaceGetBytesPerRow(io_surfaces_[!current_index_]);
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> optional: pitch -> stride? (I'd actually never seen pitch for this before.. -
> looks like they're interchangeable, and both used, but stride more common in
> Chrome)

Done.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:84: for (int i = 0; i <
2; ++i) {
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> Since it's a bit orthogonal to the core purpose, perhaps a helper function,
and
> here do, e.g.
> 
> if (!EnsureBuffersExist())
>   return nullptr;
> 
> (should this even be instead in the EnsureBackbuffer() override ?)

I moved it to EnsureBuffersExist, since EnsureBackbuffer is void.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:96:
base::mac::CFCast<CFDictionaryRef>(options)));
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> whoops (my bad) - this should be NSToCFCast instead of CFCast (equivalent I
> guess, but NSToCFCast is more constrained and doesn't need the "strict"
version
> with a runtime check)

Done.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:99: DLOG(ERROR) <<
"Failed to allocateIOSurface";
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> nit: space after allocate

Done.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:114: SkPMColor* pixels
= reinterpret_cast<SkPMColor*>(
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> static_cast

Done.

https://codereview.chromium.org/1420533005/diff/80001/content/browser/composi...
content/browser/compositor/software_output_device_mac.mm:116: size_t pitch =
IOSurfaceGetBytesPerRow(io_surfaces_[current_index_]);
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> (optional: stride?)

Done.

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/BUILD.gn (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/BUILD.gn:34: # Required by io_surface_texture.mm.
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> This comment needs updating - perhaps call out io_surface_context.mm instead,
> since I think that still needs the OpenGL refs.

Actually, this can be removed now.

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.gyp (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.gyp:40: # Required by
io_surface_texture.mm.
On 2015/10/26 02:51:41, tapted (OOO soon) wrote:
> (here too)

Removed.

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.h (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.h:171: bool flip_y);
On 2015/10/26 02:51:42, tapted (OOO soon) wrote:
> is flip_y needed here? (i.e. would it be correct to require it to be false at
> this point?)

It's still needed, because accelerated IOSurfaces are flipped. This is only true
for this patch -- the next patch (which switches everything to surfaceless
mode), makes the flag no longer used.

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
File ui/accelerated_widget_mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:110: int
AcceleratedWidgetMac::GetRendererID() const {
On 2015/10/26 02:51:42, tapted (OOO soon) wrote:
> should this just be removed?
> 
> I guess it would get removed from
> GpuProcessHostUIShim::OnAcceleratedSurfaceBuffersSwapped() and
> AcceleratedSurfaceMsg_BufferPresented_Params as well, so that's a long thread
to
> pull - maybe a good follow-up

Yes, this is on the chopping block for future patches.

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:130: void
AcceleratedWidgetMac::BeginPumpingFrames() {
On 2015/10/26 02:51:42, tapted (OOO soon) wrote:
> looks like these methods could be removed too (and might be a smaller thread -
> should just touch render_widget_host_view_mac a bit, which is already in the
CL
> [since.. I never got around to poking this missing piece for MacViews so, yay,
> one less thing \o/])

Yup, going to get burned, too.

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:236: id new_contents =
io_surface.get() ? static_cast<id>(io_surface.get()) : nil;
On 2015/10/26 02:51:42, tapted (OOO soon) wrote:
> is the ternary op needed?
> 
> (does `id new_contents = static_cast<id>(io_surface.get());` work?)

No, it's not needed -- removed.

https://codereview.chromium.org/1420533005/diff/80001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/accelerated_widget_mac.mm:247: if ([local_layer_
respondsToSelector:(@selector(contentsScale))] &&
On 2015/10/26 02:51:42, tapted (OOO soon) wrote:
> comment about this? (e.g. when would it be false - just snow leopard?)
> 
> Perhaps even `else { DCHECK(base::mac::IsOSSnowLeopard(); }` so we know that
we
> can remove the check when the IsOSSnowLeopard function is no longer required
for
> Chrome generally

Ah, that's a good idea. Yes, this check is for 10.6 only.

https://codereview.chromium.org/1420533005/diff/80001/ui/views/widget/native_...
File ui/views/widget/native_widget_mac_unittest.mm (right):

https://codereview.chromium.org/1420533005/diff/80001/ui/views/widget/native_...
ui/views/widget/native_widget_mac_unittest.mm:72: false);
On 2015/10/26 02:51:42, tapted (OOO soon) wrote:
> Sadly the NativeWidgetMacTest.InvalidateShadow views_unittest fails with only
> this (sorry - haven't yet made views_unittests part of the required tryjob
> tasks). 
> 
> The problem is that previously AcceleratedWidgetMac::GotSoftwareFrame() would
> update AcceleratedWidgetMac::last_swap_size_dip_ but now that only happens via
> AcceleratedWidgetMac::GotAcceleratedFrame()
> 
> Maybe last_swap_size_dip_ isn't really needed for software frames... in which
> case it's just the test that's broken - i.e. SimulateFrameSwap should simulate
> an accelerated frame rather than a software frame [although I couldn't figure
> out how to make this work in a unit test when I first made this - it's
probably
> fine to leave the test failing until I can figure it out]
> 
> But if last_swap_size_dip_ _should_ be updated for software frames then, one
> option: the line that updates it in GotAcceleratedFrame() could be moved and
> copied to GotAcceleratedCAContextFrame() and GotIOSurfaceFrame() [I've
verified
> that would fix the test]

I went with the last solution -- yes, last_swap_size_dip_ should be updated.

[tapted, 2015-10-26 06:59:43]

sweet! lgtm

https://codereview.chromium.org/1420533005/diff/120001/content/browser/compos...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/120001/content/browser/compos...
content/browser/compositor/software_output_device_mac.mm:7: #import
"content/browser/compositor/software_output_device_mac.h"
I think this can stay #include if you like (one could argue either way - I'm
indifferent). But I meant that it should be the first include -- the Cocoa
include above should be after this line

[ccameron, 2015-10-26 20:21:59]

Thanks!!

https://codereview.chromium.org/1420533005/diff/120001/content/browser/compos...
File content/browser/compositor/software_output_device_mac.mm (right):

https://codereview.chromium.org/1420533005/diff/120001/content/browser/compos...
content/browser/compositor/software_output_device_mac.mm:7: #import
"content/browser/compositor/software_output_device_mac.h"
On 2015/10/26 06:59:42, tapted (OOO soon) wrote:
> I think this can stay #include if you like (one could argue either way - I'm
> indifferent). But I meant that it should be the first include -- the Cocoa
> include above should be after this line

Ah, okay. Flipped it back.

[ccameron, 2015-10-26 20:22:03]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-10-26 20:22:04]

The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org
Link to the patchset: https://codereview.chromium.org/1420533005/#ps140001
(title: "Feedback 2")

[commit-bot: I haz the power, 2015-10-26 20:22:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1420533005/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1420533005/140001

[ccameron, 2015-10-26 20:25:10]

Description was changed from

==========
Mac: Kill lots of AcceleratedWidget code (with fire)

In the non-remote-CALayer model, we render to an IOSurface and pass it
to the browser process, where we draw that into a CAOpenGLLayer. Delete
all of the code to do that, and just set the contents of a CALayer to
be that IOSurface.

While we're in the neighborhood, change the software path to render into
an IOSurface, and use that same path to draw it. Double-buffer the
software output surface in SoftwareOutputDeviceMac, to avoid writing the
next frame with the CPU while the WindowServer is drawing it (it appears
that the WindowServer does not respect the IOSurface locks).

BUG=546691
==========

to

==========
Mac: Remove lots of AcceleratedWidget code

In the non-remote-CALayer model, we render to an IOSurface and pass it
to the browser process, where we draw that into a CAOpenGLLayer. Delete
all of the code to do that, and just set the contents of a CALayer to
be that IOSurface.

While we're in the neighborhood, change the software path to render into
an IOSurface, and use that same path to draw it. Double-buffer the
software output surface in SoftwareOutputDeviceMac, to avoid writing the
next frame with the CPU while the WindowServer is drawing it (it appears
that the WindowServer does not respect the IOSurface locks).

BUG=546691
==========

[commit-bot: I haz the power, 2015-10-26 21:35:19]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-10-26 21:36:19]

Patchset 8 (id:??) landed as
https://crrev.com/3c29f968998edc5aca9f8b228196b29268938deb
Cr-Commit-Position: refs/heads/master@{#356129}