Move SharedBitmapManager implementation out of content/

Move SharedBitmapManager implementation out of content/

HostSharedBitmapManager is shifted to components/display_compositor.
ChildSharedBitmapManager is shifted to services/ui/public/cpp/bitmap.

The Mojo messages used in allocating shared bitmap is decoupled from
render_message_filter.h and moved to cc/ipc/shared_bitmap_manager.mojom.

Note that the SharedBitmapManager is an associated mojo interface of
RenderMessageFilter so as to maintain the existing FIFO order of mojo
messages between the two SharedBitmap allocation functions and the
rest of RenderMessageFilter functions.

BUG=670162
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2717213004
Cr-Commit-Position: refs/heads/master@{#462979}
Committed: https://chromium.googlesource.com/chromium/src/+/11ac780e68cd3fde64d7eac350f4ebf53ce75eea

[xlai (Olivia), 2017-02-28 19:00:18]

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

[commit-bot: I haz the power, 2017-02-28 19:01:06]

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-02-28 19:04:50]

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

[commit-bot: I haz the power, 2017-02-28 19:04:51]

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...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[xlai (Olivia), 2017-02-28 19:05:43]

xlai@chromium.org changed reviewers:
+ danakj@chromium.org, jam@chromium.org

[xlai (Olivia), 2017-02-28 19:05:44]

This is a refactoring patch that factors sharedbitmaps out of
content/ as part of the Project Mustash. Please let me know if you like the way
I arranged them.

danakj@: Please review display_compositor/
jam@: Please review content/

Also +cc fsamuel@ for interest.

[xlai (Olivia), 2017-02-28 19:34:38]

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

[commit-bot: I haz the power, 2017-02-28 19:35:17]

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-02-28 19:59:28]

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

[commit-bot: I haz the power, 2017-02-28 19:59:29]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[danakj, 2017-02-28 20:34:57]

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
File components/display_compositor/child/BUILD.gn (right):

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
components/display_compositor/child/BUILD.gn:10: defines = [
"DISPLAY_COMPOSITOR_IMPLEMENTATION" ]
separate components should have separate IMPLEMENTATION and export macros

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
File components/display_compositor/child/child_shared_bitmap_manager.cc (right):

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
components/display_compositor/child/child_shared_bitmap_manager.cc:97:
ChildSharedBitmapManager::AllocateSharedMemory(size_t buf_size) {
This should be a private method then, in the anon namespace of this .cc file

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
File components/display_compositor/interfaces/shared_bitmap_manager.mojom
(right):

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
components/display_compositor/interfaces/shared_bitmap_manager.mojom:9: // This
interface is used for allocating shared bitmap memory from browser
from the display compositor, instead of from the browser process?

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
components/display_compositor/interfaces/shared_bitmap_manager.mojom:10: //
process. For mus+ash, this service will live in mus process.
mus process is ambiguous, there is mus gpu and window server, iiuc. It just
lives where the display compositor lives right. So maybe just say that and leave
out specific process?

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
components/display_compositor/interfaces/shared_bitmap_manager.mojom:15: //
bitmap before the registration message below made it to the browser).
to the display compositor?

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
components/display_compositor/interfaces/shared_bitmap_manager.mojom:17: //
Informs the browser that the child allocated a shared bitmap.
should these say display compositor instead of browser too?

https://codereview.chromium.org/2717213004/diff/40001/content/child/blob_stor...
File content/child/blob_storage/webblobregistry_impl.cc (right):

https://codereview.chromium.org/2717213004/diff/40001/content/child/blob_stor...
content/child/blob_storage/webblobregistry_impl.cc:156:
display_compositor::ChildSharedBitmapManager::AllocateSharedMemory(
I don't think this is right, because this method has nothing to do with
SharedBitmaps. Either:

a) The ChildSharedBitmapManager should be somewhere else, or
b) This method should be duplicated in to ChildSharedBitmapManager (privately)
and somewhere for this to use, or
c) This method should exist where this can use it (ChildThreadImpl maybe), and
be passed in to ChildSharedBitmapManager as a function pointer?

Maybe jam@ has some opinions.

Putting the Host stuff in display_compositor seems good.

The Child stuff in display_compositor/child seems okay, but seeing as it
allocates thru mojo, maybe it should just be in cc/surfaces/? That is where
other things used by all embedders of display_compositor clients live, that
don't depend on stuff in content, which it appears to be the case now. And the
display_compositor/interfaces/ could be in cc/ipc/ then? wdyt?

[danakj, 2017-02-28 20:44:30]

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

[danakj, 2017-02-28 20:44:31]

TBH the HostSharedBitmapManager looks like it could live in cc/surfaces/ too, it
doesnt depend on content things anymore. I wonder if we should be
updating/pruning this API more now that we're using mojo shared memory. +jbauman
for thoughts

[xlai (Olivia), 2017-03-01 20:02:45]

Not changing the CL further whilst I'm waiting for more opinions about the
better way of placing HostSharedBitmapManager and ChildSharedBitmapManager.

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
File components/display_compositor/child/BUILD.gn (right):

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
components/display_compositor/child/BUILD.gn:10: defines = [
"DISPLAY_COMPOSITOR_IMPLEMENTATION" ]
On 2017/02/28 20:34:56, danakj wrote:
> separate components should have separate IMPLEMENTATION and export macros

This is not a separate implementation. It still belongs to display_compositor,
just like //components/discardable_memory/ having separate folders for renderer
and browser respectively.

https://codereview.chromium.org/2717213004/diff/40001/content/child/blob_stor...
File content/child/blob_storage/webblobregistry_impl.cc (right):

https://codereview.chromium.org/2717213004/diff/40001/content/child/blob_stor...
content/child/blob_storage/webblobregistry_impl.cc:156:
display_compositor::ChildSharedBitmapManager::AllocateSharedMemory(
On 2017/02/28 20:34:57, danakj wrote:
> I don't think this is right, because this method has nothing to do with
> SharedBitmaps. Either:
> 
> a) The ChildSharedBitmapManager should be somewhere else, or
> b) This method should be duplicated in to ChildSharedBitmapManager (privately)
> and somewhere for this to use, or
> c) This method should exist where this can use it (ChildThreadImpl maybe), and
> be passed in to ChildSharedBitmapManager as a function pointer?
> 
> Maybe jam@ has some opinions.
> 
> Putting the Host stuff in display_compositor seems good.
> 
> The Child stuff in display_compositor/child seems okay, but seeing as it
> allocates thru mojo, maybe it should just be in cc/surfaces/? That is where
> other things used by all embedders of display_compositor clients live, that
> don't depend on stuff in content, which it appears to be the case now. And the
> display_compositor/interfaces/ could be in cc/ipc/ then? wdyt?

This AllocateSharedMemory is currently attached in ChildThreadImpl and
childsharedbitmapmanager uses it. Having a duplicate private
method in ChildSharedBitmapManager looks a fine approach. Or, putting the whole
chain of childSharedBitmapManager in cc/surfaces looks fine too, just need to
introduce build/build_config.h
to cc/.

I personally prefer the first approach as most users of ChildSharedBitmapManager
lies in content/child and content/renderer and this seems like just another
symmetric example as 
//components/discardable_memory/client. WDYT?

[jbauman, 2017-03-02 23:20:43]

On 2017/02/28 20:44:31, danakj wrote:
> TBH the HostSharedBitmapManager looks like it could live in cc/surfaces/ too,
it
> doesnt depend on content things anymore. I wonder if we should be
> updating/pruning this API more now that we're using mojo shared memory.
+jbauman
> for thoughts

The actual SharedBitmapManager API still makes sense, as we need some way to
generate SharedBitmapIds and associate them cross-process. But the
ChildSharedBitmapManager could maybe be changed to use MojoCreateSharedBuffer if
that works in the sandbox. Then we wouldn't need to use AllocateSharedMemory
here.

[danakj, 2017-03-03 19:21:55]

fsamuel is also thinking about how to position client/service code with may or
may not be related to this CL fwiw.

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
File components/display_compositor/child/BUILD.gn (right):

https://codereview.chromium.org/2717213004/diff/40001/components/display_comp...
components/display_compositor/child/BUILD.gn:10: defines = [
"DISPLAY_COMPOSITOR_IMPLEMENTATION" ]
On 2017/03/01 20:02:45, xlai (Olivia) wrote:
> On 2017/02/28 20:34:56, danakj wrote:
> > separate components should have separate IMPLEMENTATION and export macros
> 
> This is not a separate implementation. It still belongs to display_compositor,
> just like //components/discardable_memory/ having separate folders for
renderer
> and browser respectively.

That makes me question this a bit more then. //components/display_compositor is
the cc embedder for the display compositor. But the child is not part of the
display compositor so it should not be in that component.

https://codereview.chromium.org/2717213004/diff/40001/content/child/blob_stor...
File content/child/blob_storage/webblobregistry_impl.cc (right):

https://codereview.chromium.org/2717213004/diff/40001/content/child/blob_stor...
content/child/blob_storage/webblobregistry_impl.cc:156:
display_compositor::ChildSharedBitmapManager::AllocateSharedMemory(
On 2017/03/01 20:02:45, xlai (Olivia) wrote:
> On 2017/02/28 20:34:57, danakj wrote:
> > I don't think this is right, because this method has nothing to do with
> > SharedBitmaps. Either:
> > 
> > a) The ChildSharedBitmapManager should be somewhere else, or
> > b) This method should be duplicated in to ChildSharedBitmapManager
(privately)
> > and somewhere for this to use, or
> > c) This method should exist where this can use it (ChildThreadImpl maybe),
and
> > be passed in to ChildSharedBitmapManager as a function pointer?
> > 
> > Maybe jam@ has some opinions.
> > 
> > Putting the Host stuff in display_compositor seems good.
> > 
> > The Child stuff in display_compositor/child seems okay, but seeing as it
> > allocates thru mojo, maybe it should just be in cc/surfaces/? That is where
> > other things used by all embedders of display_compositor clients live, that
> > don't depend on stuff in content, which it appears to be the case now. And
the
> > display_compositor/interfaces/ could be in cc/ipc/ then? wdyt?
> 
> This AllocateSharedMemory is currently attached in ChildThreadImpl and
> childsharedbitmapmanager uses it. Having a duplicate private
> method in ChildSharedBitmapManager looks a fine approach. Or, putting the
whole
> chain of childSharedBitmapManager in cc/surfaces looks fine too, just need to
> introduce build/build_config.h
> to cc/.
> 
> I personally prefer the first approach as most users of
ChildSharedBitmapManager
> lies in content/child and content/renderer and this seems like just another
> symmetric example as 
> //components/discardable_memory/client. WDYT?
> 

I don't think either is the right final state. cc/surfaces/ is a bit of a mix of
client and service side and we can improve that. But I don't think we should
have client stuff inside the display_compositor component, as that duplicates
the cc/surfaces problem.

John's suggestion of MojoCreateSharedBuffer() would avoid needing to duplicate
or inject AllocateSharedMemory into cc/. Though I'm not totally sure how that's
better than using mojo::SharedBufferHandle::Create tbh.

[jbauman, 2017-03-03 22:21:33]

On 2017/03/03 19:21:55, danakj wrote:
> 
> I don't think either is the right final state. cc/surfaces/ is a bit of a mix
of
> client and service side and we can improve that. But I don't think we should
> have client stuff inside the display_compositor component, as that duplicates
> the cc/surfaces problem.
> 
> John's suggestion of MojoCreateSharedBuffer() would avoid needing to duplicate
> or inject AllocateSharedMemory into cc/. Though I'm not totally sure how
that's
> better than using mojo::SharedBufferHandle::Create tbh.

That probably works too.

[danakj, 2017-03-03 22:47:20]

On Fri, Mar 3, 2017 at 5:21 PM, <jbauman@chromium.org> wrote:

> On 2017/03/03 19:21:55, danakj wrote:
> >
> > I don't think either is the right final state. cc/surfaces/ is a bit of
> a mix
> of
> > client and service side and we can improve that. But I don't think we
> should
> > have client stuff inside the display_compositor component, as that
> duplicates
> > the cc/surfaces problem.
> >
> > John's suggestion of MojoCreateSharedBuffer() would avoid needing to
> duplicate
> > or inject AllocateSharedMemory into cc/. Though I'm not totally sure how
> that's
> > better than using mojo::SharedBufferHandle::Create tbh.
>
> That probably works too.
>

Oh ok cool, that's what it's doing already apparently. :)

-- 
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.

[xlai (Olivia), 2017-03-03 22:49:48]

On 2017/03/03 22:21:33, jbauman wrote:
> On 2017/03/03 19:21:55, danakj wrote:
> > 
> > I don't think either is the right final state. cc/surfaces/ is a bit of a
mix
> of
> > client and service side and we can improve that. But I don't think we should
> > have client stuff inside the display_compositor component, as that
duplicates
> > the cc/surfaces problem.
> > 
> > John's suggestion of MojoCreateSharedBuffer() would avoid needing to
duplicate
> > or inject AllocateSharedMemory into cc/. Though I'm not totally sure how
> that's
> > better than using mojo::SharedBufferHandle::Create tbh.
> 
> That probably works too.

I tried to summarize what we've come so far:

1. No change to ChildThreadImpl::AllocateSharedMemory and this function's other
users.

2. Use mojo::SharedBufferHandle::Create (which calls MojoCreateSharedBuffer())
in
ChildSharedBitmapManager to replace its call to
ChildThreadImpl::AllocateSharedMemory.

3. Shifts ChildSharedBitmapManager to cc/surfaces/ (or cc/resources/?), the IPC
it's using
should be shifted to cc/ipc/.

4. HostSharedBitmapManager remains in components/display_compositor as the
current
arrangement.

danakj@, jbauman@: Let me know if you get this correct.

[danakj, 2017-03-03 23:41:58]

On Fri, Mar 3, 2017 at 5:49 PM, <xlai@chromium.org> wrote:

> On 2017/03/03 22:21:33, jbauman wrote:
> > On 2017/03/03 19:21:55, danakj wrote:
> > >
> > > I don't think either is the right final state. cc/surfaces/ is a bit
> of a
> mix
> > of
> > > client and service side and we can improve that. But I don't think we
> should
> > > have client stuff inside the display_compositor component, as that
> duplicates
> > > the cc/surfaces problem.
> > >
> > > John's suggestion of MojoCreateSharedBuffer() would avoid needing to
> duplicate
> > > or inject AllocateSharedMemory into cc/. Though I'm not totally sure
> how
> > that's
> > > better than using mojo::SharedBufferHandle::Create tbh.
> >
> > That probably works too.
>
> I tried to summarize what we've come so far:
>
> 1. No change to ChildThreadImpl::AllocateSharedMemory and this function's
> other
> users.
>

Yep.

2. Use mojo::SharedBufferHandle::Create (which calls
> MojoCreateSharedBuffer())
> in
> ChildSharedBitmapManager to replace its call to
> ChildThreadImpl::AllocateSharedMemory.
>

This is good.


>
> 3. Shifts ChildSharedBitmapManager to cc/surfaces/ (or cc/resources/?),
> the IPC
> it's using
> should be shifted to cc/ipc/.
>

Oh the child is using the new mojom. That going in cc/ipc/ seems good like
other display compositor mojoms are such as display_compositor.mojom.

However //cc can not use //cc/ipc, the deps goes the other way. And I see
the child is the one using that mojom, so the child can't go in the //cc
target. It looks like general code for embedders that wants to be
content-agnostic is currently in /services/ui/public/cpp/ so that looks
like the place for the ChildSharedBitmapManager for now.

>
> 4. HostSharedBitmapManager remains in components/display_compositor as the
> current
> arrangement.
>

Yep.


>
> danakj@, jbauman@: Let me know if you get this correct.
>
> https://codereview.chromium.org/2717213004/
>

-- 
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.

[xlai (Olivia), 2017-03-07 19:41:57]

Description was changed from

==========
Move SharedBitmap implementation from content/ to components/

HostSharedBitmapManager is shifted to components/display_compositor.
ChildSharedBitmapManager is shifted to components/display_compositor/client.

The Mojo messages used in allocating shared bitmap is decoupled from
render_message_filter.h and moved to components/display_compositor/interfaces.

The static method of ChildThreadImpl::AllocateSharedMemory is shifted to
the new ChildSharedBitmapManager.

BUG=670162
==========

to

==========
Move SharedBitmap implementation from content/ to components/

HostSharedBitmapManager is shifted to components/display_compositor.
ChildSharedBitmapManager is shifted to components/display_compositor/client.

The Mojo messages used in allocating shared bitmap is decoupled from
render_message_filter.h and moved to components/display_compositor/interfaces.

The static method of ChildThreadImpl::AllocateSharedMemory is shifted to
the new ChildSharedBitmapManager.

BUG=670162
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[xlai (Olivia), 2017-03-07 19:42:11]

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

[commit-bot: I haz the power, 2017-03-07 19:43:16]

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-03-07 20:12:20]

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

[commit-bot: I haz the power, 2017-03-07 20:12:21]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[xlai (Olivia), 2017-03-07 21:20:30]

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

[commit-bot: I haz the power, 2017-03-07 21:21: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-03-07 22:11:40]

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

[commit-bot: I haz the power, 2017-03-07 22:11:41]

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

[xlai (Olivia), 2017-03-13 13:50:09]

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

[commit-bot: I haz the power, 2017-03-13 13:50:27]

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-03-13 13:53:00]

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

[commit-bot: I haz the power, 2017-03-13 13:53:01]

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

[xlai (Olivia), 2017-03-13 14:26:39]

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

[commit-bot: I haz the power, 2017-03-13 14:26:54]

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-03-13 14:33:20]

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

[commit-bot: I haz the power, 2017-03-13 14:33:21]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[xlai (Olivia), 2017-03-13 14:38:28]

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

[commit-bot: I haz the power, 2017-03-13 14:38:52]

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-03-13 15:18:17]

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

[commit-bot: I haz the power, 2017-03-13 15:18:18]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[xlai (Olivia), 2017-03-13 15:44:12]

xlai@chromium.org changed reviewers:
+ fsamuel@chromium.org

[xlai (Olivia), 2017-03-13 15:44:13]

+fsamuel@ to reviewers.

I followed danakj@'s suggestion to move ChildSharedBitmapManager to
//services/ui/public/cpp/bitmap/
and the mojom interface to cc/ipc/shared_bitmap_manager.mojom. 

This refactoring is not supposed to cause behavioral change; however, it is now
failing all the tests that
need to use SharedBitmapManager. I spent a lot of time debugging this last week
but couldn't figure it out.
I believe that I miss something important in the mojo channel setup when I move
the mojom interface out
of render_message_filter.mojom. 

Currently, on the browser side I do the "registry->AddInterface" in
render_process_host_impl.cc to create 
a strong binding of the impl class of the new mojom interface, just like what
discardable_memory does. 
On the renderer side, I obtain the interface pointer by doing the same thing as
before, i.e. using "channel()->GetThreadSafeRemoteAssociatedInterface" to get an
associated pointer of the mojom interface in 
render_thread_impl.cc. I am speculating that the way I bind the Mojo pointers
were wrong.

I've tried adding the new mojom interface to some manifest file; but that
doesn't work. The errors come from
all places, such as a segmentation fault of
RenderWidgetHostViewBrowserTest::GetRenderWidgetHost() in browser
tests or simply a error message of "render_process_host_impl.cc(2109): Request
for unknown Channel-associated
interface: cc::mojom::SharedBitmapManager" in gpu pixel tests.

[xlai (Olivia), 2017-04-04 19:40:15]

Patchset #9 (id:160001) has been deleted

[xlai (Olivia), 2017-04-04 21:05:50]

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

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

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xlai (Olivia), 2017-04-04 21:28:27]

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

[commit-bot: I haz the power, 2017-04-04 21:28:39]

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

[commit-bot: I haz the power, 2017-04-04 21:29:08]

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-04-04 23:06:43]

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

[commit-bot: I haz the power, 2017-04-04 23:06:45]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[xlai (Olivia), 2017-04-05 15:33:52]

I've resumed working on this CL. Basically, the previous problem I had was about
the ordering problem of the two mojo functions I've moved out of 
render_message_filter.mojom into shared_bitmap_manager.mojom. Request of bitmap
allocation of CC may come before the registration message to browser.

Now I make SharedBitmapManager interface to be associated mojo interface of 
RenderMessageFilter and this guarantees the order of bitmap allocation.

I pass in the associated interface pointer to ChildSharedBitmapManager. Note
that
it is thread-safe because the caller is from impl thread in renderer (different
thread).

This time it passed all trybots except ContentViewPopupZoomerTest in
content_shell_test_apk in android_n5x_swarming_rel. I'm still investigating why.

[jbauman, 2017-04-05 21:37:28]

https://codereview.chromium.org/2717213004/diff/240001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2717213004/diff/240001/content/renderer/rende...
content/renderer/render_thread_impl.cc:635:
cc::mojom::ThreadSafeSharedBitmapManagerAssociatedPtr::Create(
I'm not sure how this works, but I think we need to call ::Create with the IPC
task runner as well.

[xlai (Olivia), 2017-04-05 21:41:45]

https://codereview.chromium.org/2717213004/diff/240001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2717213004/diff/240001/content/renderer/rende...
content/renderer/render_thread_impl.cc:635:
cc::mojom::ThreadSafeSharedBitmapManagerAssociatedPtr::Create(
On 2017/04/05 21:37:28, jbauman wrote:
> I'm not sure how this works, but I think we need to call ::Create with the IPC
> task runner as well.

Why do we need IPC task runner? 

This line is converting the associated interface pointer to a thread-safe
interface pointer so that impl thread can use it when they invoke function from
ChildSharedBitmapManager. The associated interface pointer of
SharedBitmapManager is set on top of the RenderMessageFilter interface via the
GetSharedBitmapManager(MojoAssociatedRequest) message. It guarantees the FIFO
order of allocation of shared bitmap.

[jbauman, 2017-04-05 21:54:20]

On 2017/04/05 21:41:45, xlai (Olivia) wrote:
>
https://codereview.chromium.org/2717213004/diff/240001/content/renderer/rende...
> File content/renderer/render_thread_impl.cc (right):
> 
>
https://codereview.chromium.org/2717213004/diff/240001/content/renderer/rende...
> content/renderer/render_thread_impl.cc:635:
> cc::mojom::ThreadSafeSharedBitmapManagerAssociatedPtr::Create(
> On 2017/04/05 21:37:28, jbauman wrote:
> > I'm not sure how this works, but I think we need to call ::Create with the
IPC
> > task runner as well.
> 
> Why do we need IPC task runner? 
> 
> This line is converting the associated interface pointer to a thread-safe
> interface pointer so that impl thread can use it when they invoke function
from
> ChildSharedBitmapManager. The associated interface pointer of
> SharedBitmapManager is set on top of the RenderMessageFilter interface via the
> GetSharedBitmapManager(MojoAssociatedRequest) message. It guarantees the FIFO
> order of allocation of shared bitmap.

Using the ipc task runner would match what we currently do in
IPC::Channel::GetThreadSafeRemoteAssociatedInterface. AIUI thread-safe
associated interface pointers are implemented by posting tasks to a specific
thread to actually send the message. By default that's the thread that created
them. In this case that's the main thread, so messages from the impl thread
could be stuck for a long time behind tasks there. If it's the IO thread, then
that wouldn't be an issue (I'm still not sure about the precise ordering,
though).

[xlai (Olivia), 2017-04-05 23:06:33]

Description was changed from

==========
Move SharedBitmap implementation from content/ to components/

HostSharedBitmapManager is shifted to components/display_compositor.
ChildSharedBitmapManager is shifted to components/display_compositor/client.

The Mojo messages used in allocating shared bitmap is decoupled from
render_message_filter.h and moved to components/display_compositor/interfaces.

The static method of ChildThreadImpl::AllocateSharedMemory is shifted to
the new ChildSharedBitmapManager.

BUG=670162
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Move SharedBitmapManager implementation out of content/

HostSharedBitmapManager is shifted to components/display_compositor.
ChildSharedBitmapManager is shifted to services/ui/public/cpp/bitmap.

The Mojo messages used in allocating shared bitmap is decoupled from
render_message_filter.h and moved to cc/ipc/shared_bitmap_manager.mojom.

Note that the SharedBitmapManager is an associated mojo interface of
RenderMessageFilter so as to maintain the existing FIFO order of mojo
messages between the two SharedBitmap allocation functions and the
rest of RenderMessageFilter functions.

BUG=670162
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[xlai (Olivia), 2017-04-05 23:07:06]

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

[commit-bot: I haz the power, 2017-04-05 23:07:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xlai (Olivia), 2017-04-05 23:10:39]

On 2017/04/05 21:54:20, jbauman wrote:
> On 2017/04/05 21:41:45, xlai (Olivia) wrote:
> >
>
https://codereview.chromium.org/2717213004/diff/240001/content/renderer/rende...
> > File content/renderer/render_thread_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2717213004/diff/240001/content/renderer/rende...
> > content/renderer/render_thread_impl.cc:635:
> > cc::mojom::ThreadSafeSharedBitmapManagerAssociatedPtr::Create(
> > On 2017/04/05 21:37:28, jbauman wrote:
> > > I'm not sure how this works, but I think we need to call ::Create with the
> IPC
> > > task runner as well.
> > 
> > Why do we need IPC task runner? 
> > 
> > This line is converting the associated interface pointer to a thread-safe
> > interface pointer so that impl thread can use it when they invoke function
> from
> > ChildSharedBitmapManager. The associated interface pointer of
> > SharedBitmapManager is set on top of the RenderMessageFilter interface via
the
> > GetSharedBitmapManager(MojoAssociatedRequest) message. It guarantees the
FIFO
> > order of allocation of shared bitmap.
> 
> Using the ipc task runner would match what we currently do in
> IPC::Channel::GetThreadSafeRemoteAssociatedInterface. AIUI thread-safe
> associated interface pointers are implemented by posting tasks to a specific
> thread to actually send the message. By default that's the thread that created
> them. In this case that's the main thread, so messages from the impl thread
> could be stuck for a long time behind tasks there. If it's the IO thread, then
> that wouldn't be an issue (I'm still not sure about the precise ordering,
> though).

Thanks a lot for the explanation. I added the ipc task runner here and this
fixed
the two failed tests (at least in my local test on an Android device) on
Android's
 Disambugation Popup feature, which was previously caused by the IPC/Mojo
messages
order problem due to thread difference.

[xlai (Olivia), 2017-04-05 23:14:13]

The CQ bit was unchecked by xlai@chromium.org

[xlai (Olivia), 2017-04-06 14:11:52]

xlai@chromium.org changed reviewers:
+ tsepez@chromium.org
- jam@chromium.org

[xlai (Olivia), 2017-04-06 14:14:28]

xlai@chromium.org changed reviewers:
+ jam@chromium.org

[xlai (Olivia), 2017-04-06 14:20:51]

This CL is ready to be reviewed, after rounds of discussion
among reviewers to find the new home for content::ChildSharedBitmapManager and
content::HostSharedBitmapManager.

Gentle ping to danakj@, fsamuel@, jbauman@ about display_compositor/ and cc/ as
well as the general change of 
SharedBitmapManager. Also ping to tsepez@ about the IPC change. Also ping to
jam@ about changes in content/ and services/ui/

[danakj, 2017-04-06 15:00:51]

LGTM

https://codereview.chromium.org/2717213004/diff/260001/cc/ipc/shared_bitmap_m...
File cc/ipc/shared_bitmap_manager.mojom (right):

https://codereview.chromium.org/2717213004/diff/260001/cc/ipc/shared_bitmap_m...
cc/ipc/shared_bitmap_manager.mojom:9: // This interface is used for allocating
shared bitmap memory from display
I think this can be reworded a bit. The API doesn't actually allocate anything.
Instead, "This interface is used when allocating shared bitmap memory to be
shared with a display compositor."

https://codereview.chromium.org/2717213004/diff/260001/cc/ipc/shared_bitmap_m...
cc/ipc/shared_bitmap_manager.mojom:10: // compositor. For mus+ash, this service
will live in where the display
And then I would drop the "For mus+ash..." sentence.

[Fady Samuel, 2017-04-06 15:05:12]

Thank you for following through on this, Olivia! This is great! LGTM

[Tom Sepez, 2017-04-06 16:44:04]

LGTM.

https://codereview.chromium.org/2717213004/diff/260001/cc/ipc/shared_bitmap_m...
File cc/ipc/shared_bitmap_manager.mojom (right):

https://codereview.chromium.org/2717213004/diff/260001/cc/ipc/shared_bitmap_m...
cc/ipc/shared_bitmap_manager.mojom:18:
AllocatedSharedBitmap(handle<shared_buffer> buffer, gpu.mojom.Mailbox id);
nit: maybe call this DidAllocateSharedBitmap().  Or not.

https://codereview.chromium.org/2717213004/diff/260001/cc/ipc/shared_bitmap_m...
cc/ipc/shared_bitmap_manager.mojom:21: DeletedSharedBitmap(gpu.mojom.Mailbox
id);
nit: maybe call this WillDeleteSharedBitmap().  This happens just prior to
deletion, right?

[xlai (Olivia), 2017-04-06 17:03:47]

xlai@chromium.org changed reviewers:
+ piman@chromium.org, sky@chromium.org

[xlai (Olivia), 2017-04-06 17:03:48]

As jam@ is OOO, add sky@ and piman@ as reviewers.

sky@: Could you please review changes in services/ui? 
piman@: Could you please review changes in content/?

Thanks.

[sky, 2017-04-06 20:03:12]

https://codereview.chromium.org/2717213004/diff/260001/services/ui/public/cpp...
File services/ui/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2717213004/diff/260001/services/ui/public/cpp...
services/ui/public/cpp/BUILD.gn:24: "//services/ui/public/cpp/bitmap",
Do all users of services/ui/public need the bitmap files? More specifically, can
you remove this dep from here and have clients that need it add a specific dep.

[sky, 2017-04-06 20:03:46]

I'm wondering if the bitmap files you're moving to services/ui should be moved
to cc?

[Fady Samuel, 2017-04-06 20:04:43]

On 2017/04/06 20:03:46, sky wrote:
> I'm wondering if the bitmap files you're moving to services/ui should be moved
> to cc?

I think we'll be moving them to viz eventually.

[xlai (Olivia), 2017-04-06 20:42:27]

https://codereview.chromium.org/2717213004/diff/260001/cc/ipc/shared_bitmap_m...
File cc/ipc/shared_bitmap_manager.mojom (right):

https://codereview.chromium.org/2717213004/diff/260001/cc/ipc/shared_bitmap_m...
cc/ipc/shared_bitmap_manager.mojom:9: // This interface is used for allocating
shared bitmap memory from display
On 2017/04/06 15:00:51, danakj wrote:
> I think this can be reworded a bit. The API doesn't actually allocate
anything.
> Instead, "This interface is used when allocating shared bitmap memory to be
> shared with a display compositor."

Done here and elsewhere.

https://codereview.chromium.org/2717213004/diff/260001/cc/ipc/shared_bitmap_m...
cc/ipc/shared_bitmap_manager.mojom:21: DeletedSharedBitmap(gpu.mojom.Mailbox
id);
On 2017/04/06 16:44:04, Tom Sepez wrote:
> nit: maybe call this WillDeleteSharedBitmap().  This happens just prior to
> deletion, right?

Done here and elsewhere. I think this should be DidDeleteSharedBitmap because it
occurs when the shared bitmap deletion is confirmed (in destructor); also it is
consistent with DidAllocateSharedBitmap.

https://codereview.chromium.org/2717213004/diff/260001/services/ui/public/cpp...
File services/ui/public/cpp/BUILD.gn (right):

https://codereview.chromium.org/2717213004/diff/260001/services/ui/public/cpp...
services/ui/public/cpp/BUILD.gn:24: "//services/ui/public/cpp/bitmap",
On 2017/04/06 20:03:12, sky wrote:
> Do all users of services/ui/public need the bitmap files? More specifically,
can
> you remove this dep from here and have clients that need it add a specific
dep.

This was a mistake. I've removed this line.

[piman, 2017-04-06 21:01:33]

lgtm

[jbauman, 2017-04-06 21:17:31]

lgtm

[sky, 2017-04-06 21:47:08]

Is there a reason not to put them in viz now?

On Thu, Apr 6, 2017 at 1:04 PM, <fsamuel@chromium.org> wrote:

> On 2017/04/06 20:03:46, sky wrote:
> > I'm wondering if the bitmap files you're moving to services/ui should be
> moved
> > to cc?
>
> I think we'll be moving them to viz eventually.
>
> https://codereview.chromium.org/2717213004/
>

-- 
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.

[danakj, 2017-04-07 15:02:11]

On Thu, Apr 6, 2017 at 5:47 PM, Scott Violet <sky@chromium.org> wrote:

> Is there a reason not to put them in viz now?
>

I'd say mostly because this CL moves us in a good direction, even if it's
not 100% where we will end up (I'm not exactly sure where this will end up
in viz if we do, or if we can rethink this API instead under mojo). I think
we should move forward with this.


>
> On Thu, Apr 6, 2017 at 1:04 PM, <fsamuel@chromium.org> wrote:
>
>> On 2017/04/06 20:03:46, sky wrote:
>> > I'm wondering if the bitmap files you're moving to services/ui should
>> be moved
>> > to cc?
>>
>> I think we'll be moving them to viz eventually.
>>
>> https://codereview.chromium.org/2717213004/
>>
>
>

-- 
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.

[sky, 2017-04-07 15:21:50]

Ok, LGTM

[xlai (Olivia), 2017-04-07 17:26:10]

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

[commit-bot: I haz the power, 2017-04-07 17:26:56]

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-04-07 17:30:20]

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

[commit-bot: I haz the power, 2017-04-07 17:30:21]

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-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...)

[xlai (Olivia), 2017-04-07 17:49:26]

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

[commit-bot: I haz the power, 2017-04-07 17:50:06]

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-04-07 19:54:40]

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

[commit-bot: I haz the power, 2017-04-07 19:54:42]

Dry run: This issue passed the CQ dry run.

[xlai (Olivia), 2017-04-07 20:03:29]

The CQ bit was checked by xlai@chromium.org

[xlai (Olivia), 2017-04-07 20:03:30]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
tsepez@chromium.org, fsamuel@chromium.org, sky@chromium.org, piman@chromium.org,
jbauman@chromium.org
Link to the patchset: https://codereview.chromium.org/2717213004/#ps300001
(title: "rebase")

[commit-bot: I haz the power, 2017-04-07 20:04:23]

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-04-07 20:13:23]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1491595409703180,
"parent_rev": "9f99a6f415f9a0caaae84ed0a768b8c63e3fceb3", "commit_rev":
"11ac780e68cd3fde64d7eac350f4ebf53ce75eea"}

[commit-bot: I haz the power, 2017-04-07 20:14:15]

Description was changed from

==========
Move SharedBitmapManager implementation out of content/

HostSharedBitmapManager is shifted to components/display_compositor.
ChildSharedBitmapManager is shifted to services/ui/public/cpp/bitmap.

The Mojo messages used in allocating shared bitmap is decoupled from
render_message_filter.h and moved to cc/ipc/shared_bitmap_manager.mojom.

Note that the SharedBitmapManager is an associated mojo interface of
RenderMessageFilter so as to maintain the existing FIFO order of mojo
messages between the two SharedBitmap allocation functions and the
rest of RenderMessageFilter functions.

BUG=670162
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Move SharedBitmapManager implementation out of content/

HostSharedBitmapManager is shifted to components/display_compositor.
ChildSharedBitmapManager is shifted to services/ui/public/cpp/bitmap.

The Mojo messages used in allocating shared bitmap is decoupled from
render_message_filter.h and moved to cc/ipc/shared_bitmap_manager.mojom.

Note that the SharedBitmapManager is an associated mojo interface of
RenderMessageFilter so as to maintain the existing FIFO order of mojo
messages between the two SharedBitmap allocation functions and the
rest of RenderMessageFilter functions.

BUG=670162
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2717213004
Cr-Commit-Position: refs/heads/master@{#462979}
Committed:
https://chromium.googlesource.com/chromium/src/+/11ac780e68cd3fde64d7eac350f4...
==========

[commit-bot: I haz the power, 2017-04-07 20:14:17]

Committed patchset #15 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/11ac780e68cd3fde64d7eac350f4...