Move Mus into chrome's process when running with --mus.

Move Mus into browser process when running with --mus.

Moves Mus into browser process when running Mustash (chrome --mus).
Window Server is running on its own thread with ThreadPriority::DISPLAY when
running in browser process.

For now both WS and GPU are moved. Once WS/GPU split if finished, GPU will need
to be moved into its separate process.

A lot of this CL is fixing up various singletons to work when Mus is running
in-process. For example, CursorFactoryOzone and InputDeviceManage are made
thread-local instead of global singletons, because both Mus and ash/chrome
need an instance.

Another significant aspect of this CL is making cursors work when Mus is
running in-process. Cursor manipulation requires loading resources from a
ResourceBundle, which can only happen on the browser thread. As such, Mus
creates ui::ImageCursors objects for its displays, but the ownership is
then passed to the browser thread, and all manipulation of these objects
is subsequently also done on the browser thread via Mus posting tasks.

BUG=722527

[mfomitchev, 2017-06-01 03:18:18]

Description was changed from

==========
WIP: Move Mus into chrome's process when running with --mus.

BUG=722527
==========

to

==========
WIP: Move Mus into chrome's process when running with --mus.

There is a few open issues. Please see the doc for details:
https://docs.google.com/document/d/1EqVUW4H7vNikTd4M9KrTVYIpviVuwktq32WTmc0V6...
Questionable parts of the new code are marked with TODOs.

BUG=722527
==========

[mfomitchev, 2017-06-01 03:19:10]

mfomitchev@chromium.org changed reviewers:
+ sadrul@chromium.org, sky@chromium.org

[mfomitchev, 2017-06-01 03:22:51]

Description was changed from

==========
WIP: Move Mus into chrome's process when running with --mus.

There is a few open issues. Please see the doc for details:
https://docs.google.com/document/d/1EqVUW4H7vNikTd4M9KrTVYIpviVuwktq32WTmc0V6...
Questionable parts of the new code are marked with TODOs.

BUG=722527
==========

to

==========
WIP: Move Mus into chrome's process when running with --mus.

There is a few open issues. Please see the doc for details:
https://docs.google.com/document/d/1EqVUW4H7vNikTd4M9KrTVYIpviVuwktq32WTmc0V6...
Questionable parts of the new code are marked with TODOs.

I'd like to confirm if I am on the right track. Advice on the outstanding issues
would also be appreciated!

BUG=722527
==========

[sky, 2017-06-01 18:10:08]

I added comments to your doc. Let me know if you think it's worth still
looking at this until the doc is resolved.

On Wed, May 31, 2017 at 8:19 PM, <mfomitchev@chromium.org> wrote:

> Reviewers: sky, sadrul
> CL: https://codereview.chromium.org/2916823002/
>
> Description:
> WIP: Move Mus into chrome's process when running with --mus.
>
> There is a few open issues. Please see the doc for details:
> https://docs.google.com/document/d/1EqVUW4H7vNikTd4M9KrTVYIpviVuw
> ktq32WTmc0V6iU/edit#
>
<https://docs.google.com/document/d/1EqVUW4H7vNikTd4M9KrTVYIpviVuwktq32WTmc0V6...>
> Questionable parts of the new code are marked with TODOs.
>
> BUG=722527
>
> Affected files (+164, -50 lines):
> M chrome/app/BUILD.gn
> M chrome/app/mash/BUILD.gn
> M chrome/app/mash/embedded_services.cc
> M chrome/browser/DEPS
> M chrome/browser/chrome_content_browser_client.cc
> M chrome/browser/ui/views/chrome_browser_main_extra_parts_views.cc
> M content/browser/browser_main_loop.h
> M content/browser/browser_main_loop.cc
> M content/browser/browser_thread_impl.h
> M content/browser/browser_thread_impl.cc
> M content/public/browser/browser_thread.h
> M services/ui/display/screen_manager_forwarding.h
> M services/ui/display/screen_manager_forwarding.cc
> M services/ui/gpu/gpu_main.h
> M services/ui/gpu/gpu_main.cc
> M services/ui/main.cc
> M services/ui/service.h
> M services/ui/service.cc
> M services/ui/ws/platform_display_default.cc
> M services/ui/ws/user_display_manager.cc
>
>
>

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

[mfomitchev, 2017-06-19 19:13:36]

I uploaded a new patch. It still requires work, but can you PTAL at
services/ui/ws/threaded_image_cursors.* and the corresponding changes in
services/ui plumbing through the SingleThreadTaskRunner passed to the
ui::Service constructor and let me know if the approach looks good to you?
Thanks

[sky, 2017-06-19 21:24:37]

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

[sky, 2017-06-19 21:25:47]

+erg as he has been looking at cursors.

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h
File services/ui/service.h (right):

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h#n...
services/ui/service.h:70: Service(scoped_refptr<base::SingleThreadTaskRunner>
resource_runner);
Rather than having mus deal with the different embedding schemes could we
instead supply the set of cursors mus needs to the constructor? I'm thinking of
a key/value map of all the cursors it would need.

[Elliot Glaysher, 2017-06-19 21:46:43]

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h
File services/ui/service.h (right):

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h#n...
services/ui/service.h:70: Service(scoped_refptr<base::SingleThreadTaskRunner>
resource_runner);
On 2017/06/19 21:25:47, sky wrote:
> Rather than having mus deal with the different embedding schemes could we
> instead supply the set of cursors mus needs to the constructor? I'm thinking
of
> a key/value map of all the cursors it would need.

This sounds significantly better than having a wrapper proxy class.

Assuming your problems are with accessing the ResourceBundle, you could create
your own CursorLoader instance which takes the raw bitmaps key/value pair wise
as sky@ suggested. You'll need to lazily perform the scaling and rotation
yourself (see how cursor_loader_ozone.cc calls into cursor_util.cc, especially
GetImageCursorBitmap() and friends), but that should be entirely doable.

(If you're having threading problems with Ozone instead, that's going to be much
more difficult.)

[mfomitchev, 2017-06-19 21:50:20]

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h
File services/ui/service.h (right):

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h#n...
services/ui/service.h:70: Service(scoped_refptr<base::SingleThreadTaskRunner>
resource_runner);
On 2017/06/19 21:25:47, sky wrote:
> Rather than having mus deal with the different embedding schemes could we
> instead supply the set of cursors mus needs to the constructor? I'm thinking
of
> a key/value map of all the cursors it would need.

I considered this, but it would mean pre-loading and storing cursor resources
for all the possible display configurations (currently that just means scale
factors), which seems a bit wasteful. 
Additionally, we'd have to re-write cursor loaders for all the platforms to make
use of these pre-cached resources, and I thought it would be nice to avoid
tinkering with code in base/ too much. We could also write new resource loader
wrapper for Mus, but that's not much different from what we are doing here.

[sky, 2017-06-20 16:07:09]

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h
File services/ui/service.h (right):

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h#n...
services/ui/service.h:70: Service(scoped_refptr<base::SingleThreadTaskRunner>
resource_runner);
On 2017/06/19 21:50:19, mfomitchev wrote:
> On 2017/06/19 21:25:47, sky wrote:
> > Rather than having mus deal with the different embedding schemes could we
> > instead supply the set of cursors mus needs to the constructor? I'm thinking
> of
> > a key/value map of all the cursors it would need.
> 
> I considered this, but it would mean pre-loading and storing cursor resources
> for all the possible display configurations (currently that just means scale
> factors), which seems a bit wasteful. 
> Additionally, we'd have to re-write cursor loaders for all the platforms to
make
> use of these pre-cached resources, and I thought it would be nice to avoid
> tinkering with code in base/ too much. We could also write new resource loader
> wrapper for Mus, but that's not much different from what we are doing here.

I don't have a good feel for how many cursors and display configurations that
are supported. Is this on the order of tens of images, or hundreds? If it's
closer to hundreds, then I agree with you. If it's closer to 10-20(ish) then
preloading seems better.

[mfomitchev, 2017-06-20 17:00:42]

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h
File services/ui/service.h (right):

https://codereview.chromium.org/2916823002/diff/40001/services/ui/service.h#n...
services/ui/service.h:70: Service(scoped_refptr<base::SingleThreadTaskRunner>
resource_runner);
On 2017/06/20 16:07:09, sky wrote:
> On 2017/06/19 21:50:19, mfomitchev wrote:
> > On 2017/06/19 21:25:47, sky wrote:
> > > Rather than having mus deal with the different embedding schemes could we
> > > instead supply the set of cursors mus needs to the constructor? I'm
thinking
> > of
> > > a key/value map of all the cursors it would need.
> > 
> > I considered this, but it would mean pre-loading and storing cursor
resources
> > for all the possible display configurations (currently that just means scale
> > factors), which seems a bit wasteful. 
> > Additionally, we'd have to re-write cursor loaders for all the platforms to
> make
> > use of these pre-cached resources, and I thought it would be nice to avoid
> > tinkering with code in base/ too much. We could also write new resource
loader
> > wrapper for Mus, but that's not much different from what we are doing here.
> 
> I don't have a good feel for how many cursors and display configurations that
> are supported. Is this on the order of tens of images, or hundreds? If it's
> closer to hundreds, then I agree with you. If it's closer to 10-20(ish) then
> preloading seems better.


Ok. I think we have just two cursor sets right now - normal, and large. Each
contains 32 cursor bitmaps, plus there's 2 animated cursors which are currently
shared, but should be duplicated according to a comment.

Another potential concern with preloading is Windows. It doesn't have cursor
load functions implemented (CursorLoaderWin::LoadImageCursor), it only loads
cursors when they are set - I am not sure if there is some subtle reason for
this, or this was just an accidental omission. Do you happen to know?

If we go the preload route, I think the way to go would be to teach cursor code
in base to preload all cursor resources and operate on preloaded resources
rather than operating on the resource bundle. Does this sound reasonable? The
existing code loads cursor bitmaps, and then scales/rotates them in the same
function, e.g. see GetImageCursorBitmap. I think we'd have to break that apart
so that we don't need to duplicate code. 

I am thinking we can change the existing Cursor Loaders (ozone, win, and x11) to
support preloading cursors for all display configurations and then using that.
We can then pass a cursor loader instance to the UI Service constructor.

[sky, 2017-06-20 19:15:04]

Elliot and myself spoke about this and feel that this isn't a lot of data,
the images aren't big and windows is rather out of scope at the moment so
we should make a copy given it simplifies things. Sound good?

  -Scott

On Tue, Jun 20, 2017 at 10:00 AM, <mfomitchev@chromium.org> wrote:

>
> https://codereview.chromium.org/2916823002/diff/40001/
> services/ui/service.h
> File services/ui/service.h (right):
>
> https://codereview.chromium.org/2916823002/diff/40001/
> services/ui/service.h#newcode70
> services/ui/service.h:70:
> Service(scoped_refptr<base::SingleThreadTaskRunner> resource_runner);
> On 2017/06/20 16:07:09, sky wrote:
> > On 2017/06/19 21:50:19, mfomitchev wrote:
> > > On 2017/06/19 21:25:47, sky wrote:
> > > > Rather than having mus deal with the different embedding schemes
> could we
> > > > instead supply the set of cursors mus needs to the constructor?
> I'm thinking
> > > of
> > > > a key/value map of all the cursors it would need.
> > >
> > > I considered this, but it would mean pre-loading and storing cursor
> resources
> > > for all the possible display configurations (currently that just
> means scale
> > > factors), which seems a bit wasteful.
> > > Additionally, we'd have to re-write cursor loaders for all the
> platforms to
> > make
> > > use of these pre-cached resources, and I thought it would be nice to
> avoid
> > > tinkering with code in base/ too much. We could also write new
> resource loader
> > > wrapper for Mus, but that's not much different from what we are
> doing here.
> >
> > I don't have a good feel for how many cursors and display
> configurations that
> > are supported. Is this on the order of tens of images, or hundreds? If
> it's
> > closer to hundreds, then I agree with you. If it's closer to
> 10-20(ish) then
> > preloading seems better.
>
>
> Ok. I think we have just two cursor sets right now - normal, and large.
> Each contains 32 cursor bitmaps, plus there's 2 animated cursors which
> are currently shared, but should be duplicated according to a comment.
>
> Another potential concern with preloading is Windows. It doesn't have
> cursor load functions implemented (CursorLoaderWin::LoadImageCursor), it
> only loads cursors when they are set - I am not sure if there is some
> subtle reason for this, or this was just an accidental omission. Do you
> happen to know?
>
> If we go the preload route, I think the way to go would be to teach
> cursor code in base to preload all cursor resources and operate on
> preloaded resources rather than operating on the resource bundle. Does
> this sound reasonable? The existing code loads cursor bitmaps, and then
> scales/rotates them in the same function, e.g. see GetImageCursorBitmap.
> I think we'd have to break that apart so that we don't need to duplicate
> code.
>
> I am thinking we can change the existing Cursor Loaders (ozone, win, and
> x11) to support preloading cursors for all display configurations and
> then using that. We can then pass a cursor loader instance to the UI
> Service constructor.
>
> https://codereview.chromium.org/2916823002/
>

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

[mfomitchev, 2017-06-20 19:51:16]

Would you mind explaining why you feel that the approach in this CL is less
simple than preloading resources? It's certainly less code, and it seems
pretty straightforward. The only potential concern I can think of with the 
current implementation is that it's async, but the complexity associated
with that seems to be pretty manageable. What am I missing?


On 2017/06/20 19:15:04, sky wrote:
> Elliot and myself spoke about this and feel that this isn't a lot of data,
> the images aren't big and windows is rather out of scope at the moment so
> we should make a copy given it simplifies things. Sound good?
> 
>   -Scott
> 
> On Tue, Jun 20, 2017 at 10:00 AM, <mailto:mfomitchev@chromium.org> wrote:
> 
> >
> > Ok. I think we have just two cursor sets right now - normal, and large.
> > Each contains 32 cursor bitmaps, plus there's 2 animated cursors which
> > are currently shared, but should be duplicated according to a comment.
> >
> > Another potential concern with preloading is Windows. It doesn't have
> > cursor load functions implemented (CursorLoaderWin::LoadImageCursor), it
> > only loads cursors when they are set - I am not sure if there is some
> > subtle reason for this, or this was just an accidental omission. Do you
> > happen to know?
> >
> > If we go the preload route, I think the way to go would be to teach
> > cursor code in base to preload all cursor resources and operate on
> > preloaded resources rather than operating on the resource bundle. Does
> > this sound reasonable? The existing code loads cursor bitmaps, and then
> > scales/rotates them in the same function, e.g. see GetImageCursorBitmap.
> > I think we'd have to break that apart so that we don't need to duplicate
> > code.
> >
> > I am thinking we can change the existing Cursor Loaders (ozone, win, and
> > x11) to support preloading cursors for all display configurations and
> > then using that. We can then pass a cursor loader instance to the UI
> > Service constructor.
> >
> > https://codereview.chromium.org/2916823002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[Elliot Glaysher, 2017-06-20 20:16:47]

On 2017/06/20 19:51:16, mfomitchev wrote:
> Would you mind explaining why you feel that the approach in this CL is less
> simple than preloading resources? It's certainly less code, and it seems
> pretty straightforward. The only potential concern I can think of with the 
> current implementation is that it's async, but the complexity associated
> with that seems to be pretty manageable. What am I missing?

An outside view argument that you should stay away from asynchronous thread
hops. Sure, you have an inside view argument that it might not be that bad in
this case, but the power of the outside view is that it looks at all the times
people tried to use local, inside view reasoning and were mistaken.

(If you aren't familiar with the terms inside view/outside view,
https://en.wikipedia.org/wiki/Reference_class_forecasting ).

[sky, 2017-06-20 20:21:18]

Generally the smaller the code the less complex it is and the easier it is
to maintain. All wins! Do you feel the code you have here isn't appreciably
smaller than the preload route?

  -Scott

On Tue, Jun 20, 2017 at 1:16 PM, <erg@chromium.org> wrote:

> On 2017/06/20 19:51:16, mfomitchev wrote:
> > Would you mind explaining why you feel that the approach in this CL is
> less
> > simple than preloading resources? It's certainly less code, and it seems
> > pretty straightforward. The only potential concern I can think of with
> the
> > current implementation is that it's async, but the complexity associated
> > with that seems to be pretty manageable. What am I missing?
>
> An outside view argument that you should stay away from asynchronous thread
> hops. Sure, you have an inside view argument that it might not be that bad
> in
> this case, but the power of the outside view is that it looks at all the
> times
> people tried to use local, inside view reasoning and were mistaken.
>
> (If you aren't familiar with the terms inside view/outside view,
> https://en.wikipedia.org/wiki/Reference_class_forecasting ).
>
> https://codereview.chromium.org/2916823002/
>

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

[mfomitchev, 2017-06-20 21:41:52]

@sky:
I think the preload route will be more code. The new cursor code right now is
pretty small: services/ui/ws/threaded_image_cursors.*, the rest is plumbing
which will need to exist with the preload route as well. The preload approach 
would require teaching ui/base/cursor/ classes to preload all resources and
use them, which seems like a fair amount of changes. BTW, we will need to store
66 (32+32+2) additional bitmaps in memory (not 32). This is because the cursor
bitmaps  we are storing right now are potentially scaled and rotated. We will
need to store all the originals in addition to this.

@erg:
I see where you are coming from. I think the preload approach is a bit hacky 
as well though. And it seems the nature of the project we are working on proves
that some cross-thread/cross-process hops are okay if they are solving other
issues :)

FWIW, I asked Sadrul what he thought about this, and he said he slightly prefers
the current approach.

I don't want to argue over this too much though if you are still strongly
convinced preloading is better. Otherwise we can continue the discussion -
either
here, or perhaps over VC. I could also code up preloading and then we can
reevaluate.

On 2017/06/20 20:21:18, sky wrote:
> Generally the smaller the code the less complex it is and the easier it is
> to maintain. All wins! Do you feel the code you have here isn't appreciably
> smaller than the preload route?
> 
>   -Scott
> 
> On Tue, Jun 20, 2017 at 1:16 PM, <mailto:erg@chromium.org> wrote:
> 
> > On 2017/06/20 19:51:16, mfomitchev wrote:
> > > Would you mind explaining why you feel that the approach in this CL is
> > less
> > > simple than preloading resources? It's certainly less code, and it seems
> > > pretty straightforward. The only potential concern I can think of with
> > the
> > > current implementation is that it's async, but the complexity associated
> > > with that seems to be pretty manageable. What am I missing?
> >
> > An outside view argument that you should stay away from asynchronous thread
> > hops. Sure, you have an inside view argument that it might not be that bad
> > in
> > this case, but the power of the outside view is that it looks at all the
> > times
> > people tried to use local, inside view reasoning and were mistaken.
> >
> > (If you aren't familiar with the terms inside view/outside view,
> > https://en.wikipedia.org/wiki/Reference_class_forecasting ).
> >
> > https://codereview.chromium.org/2916823002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-06-20 21:55:37]

Ok, you convinced me, go with what you have here.

  -Scott

On Tue, Jun 20, 2017 at 2:41 PM, <mfomitchev@chromium.org> wrote:

> @sky:
> I think the preload route will be more code. The new cursor code right now
> is
> pretty small: services/ui/ws/threaded_image_cursors.*, the rest is
> plumbing
> which will need to exist with the preload route as well. The preload
> approach
> would require teaching ui/base/cursor/ classes to preload all resources and
> use them, which seems like a fair amount of changes. BTW, we will need to
> store
> 66 (32+32+2) additional bitmaps in memory (not 32). This is because the
> cursor
> bitmaps we are storing right now are potentially scaled and rotated. We
> will
> need to store all the originals in addition to this.
>
> @erg:
> I see where you are coming from. I think the preload approach is a bit
> hacky
> as well though. And it seems the nature of the project we are working on
> proves
> that some cross-thread/cross-process hops are okay if they are solving
> other
> issues :)
>
> FWIW, I asked Sadrul what he thought about this, and he said he slightly
> prefers
> the current approach.
>
> I don't want to argue over this too much though if you are still strongly
> convinced preloading is better. Otherwise we can continue the discussion -
> either
> here, or perhaps over VC. I could also code up preloading and then we can
> reevaluate.
>
> On 2017/06/20 20:21:18, sky wrote:
> > Generally the smaller the code the less complex it is and the easier it
> is
> > to maintain. All wins! Do you feel the code you have here isn't
> appreciably
> > smaller than the preload route?
> >
> > -Scott
> >
> > On Tue, Jun 20, 2017 at 1:16 PM, <mailto:erg@chromium.org> wrote:
> >
> > > On 2017/06/20 19:51:16, mfomitchev wrote:
> > > > Would you mind explaining why you feel that the approach in this CL
> is
> > > less
> > > > simple than preloading resources? It's certainly less code, and it
> seems
> > > > pretty straightforward. The only potential concern I can think of
> with
> > > the
> > > > current implementation is that it's async, but the complexity
> associated
> > > > with that seems to be pretty manageable. What am I missing?
> > >
> > > An outside view argument that you should stay away from asynchronous
> thread
> > > hops. Sure, you have an inside view argument that it might not be that
> bad
> > > in
> > > this case, but the power of the outside view is that it looks at all
> the
> > > times
> > > people tried to use local, inside view reasoning and were mistaken.
> > >
> > > (If you aren't familiar with the terms inside view/outside view,
> > > https://en.wikipedia.org/wiki/Reference_class_forecasting ).
> > >
> > > https://codereview.chromium.org/2916823002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2916823002/
>

-- 
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-06-20 22:33:13]

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
File services/ui/ws/threaded_image_cursors.cc (right):

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
services/ui/ws/threaded_image_cursors.cc:23: if (task_runner_) {
WDYT of *always* using a task runner? I realize it's unnecessary in the
non-in-process case, but it would mean far less conditionals like this.

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
services/ui/ws/threaded_image_cursors.cc:51: ui::PlatformWindow*
platform_window) {
How do you know platformwindow is still valid by the time you get here?

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
File services/ui/ws/threaded_image_cursors.h (right):

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
services/ui/ws/threaded_image_cursors.h:24: ThreadedImageCursors(
explicit.

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
services/ui/ws/threaded_image_cursors.h:43: };
DISALLOW...

[sky, 2017-06-20 22:34:00]

On 2017/06/20 22:33:13, sky wrote:
>
https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
> File services/ui/ws/threaded_image_cursors.cc (right):
> 
>
https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
> services/ui/ws/threaded_image_cursors.cc:23: if (task_runner_) {
> WDYT of *always* using a task runner? I realize it's unnecessary in the
> non-in-process case, but it would mean far less conditionals like this.
> 
>
https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
> services/ui/ws/threaded_image_cursors.cc:51: ui::PlatformWindow*
> platform_window) {
> How do you know platformwindow is still valid by the time you get here?
> 
>
https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
> File services/ui/ws/threaded_image_cursors.h (right):
> 
>
https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
> services/ui/ws/threaded_image_cursors.h:24: ThreadedImageCursors(
> explicit.
> 
>
https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
> services/ui/ws/threaded_image_cursors.h:43: };
> DISALLOW...

Also, please name methods differently that are executed in different threads.
Hopefully this will make it more understandable where things are running.

[mfomitchev, 2017-06-21 21:22:51]

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
File services/ui/ws/threaded_image_cursors.cc (right):

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
services/ui/ws/threaded_image_cursors.cc:23: if (task_runner_) {
On 2017/06/20 22:33:13, sky wrote:
> WDYT of *always* using a task runner? I realize it's unnecessary in the
> non-in-process case, but it would mean far less conditionals like this.

Done.

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
services/ui/ws/threaded_image_cursors.cc:51: ui::PlatformWindow*
platform_window) {
On 2017/06/20 22:33:13, sky wrote:
> How do you know platformwindow is still valid by the time you get here?

Sigh.

Ok, so platform_window_ will outlive ThreadedImageCursors. This is because both
are owned by PlatformDisplayDefault, and platform_window_ is only reset in
PlatformDisplayDefault's destructor, and that's done after image_cursors_ is
reset.

However, there is a thread safety issue with ThreadedImageCursors itself: we
shouldn't access its internal state (image_cursors_) on the browser UI thread,
because it is getting destroyed on the WS thread. It looks like this would only
happen at shutdown, but still..

I've fixed this by moving the ownership of image_cursors_ to the UI thread (I've
put it in ChromeContentBrowserClient, which also creates the ui::Service, there
may be a better place for it). This makes the code more complex though. Please
take another look, and let me know what you think.

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
File services/ui/ws/threaded_image_cursors.h (right):

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
services/ui/ws/threaded_image_cursors.h:24: ThreadedImageCursors(
On 2017/06/20 22:33:13, sky wrote:
> explicit.

Takes two args now.

https://codereview.chromium.org/2916823002/diff/40001/services/ui/ws/threaded...
services/ui/ws/threaded_image_cursors.h:43: };
On 2017/06/20 22:33:13, sky wrote:
> DISALLOW...

Done.

[sky, 2017-06-22 00:50:53]

This looks reasonable to me now. You mention this as WIP and there is still some
debugging code in here. Let me know when cleaned up and you are ready for a
final review.

[mfomitchev, 2017-06-22 22:27:41]

Does it make sense to not execute any code in Service::InitializeResources() in
the in-process case?
Looking at that code, I am also wondering if we could've solved the cursor
issues by making resource bundles thread-local? I guess that would've wasted a
lot of memory, but would it be a feasible option otherwise?

[sky, 2017-06-23 03:23:22]

On Thu, Jun 22, 2017 at 3:27 PM, <mfomitchev@chromium.org> wrote:

> Does it make sense to not execute any code in
> Service::InitializeResources() in
> the in-process case?
>

I agree. I think the InitializeResource code is only applicable to the out
of process case.


> Looking at that code, I am also wondering if we could've solved the cursor
> issues by making resource bundles thread-local? I guess that would've
> wasted a
> lot of memory, but would it be a feasible option otherwise?
>

RB already has some multi-threading support now, so it wouldn't make sense
to make it thread local. If anything we could make looking up images work
on multiple threads, but I suspect that would be more work than you have
and may lead to some subtle issues.

  -Scott


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

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

[mfomitchev, 2017-06-26 04:26:12]

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

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

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-26 04:28:51]

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

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

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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mfomitchev, 2017-06-26 04:34:58]

Description was changed from

==========
WIP: Move Mus into chrome's process when running with --mus.

There is a few open issues. Please see the doc for details:
https://docs.google.com/document/d/1EqVUW4H7vNikTd4M9KrTVYIpviVuwktq32WTmc0V6...
Questionable parts of the new code are marked with TODOs.

I'd like to confirm if I am on the right track. Advice on the outstanding issues
would also be appreciated!

BUG=722527
==========

to

==========
Move Mus into browser process when running with --mus.

Moves Mus into browser process when running Mustash (chrome --mus).
Window Server is running on its own thread with ThreadPriority::DISPLAY when
running in browser process.

For now both WS and GPU are moved. Once WS/GPU split if finished, GPU will need
to be moved into its separate process.

A lot of this CL is fixing up various singletons to work when Mus is running
in-process.

Another significant aspect of this CL is making cursors work when Mus is running
in-process. Cursor manipulation requires loading resources from a
ResourceBundle, which can only happen on the browser thread. As such, we pass a
weak pointer to the ui::ImageCursors object and the browser thread task runner
into the UI Service, which uses them to post tasks updating the cursor.

BUG=722527
==========

[mfomitchev, 2017-06-26 04:36:37]

Description was changed from

==========
Move Mus into browser process when running with --mus.

Moves Mus into browser process when running Mustash (chrome --mus).
Window Server is running on its own thread with ThreadPriority::DISPLAY when
running in browser process.

For now both WS and GPU are moved. Once WS/GPU split if finished, GPU will need
to be moved into its separate process.

A lot of this CL is fixing up various singletons to work when Mus is running
in-process.

Another significant aspect of this CL is making cursors work when Mus is running
in-process. Cursor manipulation requires loading resources from a
ResourceBundle, which can only happen on the browser thread. As such, we pass a
weak pointer to the ui::ImageCursors object and the browser thread task runner
into the UI Service, which uses them to post tasks updating the cursor.

BUG=722527
==========

to

==========
Move Mus into browser process when running with --mus.

Moves Mus into browser process when running Mustash (chrome --mus).
Window Server is running on its own thread with ThreadPriority::DISPLAY when
running in browser process.

For now both WS and GPU are moved. Once WS/GPU split if finished, GPU will need
to be moved into its separate process.

A lot of this CL is fixing up various singletons to work when Mus is running
in-process.

Another significant aspect of this CL is making cursors work when Mus is
running in-process. Cursor manipulation requires loading resources from a
ResourceBundle, which can only happen on the browser thread. As such, we pass a
weak pointer to the ui::ImageCursors object and the browser thread task runner
into the UI Service, which uses them to post tasks updating the cursor.

BUG=722527
==========

[mfomitchev, 2017-06-26 05:11:12]

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

[commit-bot: I haz the power, 2017-06-26 05:11: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-26 05:22:30]

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

[commit-bot: I haz the power, 2017-06-26 05:22:31]

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

[mfomitchev, 2017-06-26 05:26:18]

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

[commit-bot: I haz the power, 2017-06-26 05:26:30]

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-26 05:40:52]

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

[commit-bot: I haz the power, 2017-06-26 05:40:53]

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

[mfomitchev, 2017-06-26 05:54:56]

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

[commit-bot: I haz the power, 2017-06-26 05:55:07]

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-26 06:30:23]

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

[commit-bot: I haz the power, 2017-06-26 06:30:24]

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

[mfomitchev, 2017-06-27 15:59:10]

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

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

Dry run: CQ is trying da patch.

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

[mfomitchev, 2017-06-27 16:02:30]

Other than the remaining CursorFactoryOzone conflict with Ash, this should be
good now.

[commit-bot: I haz the power, 2017-06-27 16:41:41]

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

[commit-bot: I haz the power, 2017-06-27 16:41:42]

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

[sky, 2017-06-27 19:58:57]

https://codereview.chromium.org/2916823002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3050: if (!image_cursors_)
Is this conditional necessary?

We're starting to get a fair amount of platform specific code here that isn't
likely to scale particularly well. How about moving this code into
browser_process_platform_part_chromeos? This function would become (after the
ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS ifdef)

g_browser_process->platform_part()->RegisterInProcessServices().

You could do it in a ifdef if you prefer, and if other platforms need the
functionality we can expand the ifdef. WDYT?

https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
File services/ui/display/screen_manager_forwarding.h (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
services/ui/display/screen_manager_forwarding.h:33: // |in_process|  is true if
the UI Service runs inside WM's process, false if
optional: The boolean parameter isn't particularly obvious. Use an enum.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
services/ui/display/screen_manager_forwarding.h:35: ScreenManagerForwarding(bool
in_process);
explicit

https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
services/ui/display/screen_manager_forwarding.h:89: bool in_process_;
const.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/gpu/gpu_ma...
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:59: power_monitor_.reset(new base::PowerMonitor(
MakeUnique for PowerMonitor too?

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.cc
File services/ui/service.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.cc...
services/ui/service.cc:148: // Initialize the cursors.
This comment just documents the code and so isn't useful. Nuke it.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h
File services/ui/service.h (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
services/ui/service.h:74: Service(scoped_refptr<base::SingleThreadTaskRunner>
resource_runner,
optional: Create a structure named InProcessConfig (or something similar) that
contains the two members. This way it's a bit clearer if passed null the ui
service is not run in process.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
services/ui/service.h:86: bool in_process() { return in_process_; }
bool in_process() const
optional: prefix member and function with 'is_' as 'in process' reads like a
verb.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
services/ui/service.h:180: // Can be used to access resources. Will be null
until the service is started
Please group in-process related state into a structure to make it clear what is
needed when in process and what isn't. This structure would be difference than
the one I recommended above.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
File services/ui/ws/platform_display_default.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:53: // and ImageCursorsAsync first
because they can still be using the platform
Is the 'Async' intentional here?

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:114: // platform_window_ is destroyed
after image_cursors_, so it is guaranteed
|platform_window_|

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:115: // to outlive image_cursors_.
|image_cursors_|

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
File services/ui/ws/threaded_image_cursors.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
services/ui/ws/threaded_image_cursors.cc:6: #include "base/bind.h"
newline between 5 and 6.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
services/ui/ws/threaded_image_cursors.cc:45: ws_task_runner->PostTask(
It's worth a comment as to why you process setting the cursor on the platform
window back on the ws thread.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
File services/ui/ws/threaded_image_cursors.h (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
services/ui/ws/threaded_image_cursors.h:54: // Task runner of the Window Server
thread (where this object is created and
In some comments you refer to the 'ui service' (the comments you are adding to
service.h), and here the 'window server.' Be consistent. We should try to
standardize on ui service, given that's the name and directory.

https://codereview.chromium.org/2916823002/diff/180001/ui/aura/env.cc
File ui/aura/env.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/aura/env.cc#newcode73
ui/aura/env.cc:73: DCHECK(env) << "~~Env::CreateInstance must be called before
getting the "
remove ~~?

https://codereview.chromium.org/2916823002/diff/180001/ui/events/devices/inpu...
File ui/events/devices/input_device_manager.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/events/devices/inpu...
ui/events/devices/input_device_manager.cc:14:
base::LazyInstance<base::ThreadLocalPointer<InputDeviceManager>>::Leaky
Is this actually necessary? What code in chrome/ash is calling into this?

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
File ui/events/platform/platform_event_source.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
ui/events/platform/platform_event_source.cc:23:
base::LazyInstance<base::ThreadLocalPointer<PlatformEventSource>>::Leaky
Same comment here. What code in chrome/ash is making use of this?

[mfomitchev, 2017-06-27 22:14:55]

Just answering a couple of questions, I will address the rest of the comments in
a follow-up CL.

https://codereview.chromium.org/2916823002/diff/180001/ui/events/devices/inpu...
File ui/events/devices/input_device_manager.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/events/devices/inpu...
ui/events/devices/input_device_manager.cc:14:
base::LazyInstance<base::ThreadLocalPointer<InputDeviceManager>>::Leaky
On 2017/06/27 19:58:57, sky wrote:
> Is this actually necessary? What code in chrome/ash is calling into this?

I believe so. This is the one on the chrome side, it connects to the UI
Service's DeviceDataManager, and it sets itself as the InputDeviceManager
singletone.
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_browser_m...

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
File ui/events/platform/platform_event_source.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
ui/events/platform/platform_event_source.cc:23:
base::LazyInstance<base::ThreadLocalPointer<PlatformEventSource>>::Leaky
On 2017/06/27 19:58:57, sky wrote:
> Same comment here. What code in chrome/ash is making use of this?

Aura does it:
https://cs.chromium.org/chromium/src/ui/aura/env.cc?type=cs&q=PlatformEventSo...

[sky, 2017-06-28 00:22:02]

https://codereview.chromium.org/2916823002/diff/180001/ui/events/devices/inpu...
File ui/events/devices/input_device_manager.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/events/devices/inpu...
ui/events/devices/input_device_manager.cc:14:
base::LazyInstance<base::ThreadLocalPointer<InputDeviceManager>>::Leaky
On 2017/06/27 22:14:55, mfomitchev wrote:
> On 2017/06/27 19:58:57, sky wrote:
> > Is this actually necessary? What code in chrome/ash is calling into this?
> 
> I believe so. This is the one on the chrome side, it connects to the UI
> Service's DeviceDataManager, and it sets itself as the InputDeviceManager
> singletone.
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_browser_m...

Ok, I get it.

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
File ui/events/platform/platform_event_source.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
ui/events/platform/platform_event_source.cc:23:
base::LazyInstance<base::ThreadLocalPointer<PlatformEventSource>>::Leaky
On 2017/06/27 22:14:55, mfomitchev wrote:
> On 2017/06/27 19:58:57, sky wrote:
> > Same comment here. What code in chrome/ash is making use of this?
> 
> Aura does it:
>
https://cs.chromium.org/chromium/src/ui/aura/env.cc?type=cs&q=PlatformEventSo...

That's only for the LOCAL case. How are we triggering the LOCAL case for
mus/mash?

[mfomitchev, 2017-06-28 15:56:32]

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
File ui/events/platform/platform_event_source.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
ui/events/platform/platform_event_source.cc:23:
base::LazyInstance<base::ThreadLocalPointer<PlatformEventSource>>::Leaky
On 2017/06/28 00:22:02, sky wrote:
> On 2017/06/27 22:14:55, mfomitchev wrote:
> > On 2017/06/27 19:58:57, sky wrote:
> > > Same comment here. What code in chrome/ash is making use of this?
> > 
> > Aura does it:
> >
>
https://cs.chromium.org/chromium/src/ui/aura/env.cc?type=cs&q=PlatformEventSo...
> 
> That's only for the LOCAL case. How are we triggering the LOCAL case for
> mus/mash?

Oops, you are right, this isn't needed. I misread the code. Will revert this
back to the way it was.

[mfomitchev, 2017-07-11 21:04:13]

Description was changed from

==========
Move Mus into browser process when running with --mus.

Moves Mus into browser process when running Mustash (chrome --mus).
Window Server is running on its own thread with ThreadPriority::DISPLAY when
running in browser process.

For now both WS and GPU are moved. Once WS/GPU split if finished, GPU will need
to be moved into its separate process.

A lot of this CL is fixing up various singletons to work when Mus is running
in-process.

Another significant aspect of this CL is making cursors work when Mus is
running in-process. Cursor manipulation requires loading resources from a
ResourceBundle, which can only happen on the browser thread. As such, we pass a
weak pointer to the ui::ImageCursors object and the browser thread task runner
into the UI Service, which uses them to post tasks updating the cursor.

BUG=722527
==========

to

==========
Move Mus into browser process when running with --mus.

Moves Mus into browser process when running Mustash (chrome --mus).
Window Server is running on its own thread with ThreadPriority::DISPLAY when
running in browser process.

For now both WS and GPU are moved. Once WS/GPU split if finished, GPU will need
to be moved into its separate process.

A lot of this CL is fixing up various singletons to work when Mus is running
in-process. For example, CursorFactoryOzone and InputDeviceManage are made
thread-local instead of global singletons, because both Mus and ash/chrome
need an instance.

Another significant aspect of this CL is making cursors work when Mus is
running in-process. Cursor manipulation requires loading resources from a
ResourceBundle, which can only happen on the browser thread. As such, we pass a
weak pointer to the ui::ImageCursorsHolder object and the browser thread task
runner into the UI Service, which uses them to post tasks updating the cursor.

BUG=722527
==========

[mfomitchev, 2017-07-11 21:46:46]

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

[mfomitchev, 2017-07-11 21:47:00]

https://codereview.chromium.org/2916823002/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:3050: if (!image_cursors_)
On 2017/06/27 19:58:56, sky OOO wrote:
> Is this conditional necessary?
> 
> We're starting to get a fair amount of platform specific code here that isn't
> likely to scale particularly well. How about moving this code into
> browser_process_platform_part_chromeos? This function would become (after the
> ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS ifdef)
> 
> g_browser_process->platform_part()->RegisterInProcessServices().
> 
> You could do it in a ifdef if you prefer, and if other platforms need the
> functionality we can expand the ifdef. WDYT?

Done. Moved a bunch of code into browser_process_platform_part_chromeos.*

https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
File services/ui/display/screen_manager_forwarding.h (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
services/ui/display/screen_manager_forwarding.h:33: // |in_process|  is true if
the UI Service runs inside WM's process, false if
On 2017/06/27 19:58:56, sky wrote:
> optional: The boolean parameter isn't particularly obvious. Use an enum.

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
services/ui/display/screen_manager_forwarding.h:35: ScreenManagerForwarding(bool
in_process);
On 2017/06/27 19:58:56, sky wrote:
> explicit

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
services/ui/display/screen_manager_forwarding.h:89: bool in_process_;
On 2017/06/27 19:58:56, sky wrote:
> const.

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/gpu/gpu_ma...
File services/ui/gpu/gpu_main.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/gpu/gpu_ma...
services/ui/gpu/gpu_main.cc:59: power_monitor_.reset(new base::PowerMonitor(
On 2017/06/27 19:58:56, sky wrote:
> MakeUnique for PowerMonitor too?

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.cc
File services/ui/service.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.cc...
services/ui/service.cc:148: // Initialize the cursors.
On 2017/06/27 19:58:56, sky OOO wrote:
> This comment just documents the code and so isn't useful. Nuke it.

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h
File services/ui/service.h (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
services/ui/service.h:74: Service(scoped_refptr<base::SingleThreadTaskRunner>
resource_runner,
On 2017/06/27 19:58:56, sky OOO wrote:
> optional: Create a structure named InProcessConfig (or something similar) that
> contains the two members. This way it's a bit clearer if passed null the ui
> service is not run in process.

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
services/ui/service.h:86: bool in_process() { return in_process_; }
On 2017/06/27 19:58:56, sky OOO wrote:
> bool in_process() const
> optional: prefix member and function with 'is_' as 'in process' reads like a
> verb.

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
services/ui/service.h:180: // Can be used to access resources. Will be null
until the service is started
On 2017/06/27 19:58:56, sky OOO wrote:
> Please group in-process related state into a structure to make it clear what
is
> needed when in process and what isn't. This structure would be difference than
> the one I recommended above.

Created ThreadedImageCursorsFactory(Impl). I think it makes some other things
cleaner as well.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
File services/ui/ws/platform_display_default.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:53: // and ImageCursorsAsync first
because they can still be using the platform
On 2017/06/27 19:58:57, sky OOO wrote:
> Is the 'Async' intentional here?

Oops, old name. Fixed.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:114: // platform_window_ is destroyed
after image_cursors_, so it is guaranteed
On 2017/06/27 19:58:57, sky OOO wrote:
> |platform_window_|

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
services/ui/ws/platform_display_default.cc:115: // to outlive image_cursors_.
On 2017/06/27 19:58:56, sky OOO wrote:
> |image_cursors_|

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
File services/ui/ws/threaded_image_cursors.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
services/ui/ws/threaded_image_cursors.cc:6: #include "base/bind.h"
On 2017/06/27 19:58:57, sky OOO wrote:
> newline between 5 and 6.

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
services/ui/ws/threaded_image_cursors.cc:45: ws_task_runner->PostTask(
On 2017/06/27 19:58:57, sky wrote:
> It's worth a comment as to why you process setting the cursor on the platform
> window back on the ws thread.

Done.

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
File services/ui/ws/threaded_image_cursors.h (right):

https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
services/ui/ws/threaded_image_cursors.h:54: // Task runner of the Window Server
thread (where this object is created and
On 2017/06/27 19:58:57, sky wrote:
> In some comments you refer to the 'ui service' (the comments you are adding to
> service.h), and here the 'window server.' Be consistent. We should try to
> standardize on ui service, given that's the name and directory.

Done.

https://codereview.chromium.org/2916823002/diff/180001/ui/aura/env.cc
File ui/aura/env.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/aura/env.cc#newcode73
ui/aura/env.cc:73: DCHECK(env) << "~~Env::CreateInstance must be called before
getting the "
On 2017/06/27 19:58:57, sky wrote:
> remove ~~?

Oops. Done.

[commit-bot: I haz the power, 2017-07-11 21:47:36]

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-07-11 22:04:38]

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

[commit-bot: I haz the power, 2017-07-11 22:04:40]

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

[sky, 2017-07-11 22:16:44]

On 2017/07/11 21:47:00, mfomitchev wrote:
>
https://codereview.chromium.org/2916823002/diff/180001/chrome/browser/chrome_...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/2916823002/diff/180001/chrome/browser/chrome_...
> chrome/browser/chrome_content_browser_client.cc:3050: if (!image_cursors_)
> On 2017/06/27 19:58:56, sky OOO wrote:
> > Is this conditional necessary?
> > 
> > We're starting to get a fair amount of platform specific code here that
isn't
> > likely to scale particularly well. How about moving this code into
> > browser_process_platform_part_chromeos? This function would become (after
the
> > ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS ifdef)
> > 
> > g_browser_process->platform_part()->RegisterInProcessServices().
> > 
> > You could do it in a ifdef if you prefer, and if other platforms need the
> > functionality we can expand the ifdef. WDYT?
> 
> Done. Moved a bunch of code into browser_process_platform_part_chromeos.*
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
> File services/ui/display/screen_manager_forwarding.h (right):
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
> services/ui/display/screen_manager_forwarding.h:33: // |in_process|  is true
if
> the UI Service runs inside WM's process, false if
> On 2017/06/27 19:58:56, sky wrote:
> > optional: The boolean parameter isn't particularly obvious. Use an enum.
> 
> Done.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
> services/ui/display/screen_manager_forwarding.h:35:
ScreenManagerForwarding(bool
> in_process);
> On 2017/06/27 19:58:56, sky wrote:
> > explicit
> 
> Done.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
> services/ui/display/screen_manager_forwarding.h:89: bool in_process_;
> On 2017/06/27 19:58:56, sky wrote:
> > const.
> 
> Done.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/gpu/gpu_ma...
> File services/ui/gpu/gpu_main.cc (right):
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/gpu/gpu_ma...
> services/ui/gpu/gpu_main.cc:59: power_monitor_.reset(new base::PowerMonitor(
> On 2017/06/27 19:58:56, sky wrote:
> > MakeUnique for PowerMonitor too?
> 
> Done.
> 
> https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.cc
> File services/ui/service.cc (right):
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.cc...
> services/ui/service.cc:148: // Initialize the cursors.
> On 2017/06/27 19:58:56, sky OOO wrote:
> > This comment just documents the code and so isn't useful. Nuke it.
> 
> Done.
> 
> https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h
> File services/ui/service.h (right):
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
> services/ui/service.h:74: Service(scoped_refptr<base::SingleThreadTaskRunner>
> resource_runner,
> On 2017/06/27 19:58:56, sky OOO wrote:
> > optional: Create a structure named InProcessConfig (or something similar)
that
> > contains the two members. This way it's a bit clearer if passed null the ui
> > service is not run in process.
> 
> Done.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
> services/ui/service.h:86: bool in_process() { return in_process_; }
> On 2017/06/27 19:58:56, sky OOO wrote:
> > bool in_process() const
> > optional: prefix member and function with 'is_' as 'in process' reads like a
> > verb.
> 
> Done.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
> services/ui/service.h:180: // Can be used to access resources. Will be null
> until the service is started
> On 2017/06/27 19:58:56, sky OOO wrote:
> > Please group in-process related state into a structure to make it clear what
> is
> > needed when in process and what isn't. This structure would be difference
than
> > the one I recommended above.
> 
> Created ThreadedImageCursorsFactory(Impl). I think it makes some other things
> cleaner as well.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
> File services/ui/ws/platform_display_default.cc (right):
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
> services/ui/ws/platform_display_default.cc:53: // and ImageCursorsAsync first
> because they can still be using the platform
> On 2017/06/27 19:58:57, sky OOO wrote:
> > Is the 'Async' intentional here?
> 
> Oops, old name. Fixed.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
> services/ui/ws/platform_display_default.cc:114: // platform_window_ is
destroyed
> after image_cursors_, so it is guaranteed
> On 2017/06/27 19:58:57, sky OOO wrote:
> > |platform_window_|
> 
> Done.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
> services/ui/ws/platform_display_default.cc:115: // to outlive image_cursors_.
> On 2017/06/27 19:58:56, sky OOO wrote:
> > |image_cursors_|
> 
> Done.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> File services/ui/ws/threaded_image_cursors.cc (right):
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> services/ui/ws/threaded_image_cursors.cc:6: #include "base/bind.h"
> On 2017/06/27 19:58:57, sky OOO wrote:
> > newline between 5 and 6.
> 
> Done.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> services/ui/ws/threaded_image_cursors.cc:45: ws_task_runner->PostTask(
> On 2017/06/27 19:58:57, sky wrote:
> > It's worth a comment as to why you process setting the cursor on the
platform
> > window back on the ws thread.
> 
> Done.
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> File services/ui/ws/threaded_image_cursors.h (right):
> 
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> services/ui/ws/threaded_image_cursors.h:54: // Task runner of the Window
Server
> thread (where this object is created and
> On 2017/06/27 19:58:57, sky wrote:
> > In some comments you refer to the 'ui service' (the comments you are adding
to
> > service.h), and here the 'window server.' Be consistent. We should try to
> > standardize on ui service, given that's the name and directory.
> 
> Done.
> 
> https://codereview.chromium.org/2916823002/diff/180001/ui/aura/env.cc
> File ui/aura/env.cc (right):
> 
>
https://codereview.chromium.org/2916823002/diff/180001/ui/aura/env.cc#newcode73
> ui/aura/env.cc:73: DCHECK(env) << "~~Env::CreateInstance must be called before
> getting the "
> On 2017/06/27 19:58:57, sky wrote:
> > remove ~~?
> 
> Oops. Done.

As this incorporates parts of https://codereview.chromium.org/2961403002/ can we
wait until that lands? Alternatively make this one relative so that it doesn't
include https://codereview.chromium.org/2961403002/ as part of it.

[chromium-reviews, 2017-07-11 22:35:23]

https://codereview.chromium.org/2961403002/ was actually based on this CL (
https://codereview.chromium.org/2916823002/). I can split out the ozone
change into a separate CL though and land that first.

On Tue, Jul 11, 2017, 6:16 PM <sky@chromium.org> wrote:

> On 2017/07/11 21:47:00, mfomitchev wrote:
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/chrome/browser/chrome_...
> > File chrome/browser/chrome_content_browser_client.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/chrome/browser/chrome_...
> > chrome/browser/chrome_content_browser_client.cc:3050: if
> (!image_cursors_)
> > On 2017/06/27 19:58:56, sky OOO wrote:
> > > Is this conditional necessary?
> > >
> > > We're starting to get a fair amount of platform specific code here that
> isn't
> > > likely to scale particularly well. How about moving this code into
> > > browser_process_platform_part_chromeos? This function would become
> (after
> the
> > > ENABLE_MOJO_MEDIA_IN_BROWSER_PROCESS ifdef)
> > >
> > > g_browser_process->platform_part()->RegisterInProcessServices().
> > >
> > > You could do it in a ifdef if you prefer, and if other platforms need
> the
> > > functionality we can expand the ifdef. WDYT?
> >
> > Done. Moved a bunch of code into browser_process_platform_part_chromeos.*
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
> > File services/ui/display/screen_manager_forwarding.h (right):
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
> > services/ui/display/screen_manager_forwarding.h:33: // |in_process| is
> true
> if
> > the UI Service runs inside WM's process, false if
> > On 2017/06/27 19:58:56, sky wrote:
> > > optional: The boolean parameter isn't particularly obvious. Use an
> enum.
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
> > services/ui/display/screen_manager_forwarding.h:35:
> ScreenManagerForwarding(bool
> > in_process);
> > On 2017/06/27 19:58:56, sky wrote:
> > > explicit
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/display/sc...
> > services/ui/display/screen_manager_forwarding.h:89: bool in_process_;
> > On 2017/06/27 19:58:56, sky wrote:
> > > const.
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/gpu/gpu_ma...
> > File services/ui/gpu/gpu_main.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/gpu/gpu_ma...
> > services/ui/gpu/gpu_main.cc:59: power_monitor_.reset(new
> base::PowerMonitor(
> > On 2017/06/27 19:58:56, sky wrote:
> > > MakeUnique for PowerMonitor too?
> >
> > Done.
> >
> >
> https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.cc
> > File services/ui/service.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.cc...
> > services/ui/service.cc:148: // Initialize the cursors.
> > On 2017/06/27 19:58:56, sky OOO wrote:
> > > This comment just documents the code and so isn't useful. Nuke it.
> >
> > Done.
> >
> >
> https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h
> > File services/ui/service.h (right):
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
> > services/ui/service.h:74:
> Service(scoped_refptr<base::SingleThreadTaskRunner>
> > resource_runner,
> > On 2017/06/27 19:58:56, sky OOO wrote:
> > > optional: Create a structure named InProcessConfig (or something
> similar)
> that
> > > contains the two members. This way it's a bit clearer if passed null
> the ui
> > > service is not run in process.
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
> > services/ui/service.h:86: bool in_process() { return in_process_; }
> > On 2017/06/27 19:58:56, sky OOO wrote:
> > > bool in_process() const
> > > optional: prefix member and function with 'is_' as 'in process' reads
> like a
> > > verb.
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/service.h#...
> > services/ui/service.h:180: // Can be used to access resources. Will be
> null
> > until the service is started
> > On 2017/06/27 19:58:56, sky OOO wrote:
> > > Please group in-process related state into a structure to make it
> clear what
> > is
> > > needed when in process and what isn't. This structure would be
> difference
> than
> > > the one I recommended above.
> >
> > Created ThreadedImageCursorsFactory(Impl). I think it makes some other
> things
> > cleaner as well.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
> > File services/ui/ws/platform_display_default.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
> > services/ui/ws/platform_display_default.cc:53: // and ImageCursorsAsync
> first
> > because they can still be using the platform
> > On 2017/06/27 19:58:57, sky OOO wrote:
> > > Is the 'Async' intentional here?
> >
> > Oops, old name. Fixed.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
> > services/ui/ws/platform_display_default.cc:114: // platform_window_ is
> destroyed
> > after image_cursors_, so it is guaranteed
> > On 2017/06/27 19:58:57, sky OOO wrote:
> > > |platform_window_|
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/platfor...
> > services/ui/ws/platform_display_default.cc:115: // to outlive
> image_cursors_.
> > On 2017/06/27 19:58:56, sky OOO wrote:
> > > |image_cursors_|
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> > File services/ui/ws/threaded_image_cursors.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> > services/ui/ws/threaded_image_cursors.cc:6: #include "base/bind.h"
> > On 2017/06/27 19:58:57, sky OOO wrote:
> > > newline between 5 and 6.
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> > services/ui/ws/threaded_image_cursors.cc:45: ws_task_runner->PostTask(
> > On 2017/06/27 19:58:57, sky wrote:
> > > It's worth a comment as to why you process setting the cursor on the
> platform
> > > window back on the ws thread.
> >
> > Done.
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> > File services/ui/ws/threaded_image_cursors.h (right):
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/services/ui/ws/threade...
> > services/ui/ws/threaded_image_cursors.h:54: // Task runner of the Window
> Server
> > thread (where this object is created and
> > On 2017/06/27 19:58:57, sky wrote:
> > > In some comments you refer to the 'ui service' (the comments you are
> adding
> to
> > > service.h), and here the 'window server.' Be consistent. We should try
> to
> > > standardize on ui service, given that's the name and directory.
> >
> > Done.
> >
> > https://codereview.chromium.org/2916823002/diff/180001/ui/aura/env.cc
> > File ui/aura/env.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2916823002/diff/180001/ui/aura/env.cc#newcode73
> > ui/aura/env.cc:73: DCHECK(env) << "~~Env::CreateInstance must be called
> before
> > getting the "
> > On 2017/06/27 19:58:57, sky wrote:
> > > remove ~~?
> >
> > Oops. Done.
>
> As this incorporates parts of https://codereview.chromium.org/2961403002/
> can we
> wait until that lands? Alternatively make this one relative so that it
> doesn't
> include https://codereview.chromium.org/2961403002/ as part of it.
>
> https://codereview.chromium.org/2916823002/
>

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

[mfomitchev, 2017-07-12 18:09:51]

Description was changed from

==========
Move Mus into browser process when running with --mus.

Moves Mus into browser process when running Mustash (chrome --mus).
Window Server is running on its own thread with ThreadPriority::DISPLAY when
running in browser process.

For now both WS and GPU are moved. Once WS/GPU split if finished, GPU will need
to be moved into its separate process.

A lot of this CL is fixing up various singletons to work when Mus is running
in-process. For example, CursorFactoryOzone and InputDeviceManage are made
thread-local instead of global singletons, because both Mus and ash/chrome
need an instance.

Another significant aspect of this CL is making cursors work when Mus is
running in-process. Cursor manipulation requires loading resources from a
ResourceBundle, which can only happen on the browser thread. As such, we pass a
weak pointer to the ui::ImageCursorsHolder object and the browser thread task
runner into the UI Service, which uses them to post tasks updating the cursor.

BUG=722527
==========

to

==========
Move Mus into browser process when running with --mus.

Moves Mus into browser process when running Mustash (chrome --mus).
Window Server is running on its own thread with ThreadPriority::DISPLAY when
running in browser process.

For now both WS and GPU are moved. Once WS/GPU split if finished, GPU will need
to be moved into its separate process.

A lot of this CL is fixing up various singletons to work when Mus is running
in-process. For example, CursorFactoryOzone and InputDeviceManage are made
thread-local instead of global singletons, because both Mus and ash/chrome
need an instance.

Another significant aspect of this CL is making cursors work when Mus is
running in-process. Cursor manipulation requires loading resources from a
ResourceBundle, which can only happen on the browser thread. As such, Mus
creates ui::ImageCursors objects for its displays, but the ownership is
then passed to the browser thread, and all manipulation of these objects
is subsequently also done on the browser thread via Mus posting tasks.

BUG=722527
==========

[mfomitchev, 2017-07-12 18:11:59]

PTAL. Added multi-display support.
Next going to factor out the ozone changes, and work on fixing the build
failures.

[mfomitchev, 2017-07-12 18:31:38]

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

[commit-bot: I haz the power, 2017-07-12 18:31:59]

Dry run: CQ is trying da patch.

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

[mfomitchev, 2017-07-12 18:38:08]

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

[commit-bot: I haz the power, 2017-07-12 18:38:32]

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-07-12 19:01:02]

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

[commit-bot: I haz the power, 2017-07-12 19:01:03]

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

[sky, 2017-07-12 19:25:24]

Can you separate the parts out now that are ready into it's own patch?

On Wed, Jul 12, 2017 at 11:11 AM, <mfomitchev@chromium.org> wrote:

> PTAL. Added multi-display support.
> Next going to factor out the ozone changes, and work on fixing the build
> failures.
>
> https://codereview.chromium.org/2916823002/
>

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

[mfomitchev, 2017-07-12 19:41:11]

On 2017/07/12 19:25:24, sky wrote:
> Can you separate the parts out now that are ready into it's own patch?

You mean the ozone part? I will do that now.

[mfomitchev, 2017-07-12 20:37:13]

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
File ui/events/platform/platform_event_source.cc (right):

https://codereview.chromium.org/2916823002/diff/180001/ui/events/platform/pla...
ui/events/platform/platform_event_source.cc:23:
base::LazyInstance<base::ThreadLocalPointer<PlatformEventSource>>::Leaky
On 2017/06/28 15:56:32, mfomitchev wrote:
> On 2017/06/28 00:22:02, sky wrote:
> > On 2017/06/27 22:14:55, mfomitchev wrote:
> > > On 2017/06/27 19:58:57, sky wrote:
> > > > Same comment here. What code in chrome/ash is making use of this?
> > > 
> > > Aura does it:
> > >
> >
>
https://cs.chromium.org/chromium/src/ui/aura/env.cc?type=cs&q=PlatformEventSo...
> > 
> > That's only for the LOCAL case. How are we triggering the LOCAL case for
> > mus/mash?
> 
> Oops, you are right, this isn't needed. I misread the code. Will revert this
> back to the way it was.

I remember now there was another reason I made this TLS. If it's not TLS, then
Ash can execute code on Mus thread by adding |PlatformEventObserver|s, which
doesn't seem good. This also leads to a concrete issue: 

ui::PlatformEventSource::DispatchEvent ->
ui::UserActivityDetector::DidProcessEvent ->
chromeos::SessionLengthLimiter::OnUserActivity ->
SequenceChecker DCHECK in BrowserProcessImple::local_state()

Do you think it would be okay to make PlatformEventObserver thread-local?

[mfomitchev, 2017-07-12 20:55:12]

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

[commit-bot: I haz the power, 2017-07-12 20:55:44]

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-07-12 21:06:18]

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

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

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

[mfomitchev, 2017-07-12 22:06:07]

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

[commit-bot: I haz the power, 2017-07-12 22:06:29]

Dry run: CQ is trying da patch.

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

[sky, 2017-07-12 22:33:40]

We only want PlatformEventSource used from mus, right? In that case I don't
think you need a TLS, you just need to make sure PlatformEventSource is
only used on mus's thread. How about making it so that ash hits a CHECK if
it attempts to use PlatformEventObserver? Maybe adding a thread check in
PlatformEventSource and having mus call a setter so that PlatformEventSource
knows the only thread it should be used in?

  -Scott

On Wed, Jul 12, 2017 at 1:37 PM, <mfomitchev@chromium.org> wrote:

>
> https://codereview.chromium.org/2916823002/diff/180001/ui/
> events/platform/platform_event_source.cc
> File ui/events/platform/platform_event_source.cc (right):
>
> https://codereview.chromium.org/2916823002/diff/180001/ui/
> events/platform/platform_event_source.cc#newcode23
> ui/events/platform/platform_event_source.cc:23:
> base::LazyInstance<base::ThreadLocalPointer<PlatformEventSource>>::Leaky
> On 2017/06/28 15:56:32, mfomitchev wrote:
> > On 2017/06/28 00:22:02, sky wrote:
> > > On 2017/06/27 22:14:55, mfomitchev wrote:
> > > > On 2017/06/27 19:58:57, sky wrote:
> > > > > Same comment here. What code in chrome/ash is making use of
> this?
> > > >
> > > > Aura does it:
> > > >
> > >
> >
> https://cs.chromium.org/chromium/src/ui/aura/env.cc?
> type=cs&q=PlatformEventSource&sq=package:chromium&l=180
> > >
> > > That's only for the LOCAL case. How are we triggering the LOCAL case
> for
> > > mus/mash?
> >
> > Oops, you are right, this isn't needed. I misread the code. Will
> revert this
> > back to the way it was.
>
> I remember now there was another reason I made this TLS. If it's not
> TLS, then Ash can execute code on Mus thread by adding
> |PlatformEventObserver|s, which doesn't seem good. This also leads to a
> concrete issue:
>
> ui::PlatformEventSource::DispatchEvent ->
> ui::UserActivityDetector::DidProcessEvent ->
> chromeos::SessionLengthLimiter::OnUserActivity ->
> SequenceChecker DCHECK in BrowserProcessImple::local_state()
>
> Do you think it would be okay to make PlatformEventObserver
> thread-local?
>
> https://codereview.chromium.org/2916823002/
>

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

[commit-bot: I haz the power, 2017-07-12 22:48:14]

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

[commit-bot: I haz the power, 2017-07-12 22:48:16]

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

[mfomitchev, 2017-07-12 22:59:13]

It's okay for classic ash to use PlatformEventSource though (e.g. via
UserActivityDetector). The flow which is causing use a problem with in-process
Mus would be WAI with classic Ash. Note that this flow is broken for
out-of-process Mus, but it's not noticeable because UserActivityDetector has
null checks:
https://cs-staging.chromium.org/chromium/src/ui/base/user_activity/user_activ...

The right way to fix this could be to create a client-side PlatformEventSource,
similar to what we have for InputDeviceManager:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_browser_m...
Or create a Mus version of UserActivityDetector talking with the UI Service over
Mojo.

For now though making PlatformEventSource TLS seems like an okay stop gap?

On 2017/07/12 22:33:40, sky wrote:
> We only want PlatformEventSource used from mus, right? In that case I don't
> think you need a TLS, you just need to make sure PlatformEventSource is
> only used on mus's thread. How about making it so that ash hits a CHECK if
> it attempts to use PlatformEventObserver? Maybe adding a thread check in
> PlatformEventSource and having mus call a setter so that PlatformEventSource
> knows the only thread it should be used in?
> 
>   -Scott
> 
> On Wed, Jul 12, 2017 at 1:37 PM, <mailto:mfomitchev@chromium.org> wrote:
> 
> >
> > https://codereview.chromium.org/2916823002/diff/180001/ui/
> > events/platform/platform_event_source.cc
> > File ui/events/platform/platform_event_source.cc (right):
> >
> > https://codereview.chromium.org/2916823002/diff/180001/ui/
> > events/platform/platform_event_source.cc#newcode23
> > ui/events/platform/platform_event_source.cc:23:
> > base::LazyInstance<base::ThreadLocalPointer<PlatformEventSource>>::Leaky
> > On 2017/06/28 15:56:32, mfomitchev wrote:
> > > On 2017/06/28 00:22:02, sky wrote:
> > > > On 2017/06/27 22:14:55, mfomitchev wrote:
> > > > > On 2017/06/27 19:58:57, sky wrote:
> > > > > > Same comment here. What code in chrome/ash is making use of
> > this?
> > > > >
> > > > > Aura does it:
> > > > >
> > > >
> > >
> > https://cs.chromium.org/chromium/src/ui/aura/env.cc
> > type=cs&q=PlatformEventSource&sq=package:chromium&l=180
> > > >
> > > > That's only for the LOCAL case. How are we triggering the LOCAL case
> > for
> > > > mus/mash?
> > >
> > > Oops, you are right, this isn't needed. I misread the code. Will
> > revert this
> > > back to the way it was.
> >
> > I remember now there was another reason I made this TLS. If it's not
> > TLS, then Ash can execute code on Mus thread by adding
> > |PlatformEventObserver|s, which doesn't seem good. This also leads to a
> > concrete issue:
> >
> > ui::PlatformEventSource::DispatchEvent ->
> > ui::UserActivityDetector::DidProcessEvent ->
> > chromeos::SessionLengthLimiter::OnUserActivity ->
> > SequenceChecker DCHECK in BrowserProcessImple::local_state()
> >
> > Do you think it would be okay to make PlatformEventObserver
> > thread-local?
> >
> > https://codereview.chromium.org/2916823002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[mfomitchev, 2017-07-12 23:05:42]

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

[commit-bot: I haz the power, 2017-07-12 23:05:58]

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-07-12 23:57:55]

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

[commit-bot: I haz the power, 2017-07-12 23:57:56]

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

[mfomitchev, 2017-07-13 00:41:02]

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

[commit-bot: I haz the power, 2017-07-13 00:41:30]

Dry run: CQ is trying da patch.

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

[mfomitchev, 2017-07-13 01:17:02]

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

[commit-bot: I haz the power, 2017-07-13 01:17:24]

Dry run: CQ is trying da patch.

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

[mfomitchev, 2017-07-13 01:23:35]

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

[commit-bot: I haz the power, 2017-07-13 01:23:48]

Dry run: CQ is trying da patch.

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

[mfomitchev, 2017-07-13 01:30:00]

Most of the bots seem happy now. I am going to break this up into three parts to
make reviewing easier:
- UI Service changes
- Various singletons changes
- Chrome changes to run Mus in-process

[commit-bot: I haz the power, 2017-07-13 03:00:31]

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

[commit-bot: I haz the power, 2017-07-13 03:00:32]

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

[sky, 2017-07-13 17:02:13]

On Wed, Jul 12, 2017 at 3:59 PM, <mfomitchev@chromium.org> wrote:

> It's okay for classic ash to use PlatformEventSource though (e.g. via
> UserActivityDetector). The flow which is causing use a problem with
> in-process
> Mus would be WAI with classic Ash. Note that this flow is broken for
> out-of-process Mus, but it's not noticeable because UserActivityDetector
> has
> null checks:
> https://cs-staging.chromium.org/chromium/src/ui/base/user_
> activity/user_activity_detector.cc?type=cs&sq=package:chromium&l=56
>
> The right way to fix this could be to create a client-side
> PlatformEventSource,
> similar to what we have for InputDeviceManager:
> https://cs.chromium.org/chromium/src/chrome/browser/
> ui/views/chrome_browser_main_extra_parts_views.cc?type=cs&
> q=inputdeviceclient&l=164
> Or create a Mus version of UserActivityDetector talking with the UI
> Service over
> Mojo.
>
> For now though making PlatformEventSource TLS seems like an okay stop gap?
>

SGTM

  -Scott


>
>
> On 2017/07/12 22:33:40, sky wrote:
> > We only want PlatformEventSource used from mus, right? In that case I
> don't
> > think you need a TLS, you just need to make sure PlatformEventSource is
> > only used on mus's thread. How about making it so that ash hits a CHECK
> if
> > it attempts to use PlatformEventObserver? Maybe adding a thread check in
> > PlatformEventSource and having mus call a setter so that
> PlatformEventSource
> > knows the only thread it should be used in?
> >
> > -Scott
> >
> > On Wed, Jul 12, 2017 at 1:37 PM, <mailto:mfomitchev@chromium.org> wrote:
> >
> > >
> > > https://codereview.chromium.org/2916823002/diff/180001/ui/
> > > events/platform/platform_event_source.cc
> > > File ui/events/platform/platform_event_source.cc (right):
> > >
> > > https://codereview.chromium.org/2916823002/diff/180001/ui/
> > > events/platform/platform_event_source.cc#newcode23
> > > ui/events/platform/platform_event_source.cc:23:
> > > base::LazyInstance<base::ThreadLocalPointer<
> PlatformEventSource>>::Leaky
> > > On 2017/06/28 15:56:32, mfomitchev wrote:
> > > > On 2017/06/28 00:22:02, sky wrote:
> > > > > On 2017/06/27 22:14:55, mfomitchev wrote:
> > > > > > On 2017/06/27 19:58:57, sky wrote:
> > > > > > > Same comment here. What code in chrome/ash is making use of
> > > this?
> > > > > >
> > > > > > Aura does it:
> > > > > >
> > > > >
> > > >
> > > https://cs.chromium.org/chromium/src/ui/aura/env.cc
> > > type=cs&q=PlatformEventSource&sq=package:chromium&l=180
> > > > >
> > > > > That's only for the LOCAL case. How are we triggering the LOCAL
> case
> > > for
> > > > > mus/mash?
> > > >
> > > > Oops, you are right, this isn't needed. I misread the code. Will
> > > revert this
> > > > back to the way it was.
> > >
> > > I remember now there was another reason I made this TLS. If it's not
> > > TLS, then Ash can execute code on Mus thread by adding
> > > |PlatformEventObserver|s, which doesn't seem good. This also leads to a
> > > concrete issue:
> > >
> > > ui::PlatformEventSource::DispatchEvent ->
> > > ui::UserActivityDetector::DidProcessEvent ->
> > > chromeos::SessionLengthLimiter::OnUserActivity ->
> > > SequenceChecker DCHECK in BrowserProcessImple::local_state()
> > >
> > > Do you think it would be okay to make PlatformEventObserver
> > > thread-local?
> > >
> > > https://codereview.chromium.org/2916823002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2916823002/
>

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