viz: Move some code into //components/viz/common.

viz: Move some code into //components/viz/common.

Move ServerGpuMemoryBufferManager out of //services/ui/common into
//components/viz/common. It is used in both the host process (to allocate
memory for clients), and the viz process (by the display compositor).

BUG=719931
TBR=jbauman@ for unused dependency removal from content/gpu/BUILD.gn

Review-Url: https://codereview.chromium.org/2939953004
Cr-Commit-Position: refs/heads/master@{#479830}
Committed: https://chromium.googlesource.com/chromium/src/+/950d9d8673a279b9c739db2b9d0aab5ae7f82e3b

[sadrul, 2017-06-14 20:57:02]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 20:57:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 21:01:23]

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

[commit-bot: I haz the power, 2017-06-14 21:01:23]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2017-06-14 21:08:26]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 21:08:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-06-14 21:10:03]

sadrul@chromium.org changed reviewers:
+ danakj@chromium.org, fsamuel@chromium.org

[sadrul, 2017-06-14 21:10:04]

https://codereview.chromium.org/2939953004/diff/20001/services/ui/gpu/BUILD.gn
File services/ui/gpu/BUILD.gn (right):

https://codereview.chromium.org/2939953004/diff/20001/services/ui/gpu/BUILD.g...
services/ui/gpu/BUILD.gn:25: "//components/viz/host/gpu",
This seems unfortunate. Should the code go into viz/common instead?

[Fady Samuel, 2017-06-14 21:15:22]

Cool! I was about to write the same CL! :D Thanks Sadrul! LGTM

[danakj, 2017-06-14 21:19:24]

On Wed, Jun 14, 2017 at 5:15 PM, <fsamuel@chromium.org> wrote:

> Cool! I was about to write the same CL! :D Thanks Sadrul! LGTM
>

> BUG=none

Maybe that is why. Is there no bug for this work?

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sadrul, 2017-06-14 21:21:49]

Description was changed from

==========
viz: Add a component for gpu host.

Move some code out of //services/ui into //components/viz/host/gpu.

BUG=none
==========

to

==========
viz: Add a component for gpu host.

Move some code out of //services/ui into //components/viz/host/gpu.

BUG=719931
==========

[sadrul, 2017-06-14 21:22:14]

On 2017/06/14 21:19:24, danakj wrote:
> On Wed, Jun 14, 2017 at 5:15 PM, <mailto:fsamuel@chromium.org> wrote:
> 
> > Cool! I was about to write the same CL! :D Thanks Sadrul! LGTM
> >
> 
> > BUG=none
> 
> Maybe that is why. Is there no bug for this work?

Turns out there is one (crbug.com/719931). Updated CL description.

[danakj, 2017-06-14 21:34:06]

https://codereview.chromium.org/2939953004/diff/20001/components/viz/host/gpu...
File components/viz/host/gpu/DEPS (right):

https://codereview.chromium.org/2939953004/diff/20001/components/viz/host/gpu...
components/viz/host/gpu/DEPS:2: "+services/ui/gpu/interfaces",
Is it weird to dep from components to services? Can you convince me this isn't
weird, and we don't need to do dependency injection instead?

[Fady Samuel, 2017-06-14 21:35:23]

https://codereview.chromium.org/2939953004/diff/20001/components/viz/host/gpu...
File components/viz/host/gpu/DEPS (right):

https://codereview.chromium.org/2939953004/diff/20001/components/viz/host/gpu...
components/viz/host/gpu/DEPS:2: "+services/ui/gpu/interfaces",
On 2017/06/14 21:34:06, danakj wrote:
> Is it weird to dep from components to services? Can you convince me this isn't
> weird, and we don't need to do dependency injection instead?

I think the interface should just move here...

https://codereview.chromium.org/2939953004/diff/20001/services/ui/gpu/BUILD.gn
File services/ui/gpu/BUILD.gn (right):

https://codereview.chromium.org/2939953004/diff/20001/services/ui/gpu/BUILD.g...
services/ui/gpu/BUILD.gn:25: "//components/viz/host/gpu",
On 2017/06/14 21:10:04, sadrul wrote:
> This seems unfortunate. Should the code go into viz/common instead?

Sure.

[sadrul, 2017-06-14 21:40:59]

https://codereview.chromium.org/2939953004/diff/20001/components/viz/host/gpu...
File components/viz/host/gpu/DEPS (right):

https://codereview.chromium.org/2939953004/diff/20001/components/viz/host/gpu...
components/viz/host/gpu/DEPS:2: "+services/ui/gpu/interfaces",
On 2017/06/14 21:35:23, Fady Samuel wrote:
> On 2017/06/14 21:34:06, danakj wrote:
> > Is it weird to dep from components to services? Can you convince me this
isn't
> > weird, and we don't need to do dependency injection instead?
> 
> I think the interface should just move here...

My opinion is that interfaces should remain under //services/. One of the
reasons we are moving code out of //services/ui/* into //components/viz is to
avoid introducing dependency on interface-implementation code in //services/
from //content. So it should be OK to leave just the interfaces in their place
under //services. This does not drag in additional dependency on impl code in
//services/, so this should be acceptable.

[danakj, 2017-06-14 21:52:08]

https://codereview.chromium.org/2939953004/diff/20001/components/viz/host/gpu...
File components/viz/host/gpu/DEPS (right):

https://codereview.chromium.org/2939953004/diff/20001/components/viz/host/gpu...
components/viz/host/gpu/DEPS:2: "+services/ui/gpu/interfaces",
On 2017/06/14 21:40:59, sadrul wrote:
> On 2017/06/14 21:35:23, Fady Samuel wrote:
> > On 2017/06/14 21:34:06, danakj wrote:
> > > Is it weird to dep from components to services? Can you convince me this
> isn't
> > > weird, and we don't need to do dependency injection instead?
> > 
> > I think the interface should just move here...
> 
> My opinion is that interfaces should remain under //services/. One of the
> reasons we are moving code out of //services/ui/* into //components/viz is to
> avoid introducing dependency on interface-implementation code in //services/
> from //content. So it should be OK to leave just the interfaces in their place
> under //services. This does not drag in additional dependency on impl code in
> //services/, so this should be acceptable.

Ok thanks, so the rule would be .../interfaces is okay, but nothing else. A
comment here explaining this rule would be appreciated, and maybe up at a higher
level in c/viz/README.md so you can point me at that in the future :)

One more Q to verify, but what you said there makes sense to me: Do the
interfaces in this directory always exist? Would there be cases where we want to
use this component but not use these services interfaces? In content? In tests?
(Assume tests are happy making mojo interfaces in general.)

[sadrul, 2017-06-14 22:43:37]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 22:43:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-06-14 22:57:57]

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/B...
File components/viz/common/BUILD.gn (right):

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/B...
components/viz/common/BUILD.gn:19: "//services/ui/gpu/interfaces",
does this need to be a public dep for a forward declare?

i'm not sure what the other deps are for, can you make sure they make sense as
public deps too for the whole directory?

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
File components/viz/common/server_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
components/viz/common/server_gpu_memory_buffer_manager.h:15: #include
"services/ui/gpu/interfaces/gpu_service.mojom.h"
move to the .cc and forward declare it?

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
components/viz/common/server_gpu_memory_buffer_manager.h:20: // mus locally.
Please update this comment, it is not right anymore I don't think, and could be
expanded to explain what we discussed about mus and gpu service using this
(without naming names cuz layering etc)?

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
components/viz/common/server_gpu_memory_buffer_manager.h:21: class
ServerGpuMemoryBufferManager : public gpu::GpuMemoryBufferManager,
While we're here, why is this called "Server"?

https://codereview.chromium.org/2939953004/diff/40001/services/ui/ws/BUILD.gn
File services/ui/ws/BUILD.gn (right):

https://codereview.chromium.org/2939953004/diff/40001/services/ui/ws/BUILD.gn...
services/ui/ws/BUILD.gn:138: "//components/viz/common",
I don't think this belongs as a public_dep? This component can be used without
this, even that header can be included without it.

[sadrul, 2017-06-14 23:16:14]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 23:16:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-06-14 23:17:05]

Description was changed from

==========
viz: Add a component for gpu host.

Move some code out of //services/ui into //components/viz/host/gpu.

BUG=719931
==========

to

==========
viz: Move some code into //components/viz/common.

Move ServerGpuMemoryBufferManager out of //services/ui/common into
//components/viz/common. It is used in both the host process (to allocate
memory for clients), and the viz process (by the display compositor).

BUG=719931
==========

[sadrul, 2017-06-14 23:18:34]

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/B...
File components/viz/common/BUILD.gn (right):

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/B...
components/viz/common/BUILD.gn:19: "//services/ui/gpu/interfaces",
On 2017/06/14 22:57:57, danakj wrote:
> does this need to be a public dep for a forward declare?
> 
> i'm not sure what the other deps are for, can you make sure they make sense as
> public deps too for the whole directory?

I moved gpu/ipc/common into deps. But the rest are exposed in the header file,
so it would be better to leave them in public deps.

For mojom, the user of ServerGMBManager has to inject in the mojom impl. So it
feels reasonable to me to make it a public dep too.

I am not really sure if all of these deps will apply equally to all code in
here. We could have sub-components here too, but maybe we can wait until this
grows a bit to see how/whether we can organize it better?

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
File components/viz/common/server_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
components/viz/common/server_gpu_memory_buffer_manager.h:15: #include
"services/ui/gpu/interfaces/gpu_service.mojom.h"
On 2017/06/14 22:57:57, danakj wrote:
> move to the .cc and forward declare it?

The user has to inject in the mojom interface during construction. So makes
sense to expose this dep to users?

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
components/viz/common/server_gpu_memory_buffer_manager.h:20: // mus locally.
On 2017/06/14 22:57:57, danakj wrote:
> Please update this comment, it is not right anymore I don't think, and could
be
> expanded to explain what we discussed about mus and gpu service using this
> (without naming names cuz layering etc)?

Done.

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
components/viz/common/server_gpu_memory_buffer_manager.h:21: class
ServerGpuMemoryBufferManager : public gpu::GpuMemoryBufferManager,
On 2017/06/14 22:57:57, danakj wrote:
> While we're here, why is this called "Server"?

It's used by the non-clients. It could be HostGMBManager, but then, it is used
from viz too (which is non-host). So we ended up with ServerGMBManager, while
clients use ClientGMBManager. I can rename it to something better if you have
suggestions.

https://codereview.chromium.org/2939953004/diff/40001/services/ui/ws/BUILD.gn
File services/ui/ws/BUILD.gn (right):

https://codereview.chromium.org/2939953004/diff/40001/services/ui/ws/BUILD.gn...
services/ui/ws/BUILD.gn:138: "//components/viz/common",
On 2017/06/14 22:57:57, danakj wrote:
> I don't think this belongs as a public_dep? This component can be used without
> this, even that header can be included without it.

You are right. Moved into dep.

[sadrul, 2017-06-15 00:50:17]

sadrul@chromium.org changed reviewers:
+ erg@chromium.org

[sadrul, 2017-06-15 00:50:18]

+erg@ for services/ui owner

[commit-bot: I haz the power, 2017-06-15 01:36:24]

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

[commit-bot: I haz the power, 2017-06-15 01:36:25]

Dry run: This issue passed the CQ dry run.

[sadrul, 2017-06-15 04:21:40]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 04:21:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-06-15 04:23:19]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-06-15 04:24:07]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[commit-bot: I haz the power, 2017-06-15 04:32:59]

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

[commit-bot: I haz the power, 2017-06-15 04:33:00]

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

[sadrul, 2017-06-15 04:34:07]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 04:34:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 06:31:25]

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

[commit-bot: I haz the power, 2017-06-15 06:31:27]

Dry run: This issue passed the CQ dry run.

[danakj, 2017-06-15 15:08:58]

LGTM

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/B...
File components/viz/common/BUILD.gn (right):

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/B...
components/viz/common/BUILD.gn:19: "//services/ui/gpu/interfaces",
On 2017/06/14 23:18:33, sadrul wrote:
> On 2017/06/14 22:57:57, danakj wrote:
> > does this need to be a public dep for a forward declare?
> > 
> > i'm not sure what the other deps are for, can you make sure they make sense
as
> > public deps too for the whole directory?
> 
> I moved gpu/ipc/common into deps. But the rest are exposed in the header file,
> so it would be better to leave them in public deps.
> 
> For mojom, the user of ServerGMBManager has to inject in the mojom impl. So it
> feels reasonable to me to make it a public dep too.
> 
> I am not really sure if all of these deps will apply equally to all code in
> here. We could have sub-components here too, but maybe we can wait until this
> grows a bit to see how/whether we can organize it better?

Similar to the header, the user of this has to construct the mojom types and
pass them in, so it needs to depend on the mojom targets in order to call the
constructors. That dep does not need to come from here.

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
File components/viz/common/server_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
components/viz/common/server_gpu_memory_buffer_manager.h:15: #include
"services/ui/gpu/interfaces/gpu_service.mojom.h"
On 2017/06/14 23:18:34, sadrul wrote:
> On 2017/06/14 22:57:57, danakj wrote:
> > move to the .cc and forward declare it?
> 
> The user has to inject in the mojom interface during construction. So makes
> sense to expose this dep to users?

I don't think we need to. The fact that they inject the mojom needs *they* need
to construct, and depend on, the mojom types. It doesn't need to come from here.

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
components/viz/common/server_gpu_memory_buffer_manager.h:21: class
ServerGpuMemoryBufferManager : public gpu::GpuMemoryBufferManager,
On 2017/06/14 23:18:34, sadrul wrote:
> On 2017/06/14 22:57:57, danakj wrote:
> > While we're here, why is this called "Server"?
> 
> It's used by the non-clients. It could be HostGMBManager, but then, it is used
> from viz too (which is non-host). So we ended up with ServerGMBManager, while
> clients use ClientGMBManager. I can rename it to something better if you have
> suggestions.

Ah there is a Client one. Hm.. we need a host+service name, this will surely
happen again. Idk what tho right now.

[sadrul, 2017-06-15 16:32:31]

The CQ bit was checked by sadrul@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-15 16:32:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sadrul, 2017-06-15 16:34:13]

Moved gpu/interfaces out of public_deps

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
File components/viz/common/server_gpu_memory_buffer_manager.h (right):

https://codereview.chromium.org/2939953004/diff/40001/components/viz/common/s...
components/viz/common/server_gpu_memory_buffer_manager.h:15: #include
"services/ui/gpu/interfaces/gpu_service.mojom.h"
On 2017/06/15 15:08:58, danakj wrote:
> On 2017/06/14 23:18:34, sadrul wrote:
> > On 2017/06/14 22:57:57, danakj wrote:
> > > move to the .cc and forward declare it?
> > 
> > The user has to inject in the mojom interface during construction. So makes
> > sense to expose this dep to users?
> 
> I don't think we need to. The fact that they inject the mojom needs *they*
need
> to construct, and depend on, the mojom types. It doesn't need to come from
here.

Good point. Thanks. Made this change.

[Elliot Glaysher, 2017-06-15 17:40:10]

//services/ui lgtm

[commit-bot: I haz the power, 2017-06-15 18:24:00]

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

[commit-bot: I haz the power, 2017-06-15 18:24:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sadrul, 2017-06-15 21:03:03]

Description was changed from

==========
viz: Move some code into //components/viz/common.

Move ServerGpuMemoryBufferManager out of //services/ui/common into
//components/viz/common. It is used in both the host process (to allocate
memory for clients), and the viz process (by the display compositor).

BUG=719931
==========

to

==========
viz: Move some code into //components/viz/common.

Move ServerGpuMemoryBufferManager out of //services/ui/common into
//components/viz/common. It is used in both the host process (to allocate
memory for clients), and the viz process (by the display compositor).

BUG=719931
TBR=jbauman@ for unused dependency removal from content/gpu/BUILD.gn
==========

[sadrul, 2017-06-15 21:03:30]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2017-06-15 21:03:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2939953004/#ps120001
(title: ".")

[sadrul, 2017-06-15 21:03:52]

sadrul@chromium.org changed reviewers:
+ jbauman@chromium.org

[sadrul, 2017-06-15 21:03:53]

+jbauman@ for content/gpu (TBR'ing since removing unused dependency)

[commit-bot: I haz the power, 2017-06-15 21:04:07]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 21:10:18]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497560610584330,
"parent_rev": "4b7bd924b5d4c938a1df51270fa7bd74e632f66a", "commit_rev":
"950d9d8673a279b9c739db2b9d0aab5ae7f82e3b"}

[commit-bot: I haz the power, 2017-06-15 21:10:34]

Description was changed from

==========
viz: Move some code into //components/viz/common.

Move ServerGpuMemoryBufferManager out of //services/ui/common into
//components/viz/common. It is used in both the host process (to allocate
memory for clients), and the viz process (by the display compositor).

BUG=719931
TBR=jbauman@ for unused dependency removal from content/gpu/BUILD.gn
==========

to

==========
viz: Move some code into //components/viz/common.

Move ServerGpuMemoryBufferManager out of //services/ui/common into
//components/viz/common. It is used in both the host process (to allocate
memory for clients), and the viz process (by the display compositor).

BUG=719931
TBR=jbauman@ for unused dependency removal from content/gpu/BUILD.gn

Review-Url: https://codereview.chromium.org/2939953004
Cr-Commit-Position: refs/heads/master@{#479830}
Committed:
https://chromium.googlesource.com/chromium/src/+/950d9d8673a279b9c739db2b9d0a...
==========

[commit-bot: I haz the power, 2017-06-15 21:10:36]

Committed patchset #6 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/950d9d8673a279b9c739db2b9d0a...