MacViews: Move content::AcceleratedWidget to new component, ui/accelerated_widget_mac

MacViews: Move content::AcceleratedWidget to new component, ui/accelerated_widget_mac

This permits ContextFactory on Mac to create composited surfaces for
ui/views.

Moves files: surface_handle_types_mac, accelerated_widget_mac,
software_layer_mac, and io_surface_{context,layer,texture}_mac to
ui/accelerated_widget_mac. accelerated_widget_mac.gyp and other
component friends are also added there.

content dependencies from these files are removed.

Only one real dependency was left: a call to GpuDataManager::
IsDriverBugWorkaroundActive(gpu::FORCE_GL_FINISH_AFTER_COMPOSITING),
which is now plumbed through when constructing an AcceleratedWidgetMac.

TBR=bsalomon@google.com
BUG=424058
Committed: https://crrev.com/736e14479ad04dbd4d9df9d277db2bde0a5f28c4
Cr-Commit-Position: refs/heads/master@{#306272}

[tapted, 2014-11-25 05:31:53]

tapted@chromium.org changed reviewers:
+ ccameron@chromium.org, danakj@chromium.org, piman@chromium.org

[ccameron, 2014-11-25 17:54:25]

Looks good in general, couple of suggestions.

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
File ui/compositor/mac/accelerated_widget_mac.h (right):

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
ui/compositor/mac/accelerated_widget_mac.h:19: #include
"ui/base/cocoa/remote_layer_api.h"
I'd prefer for software_layer to live in ui/compositor.mac like the rest of the
stuff here rather than being moved to ui/base (does it need to be in ui/base for
a reason I didn't see)?

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
ui/compositor/mac/accelerated_widget_mac.h:30: // TODO(tapted): Move this out of
content namespace.
I'd prefer to move this stuff to ui:: in this patch (it's not a big diff). I was
also wondering if this should be in gfx:: as-in gfx::AcceleratedWidget -- maybe
piman has a preference?

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
File ui/compositor/mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
ui/compositor/mac/accelerated_widget_mac.mm:39: gfx::Size ConvertSizeToDIP(float
scale_factor, const gfx::Size& size_in_pixel) {
(re-resolve against https://codereview.chromium.org/750173002/, once it lands)

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/io_sur...
File ui/compositor/mac/io_surface_layer_mac.h (right):

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/io_sur...
ui/compositor/mac/io_surface_layer_mac.h:30: 
I was wondering if to make the glFinish workaround argument to the creation of
the AcceleratedWidgetMac (then passed down here) versus making it a client
method. The way you went with is probably simpler (either way it's
somewhat-odd-plumbing).

[tapted, 2014-11-25 23:53:54]

tapted@chromium.org changed reviewers:
- danakj@chromium.org

[tapted, 2014-11-25 23:53:55]

thanks Chris! PTAL. piman, PTAL for OWNERS.

I delayed this a bit - out-of-process was easy, but I wanted to make sure I had
an in-process story figured out and working (This is poised in
http://crrev.com/756663004 )

I've also updated the ui/views changes which follow on from this:
http://crrev.com/710253002 

The changes here are mostly a vanilla file-move/namespace change except for the
GpuDataManagerImpl part. All other content dependencies were gone already thanks
to Chris :)

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
File ui/compositor/mac/accelerated_widget_mac.h (right):

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
ui/compositor/mac/accelerated_widget_mac.h:19: #include
"ui/base/cocoa/remote_layer_api.h"
On 2014/11/25 17:54:25, ccameron1 wrote:
> I'd prefer for software_layer to live in ui/compositor.mac like the rest of
the
> stuff here rather than being moved to ui/base (does it need to be in ui/base
for
> a reason I didn't see)?

Done. There was an earlier objection to it being in ui/gl, but with the new
approach, the follow-up to this for in-process (i.e. http://crrev.com/756663004
) no longer needs it in ui/gl, so I agree ui/compositor/mac feels nicer.

Done (merged http://crrev.com/723903002 to this CL)

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
ui/compositor/mac/accelerated_widget_mac.h:30: // TODO(tapted): Move this out of
content namespace.
On 2014/11/25 17:54:25, ccameron1 wrote:
> I'd prefer to move this stuff to ui:: in this patch (it's not a big diff). I
was
> also wondering if this should be in gfx:: as-in gfx::AcceleratedWidget --
maybe
> piman has a preference?

Done. (with ui:: for now so you can see how it looks [feels right to me])

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
File ui/compositor/mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/accele...
ui/compositor/mac/accelerated_widget_mac.mm:39: gfx::Size ConvertSizeToDIP(float
scale_factor, const gfx::Size& size_in_pixel) {
On 2014/11/25 17:54:25, ccameron1 wrote:
> (re-resolve against https://codereview.chromium.org/750173002/, once it lands)

Done.

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/io_sur...
File ui/compositor/mac/io_surface_layer_mac.h (right):

https://codereview.chromium.org/753933002/diff/20001/ui/compositor/mac/io_sur...
ui/compositor/mac/io_surface_layer_mac.h:30: 
On 2014/11/25 17:54:25, ccameron1 wrote:
> I was wondering if to make the glFinish workaround argument to the creation of
> the AcceleratedWidgetMac (then passed down here) versus making it a client
> method. The way you went with is probably simpler (either way it's
> somewhat-odd-plumbing).

re: odd plumbing: Ash faces a similar problem (i.e. injecting
content::GpuDataManager), which is solved by ash::GPUSupport and
ShellDelegateImpl::CreateGPUSupport().

I like the idea of an argument to AcceleratedWidgetMac - that way it can be fed
in through views::ViewsDelegate, which can be provided from chrome-land since it
has the content dependency already. Then we're following a similar pattern to
Ash.

Done.

[ccameron, 2014-11-26 18:36:52]

Awesome -- lgtm mod the couple of questions below

https://codereview.chromium.org/753933002/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/753933002/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:281:
content::GpuSurfaceTracker::Get()->AcquireNativeWidget(params.surface_id);
Shouldn't this be ui::AcceleratedWidgetMacGotAcceleratedFrame.

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/BUILD.gn
File ui/compositor/BUILD.gn (right):

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/BUILD.gn#n...
ui/compositor/BUILD.gn:91: if (is_mac) {
What is base needed for (events is latency_info, I just can't remember the dep
there)?

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/compositor...
File ui/compositor/compositor.gyp (right):

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.gyp:96: 'dependencies': [
(likewise WRT base)

[piman, 2014-11-26 20:58:08]

Unless I'm mistaken, except for one thing (that can be fixed, see below), the
parts in ui/compositor/mac don't actually interact with the compositor.
Would it make sense to move them to ui/base/cocoa?

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/mac/accele...
File ui/compositor/mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/mac/accele...
ui/compositor/mac/accelerated_widget_mac.mm:271: cc::SoftwareFrameData*
frame_data,
It doesn't look like frame_data is used at all (except to check for NULL). Can
we remove it?

[ccameron, 2014-11-26 21:16:18]

On 2014/11/26 20:58:08, piman (Very slow to review) wrote:
> Unless I'm mistaken, except for one thing (that can be fixed, see below), the
> parts in ui/compositor/mac don't actually interact with the compositor.
> Would it make sense to move them to ui/base/cocoa?

That would be reasonable as well -- I thought to put in ui/compositor because
it's only used by the compositor (it's not general purpose in the sense that
multiple things use it). Either way is fine by me.

[piman, 2014-11-26 21:30:47]

On Wed, Nov 26, 2014 at 1:16 PM, <ccameron@chromium.org> wrote:

> On 2014/11/26 20:58:08, piman (Very slow to review) wrote:
>
>> Unless I'm mistaken, except for one thing (that can be fixed, see below),
>> the
>> parts in ui/compositor/mac don't actually interact with the compositor.
>> Would it make sense to move them to ui/base/cocoa?
>>
>
> That would be reasonable as well -- I thought to put in ui/compositor
> because
> it's only used by the compositor (it's not general purpose in the sense
> that
> multiple things use it). Either way is fine by me.
>

Well, it's not even used (directly) by ui/compositor :)


>
> https://codereview.chromium.org/753933002/
>

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

[tapted, 2014-11-26 22:04:52]

On 2014/11/26 21:30:47, piman (Very slow to review) wrote:
> On Wed, Nov 26, 2014 at 1:16 PM, <mailto:ccameron@chromium.org> wrote:
> 
> > On 2014/11/26 20:58:08, piman (Very slow to review) wrote:
> >
> >> Unless I'm mistaken, except for one thing (that can be fixed, see below),
> >> the
> >> parts in ui/compositor/mac don't actually interact with the compositor.
> >> Would it make sense to move them to ui/base/cocoa?
> >>
> >
> > That would be reasonable as well -- I thought to put in ui/compositor
> > because
> > it's only used by the compositor (it's not general purpose in the sense
> > that
> > multiple things use it). Either way is fine by me.
> >
> 
> Well, it's not even used (directly) by ui/compositor :)

no CL update yet, but I had a quick look at how ui/base/cocoa might fit.

ui/base doesn't currently have a ui/gl dependency. However, it does have a cc
dependency (for android I think), which does have a ui/gl dep. So I guess adding
a ui/gl dependency in ui/base isn't really preventing things (i.e. gl/cc)
depending on ui/base that previously could (or did). However, the cc dependency
that ui/base currently has seems a bit weak - just for #includes/DEPS (not
linking), and just android.

WDYT about a new folder like src/ui/accelerated_widget_mac/ ? (or a better
folder name - suggestions?) Then just make it a standalone component. I think
this fits with the general chrome componentization goals, and there's a kind of
precedent already with ui/ozone .

[ccameron, 2014-11-26 22:11:42]

On 2014/11/26 22:04:52, tapted wrote:
> WDYT about a new folder like src/ui/accelerated_widget_mac/ ? (or a better
> folder name - suggestions?) Then just make it a standalone component. I think
> this fits with the general chrome componentization goals, and there's a kind
of
> precedent already with ui/ozone .

I like the idea of having its own component. That name seems good to me.

[piman, 2014-11-26 22:11:49]

On Wed, Nov 26, 2014 at 2:04 PM, <tapted@chromium.org> wrote:

> On 2014/11/26 21:30:47, piman (Very slow to review) wrote:
>
>> On Wed, Nov 26, 2014 at 1:16 PM, <mailto:ccameron@chromium.org> wrote:
>>
>
>  > On 2014/11/26 20:58:08, piman (Very slow to review) wrote:
>> >
>> >> Unless I'm mistaken, except for one thing (that can be fixed, see
>> below),
>> >> the
>> >> parts in ui/compositor/mac don't actually interact with the compositor.
>> >> Would it make sense to move them to ui/base/cocoa?
>> >>
>> >
>> > That would be reasonable as well -- I thought to put in ui/compositor
>> > because
>> > it's only used by the compositor (it's not general purpose in the sense
>> > that
>> > multiple things use it). Either way is fine by me.
>> >
>>
>
>  Well, it's not even used (directly) by ui/compositor :)
>>
>
> no CL update yet, but I had a quick look at how ui/base/cocoa might fit.
>
> ui/base doesn't currently have a ui/gl dependency. However, it does have a
> cc
> dependency (for android I think), which does have a ui/gl dep. So I guess
> adding
> a ui/gl dependency in ui/base isn't really preventing things (i.e. gl/cc)
> depending on ui/base that previously could (or did). However, the cc
> dependency
> that ui/base currently has seems a bit weak - just for #includes/DEPS (not
> linking), and just android.
>
> WDYT about a new folder like src/ui/accelerated_widget_mac/ ? (or a better
> folder name - suggestions?) Then just make it a standalone component. I
> think
> this fits with the general chrome componentization goals, and there's a
> kind of
> precedent already with ui/ozone .
>

SG too.


>
> https://codereview.chromium.org/753933002/
>

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

[tapted, 2014-11-27 02:38:34]

Patchset #5 (id:80001) has been deleted

[tapted, 2014-11-27 02:50:32]

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

[tapted, 2014-11-27 02:50:34]

As discussed - now a standalone component in ui/accelerated_widget_mac - PTAL

+avi for ui OWNERS (for the new component, as well as incoming DEPS). Please
take a look. Thanks!

https://codereview.chromium.org/753933002/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

https://codereview.chromium.org/753933002/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host_ui_shim.cc:281:
content::GpuSurfaceTracker::Get()->AcquireNativeWidget(params.surface_id);
On 2014/11/26 18:36:51, ccameron1 wrote:
> Shouldn't this be ui::AcceleratedWidgetMacGotAcceleratedFrame.

Yep - Done. I think it must have been relying on Koenig lookup.

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/BUILD.gn
File ui/compositor/BUILD.gn (right):

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/BUILD.gn#n...
ui/compositor/BUILD.gn:91: if (is_mac) {
On 2014/11/26 18:36:51, ccameron1 wrote:
> What is base needed for (events is latency_info, I just can't remember the dep
> there)?

ScopedCAActionDisabler is in ui/base, but it can be inlined. I think this dep
was only essential while SoftwareLayer was also in ui/base, so it can be removed
if we rely on the inlining. Done.

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/compositor...
File ui/compositor/compositor.gyp (right):

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.gyp:96: 'dependencies': [
On 2014/11/26 18:36:52, ccameron1 wrote:
> (likewise WRT base)

Done.

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/mac/accele...
File ui/compositor/mac/accelerated_widget_mac.mm (right):

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/mac/accele...
ui/compositor/mac/accelerated_widget_mac.mm:271: cc::SoftwareFrameData*
frame_data,
On 2014/11/26 20:58:08, piman (Very slow to review) wrote:
> It doesn't look like frame_data is used at all (except to check for NULL). Can
> we remove it?

Done. (InProcess doesn't need it either, I even had it just making a dummy
object to pass in).

[Avi (use Gerrit), 2014-11-27 03:16:02]

I'm no expert in graphics, but it's mostly a move. ui/ LGTM

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

https://codereview.chromium.org/753933002/diff/120001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/BUILD.gn:10: "accelerated_widget_export.h",
alphabetical

[tapted, 2014-11-27 05:58:41]

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/BUILD.gn
File ui/compositor/BUILD.gn (right):

https://codereview.chromium.org/753933002/diff/60001/ui/compositor/BUILD.gn#n...
ui/compositor/BUILD.gn:91: if (is_mac) {
On 2014/11/27 02:50:33, tapted wrote:
> On 2014/11/26 18:36:51, ccameron1 wrote:
> > What is base needed for (events is latency_info, I just can't remember the
dep
> > there)?
> 
> ScopedCAActionDisabler is in ui/base, but it can be inlined. I think this dep
> was only essential while SoftwareLayer was also in ui/base, so it can be
removed
> if we rely on the inlining. Done.

Actually, `gn check` complains if I remove the //ui/base dependency, so I've
brought it back.

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

https://codereview.chromium.org/753933002/diff/120001/ui/accelerated_widget_m...
ui/accelerated_widget_mac/BUILD.gn:10: "accelerated_widget_export.h",
On 2014/11/27 03:16:02, Avi wrote:
> alphabetical

Whoops. The gn build built fine, but I forgot to run the awesome `gn check`
(which would have told me I was missing a "_mac"). Fixed.

[piman, 2014-12-01 19:32:42]

lgtm

[tapted, 2014-12-01 21:56:27]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-12-01 21:57:13]

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

[commit-bot: I haz the power, 2014-12-01 22:02:39]

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

[commit-bot: I haz the power, 2014-12-01 22:02:41]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[tapted, 2014-12-01 22:18:48]

tapted@chromium.org changed reviewers:
+ bsalomon@google.com

[tapted, 2014-12-01 22:20:06]

+bsalomon@goog TBR for the refactoring change, moving the skia dep. i.e.

** Presubmit ERRORS **
Missing LGTM from OWNERS of dependencies added to DEPS:
    '+third_party/skia',

[tapted, 2014-12-01 22:20:20]

The CQ bit was checked by tapted@chromium.org

[commit-bot: I haz the power, 2014-12-01 22:21:14]

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

[commit-bot: I haz the power, 2014-12-01 22:26:00]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2014-12-01 22:26:58]

Patchset 7 (id:??) landed as
https://crrev.com/736e14479ad04dbd4d9df9d277db2bde0a5f28c4
Cr-Commit-Position: refs/heads/master@{#306272}