WIP fix for Ozone cursor woes in Mushrome: Numéro deux

WIP fix for Ozone cursor woes in Mushrome: Numéro deux

This makes CursorFactoryOzone TLS, so that both the UI Service thread,
and the browser thread cad maintain an instance. 

Additionally, CursorLoaderOzoner saves the factory when created via
CursorFactoryOzone::GetInstance(), and then uses it through its
lifetime. This means that CursorLoaderOzone ends up using the factory
corresponding to the thread it was created on. 

Now, for Mus, we create the ImageCursors object on the Mus thread, but
then pass the ownership to the browser thread via a helper
ImageCursorsHolder class. This ensures that this ImageCursors object
uses Mus's factory, even though it's used on the browser thread.

BUG=722527

[mfomitchev, 2017-06-29 21:23:42]

Description was changed from

==========
WIP fix for Ozone cursor woes in Mushrome: Numéro deux

BUG=NONE
==========

to

==========
WIP fix for Ozone cursor woes in Mushrome: Numéro deux

This makes CursorFactoryOzone TLS, so that both the UI Service thread, and the
browser thread cad maintain an instance. 

Additionally, CursorLoaderOzoner saves the factory when created via
CursorFactoryOzone::GetInstance(), and then uses it through its lifetime. This
means that CursorLoaderOzone ends up using the factory corresponding to the
thread it was created on. 

Now, for Mus, we create the ImageCursors object on the Mus thread, but then pass
the ownership to the browser thread via a helper ImageCursorsHolder class. This
ensures that this ImageCursors object uses Mus's factory, even though it's used
on the browser thread.

BUG=NONE
==========

[mfomitchev, 2017-06-29 21:23:42]

mfomitchev@chromium.org changed reviewers:
+ erg@google.com, sadrul@chromium.org

[mfomitchev, 2017-06-29 21:25:16]

Description was changed from

==========
WIP fix for Ozone cursor woes in Mushrome: Numéro deux

This makes CursorFactoryOzone TLS, so that both the UI Service thread, and the
browser thread cad maintain an instance. 

Additionally, CursorLoaderOzoner saves the factory when created via
CursorFactoryOzone::GetInstance(), and then uses it through its lifetime. This
means that CursorLoaderOzone ends up using the factory corresponding to the
thread it was created on. 

Now, for Mus, we create the ImageCursors object on the Mus thread, but then pass
the ownership to the browser thread via a helper ImageCursorsHolder class. This
ensures that this ImageCursors object uses Mus's factory, even though it's used
on the browser thread.

BUG=NONE
==========

to

==========
WIP fix for Ozone cursor woes in Mushrome: Numéro deux

This makes CursorFactoryOzone TLS, so that both the UI Service thread,
and the browser thread cad maintain an instance. 

Additionally, CursorLoaderOzoner saves the factory when created via
CursorFactoryOzone::GetInstance(), and then uses it through its
lifetime. This means that CursorLoaderOzone ends up using the factory
corresponding to the thread it was created on. 

Now, for Mus, we create the ImageCursors object on the Mus thread, but
then pass the ownership to the browser thread via a helper
ImageCursorsHolder class. This ensures that this ImageCursors object
uses Mus's factory, even though it's used on the browser thread.

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

[sky, 2017-07-11 20:34:26]

On 2017/06/29 21:25:16, mfomitchev wrote:
> Description was changed from
> 
> ==========
> WIP fix for Ozone cursor woes in Mushrome: Numéro deux
> 
> This makes CursorFactoryOzone TLS, so that both the UI Service thread, and the
> browser thread cad maintain an instance. 
> 
> Additionally, CursorLoaderOzoner saves the factory when created via
> CursorFactoryOzone::GetInstance(), and then uses it through its lifetime. This
> means that CursorLoaderOzone ends up using the factory corresponding to the
> thread it was created on. 
> 
> Now, for Mus, we create the ImageCursors object on the Mus thread, but then
pass
> the ownership to the browser thread via a helper ImageCursorsHolder class.
This
> ensures that this ImageCursors object uses Mus's factory, even though it's
used
> on the browser thread.
> 
> BUG=NONE
> ==========
> 
> to
> 
> ==========
> WIP fix for Ozone cursor woes in Mushrome: Numéro deux
> 
> This makes CursorFactoryOzone TLS, so that both the UI Service thread,
> and the browser thread cad maintain an instance. 
> 
> Additionally, CursorLoaderOzoner saves the factory when created via
> CursorFactoryOzone::GetInstance(), and then uses it through its
> lifetime.

Does CursorFactoryOzone have any potential code that requires it to be used on
the thread it was created on?

> This means that CursorLoaderOzone ends up using the factory
> corresponding to the thread it was created on. 
> 
> Now, for Mus, we create the ImageCursors object on the Mus thread, but
> then pass the ownership to the browser thread via a helper
> ImageCursorsHolder class. This ensures that this ImageCursors object
> uses Mus's factory, even though it's used on the browser thread.

Why do you need the helper class rather than passing around a
unique_ptr<ImageCursors>?

> 
> BUG=722527
> ==========

[sky, 2017-07-11 20:34:49]

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

[mfomitchev, 2017-07-11 21:29:24]

> Does CursorFactoryOzone have any potential code that requires it to be used on
> the thread it was created on?

There's three implementation: X11CursorFactoryOzone, BitmapCursorFactoryOzone,
and CursorDataFactoryOzone. None of them have any mention of thread-(un)safety.
X11CursorFactoryOzone is the implementation that ends up getting used on the
desktop, and BitmapCursorFactoryOzoneused with DRM. The latter's constructor
doesn't do anything special. The former executes some X11 code, which seems ok,
but I don't know the details of how that stuff works. Still no mention of
thread-(un)safety though. It also runs fine both on the desktop and on the
device as far as I can tell.

> Why do you need the helper class rather than passing around a
> unique_ptr<ImageCursors>?

All operations on ImageCursors past its initialization will be executed on the 
browser thread, so it needs to be owned by the browser thread as well (otherwise

it might be destroyed by the UI Service's thread while the browser thread uses 
it). So we need something on the browser thread to own it, but we need to create
it on the UI Service's thread. We could just pass around ImageCursors, but the
question is what would we pass it to after creating it on the UI Service's
thread?
This need is fulfilled by ImageCursorsHolder - it is owned by the browser
thread,
and then the wek ref is passed to the UI Service, so that it can set the 
ImageCursors on it.

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

On Tue, Jul 11, 2017 at 2:29 PM, <mfomitchev@chromium.org> wrote:

> Reviewers: sadrul, erg, sky
> CL: https://codereview.chromium.org/2961403002/
>
> Message:
> > Does CursorFactoryOzone have any potential code that requires it to be
> used on
> > the thread it was created on?
>
> There's three implementation: X11CursorFactoryOzone,
> BitmapCursorFactoryOzone,
> and CursorDataFactoryOzone. None of them have any mention of
> thread-(un)safety.
> X11CursorFactoryOzone is the implementation that ends up getting used on
> the
> desktop, and BitmapCursorFactoryOzoneused with DRM. The latter's
> constructor
> doesn't do anything special. The former executes some X11 code, which
> seems ok,
> but I don't know the details of how that stuff works. Still no mention of
> thread-(un)safety though. It also runs fine both on the desktop and on the
> device as far as I can tell.
>
> > Why do you need the helper class rather than passing around a
> > unique_ptr<ImageCursors>?
>
> All operations on ImageCursors past its initialization will be executed on
> the
> browser thread, so it needs to be owned by the browser thread as well
> (otherwise
>
> it might be destroyed by the UI Service's thread while the browser thread
> uses
> it). So we need something on the browser thread to own it, but we need to
> create
> it on the UI Service's thread. We could just pass around ImageCursors, but
> the
> question is what would we pass it to after creating it on the UI Service's
> thread?
>

A callback that takes ownership of the ImageCursors? I think that makes the
expectations clearer. Basically get rid of ImageCursorsHandler and rather
than plumbing through ImageCursorsHandler there is a callback that is run
with the ImageCursors. WDYT?


> This need is fulfilled by ImageCursorsHolder - it is owned by the browser
> thread,
> and then the wek ref is passed to the UI Service, so that it can set the
> ImageCursors on it.
>
>
> Description:
> WIP fix for Ozone cursor woes in Mushrome: Numéro deux
>
> This makes CursorFactoryOzone TLS, so that both the UI Service thread,
> and the browser thread cad maintain an instance.
>
> Additionally, CursorLoaderOzoner saves the factory when created via
> CursorFactoryOzone::GetInstance(), and then uses it through its
> lifetime. This means that CursorLoaderOzone ends up using the factory
> corresponding to the thread it was created on.
>
> Now, for Mus, we create the ImageCursors object on the Mus thread, but
> then pass the ownership to the browser thread via a helper
> ImageCursorsHolder class. This ensures that this ImageCursors object
> uses Mus's factory, even though it's used on the browser thread.
>
> BUG=722527
>
> Affected files (+147, -73 lines):
> M chrome/browser/chrome_content_browser_client.h
> M chrome/browser/chrome_content_browser_client.cc
> M services/ui/service.h
> M services/ui/service.cc
> M services/ui/ws/display.cc
> M services/ui/ws/platform_display.h
> M services/ui/ws/platform_display.cc
> M services/ui/ws/threaded_image_cursors.h
> M services/ui/ws/threaded_image_cursors.cc
> M services/ui/ws/window_server.h
> M services/ui/ws/window_server.cc
> M services/ui/ws/window_server_delegate.h
> M ui/base/cursor/cursor_loader_ozone.h
> M ui/base/cursor/cursor_loader_ozone.cc
> M ui/base/cursor/image_cursors.h
> M ui/base/cursor/image_cursors.cc
> M ui/ozone/public/cursor_factory_ozone.h
> M ui/ozone/public/cursor_factory_ozone.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.

[chromium-reviews, 2017-07-11 22:30:18]

Good idea, I will do that.

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

> On Tue, Jul 11, 2017 at 2:29 PM, <mfomitchev@chromium.org> wrote:
>
>> Reviewers: sadrul, erg, sky
>> CL: https://codereview.chromium.org/2961403002/
>>
>> Message:
>> > Does CursorFactoryOzone have any potential code that requires it to be
>> used on
>> > the thread it was created on?
>>
>> There's three implementation: X11CursorFactoryOzone,
>> BitmapCursorFactoryOzone,
>> and CursorDataFactoryOzone. None of them have any mention of
>> thread-(un)safety.
>> X11CursorFactoryOzone is the implementation that ends up getting used on
>> the
>> desktop, and BitmapCursorFactoryOzoneused with DRM. The latter's
>> constructor
>> doesn't do anything special. The former executes some X11 code, which
>> seems ok,
>> but I don't know the details of how that stuff works. Still no mention of
>> thread-(un)safety though. It also runs fine both on the desktop and on the
>> device as far as I can tell.
>>
>> > Why do you need the helper class rather than passing around a
>> > unique_ptr<ImageCursors>?
>>
>> All operations on ImageCursors past its initialization will be executed
>> on the
>> browser thread, so it needs to be owned by the browser thread as well
>> (otherwise
>>
>> it might be destroyed by the UI Service's thread while the browser thread
>> uses
>> it). So we need something on the browser thread to own it, but we need to
>> create
>> it on the UI Service's thread. We could just pass around ImageCursors,
>> but the
>> question is what would we pass it to after creating it on the UI Service's
>> thread?
>>
>
> A callback that takes ownership of the ImageCursors? I think that makes
> the expectations clearer. Basically get rid of ImageCursorsHandler and
> rather than plumbing through ImageCursorsHandler there is a callback that
> is run with the ImageCursors. WDYT?
>
>
>> This need is fulfilled by ImageCursorsHolder - it is owned by the browser
>> thread,
>> and then the wek ref is passed to the UI Service, so that it can set the
>> ImageCursors on it.
>>
>>
>> Description:
>> WIP fix for Ozone cursor woes in Mushrome: Numéro deux
>>
>> This makes CursorFactoryOzone TLS, so that both the UI Service thread,
>> and the browser thread cad maintain an instance.
>>
>> Additionally, CursorLoaderOzoner saves the factory when created via
>> CursorFactoryOzone::GetInstance(), and then uses it through its
>> lifetime. This means that CursorLoaderOzone ends up using the factory
>> corresponding to the thread it was created on.
>>
>> Now, for Mus, we create the ImageCursors object on the Mus thread, but
>> then pass the ownership to the browser thread via a helper
>> ImageCursorsHolder class. This ensures that this ImageCursors object
>> uses Mus's factory, even though it's used on the browser thread.
>>
>> BUG=722527
>>
>> Affected files (+147, -73 lines):
>> M chrome/browser/chrome_content_browser_client.h
>> M chrome/browser/chrome_content_browser_client.cc
>> M services/ui/service.h
>> M services/ui/service.cc
>> M services/ui/ws/display.cc
>> M services/ui/ws/platform_display.h
>> M services/ui/ws/platform_display.cc
>> M services/ui/ws/threaded_image_cursors.h
>> M services/ui/ws/threaded_image_cursors.cc
>> M services/ui/ws/window_server.h
>> M services/ui/ws/window_server.cc
>> M services/ui/ws/window_server_delegate.h
>> M ui/base/cursor/cursor_loader_ozone.h
>> M ui/base/cursor/cursor_loader_ozone.cc
>> M ui/base/cursor/image_cursors.h
>> M ui/base/cursor/image_cursors.cc
>> M ui/ozone/public/cursor_factory_ozone.h
>> M ui/ozone/public/cursor_factory_ozone.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-07-12 03:12:34]

I have just realized there is an issue with this code: we may have multiple
displays, and each of them needs its own ImageCursors. Furthermore, displays may
be added and removed at runtime, so these ImageCursors objects need to be
added/removed accordingly. So I think instead of an ImageCursorsHolder with
SetImageCursors, we need something like an ImageCursorsCollection with
Add/Remove. We could still go the callback route, supplying two callbacks: one
for adding and another for removing, but at that point I am not sure the
callback approach looks simpler anymore. We'd also need separate callbacks
implementations for the tests, but if we use ImageCursorsCollection, we'd be
able to reuse it in tests. So I am leaning towards using ImageCursorsCollection.
WDYT?

[sky, 2017-07-12 15:38:32]

I agree with your assessment. ImageCursorsCollection SGTM

On Tue, Jul 11, 2017 at 8:12 PM, <mfomitchev@chromium.org> wrote:

> I have just realized there is an issue with this code: we may have multiple
> displays, and each of them needs its own ImageCursors. Furthermore,
> displays may
> be added and removed at runtime, so these ImageCursors objects need to be
> added/removed accordingly. So I think instead of an ImageCursorsHolder with
> SetImageCursors, we need something like an ImageCursorsCollection with
> Add/Remove. We could still go the callback route, supplying two callbacks:
> one
> for adding and another for removing, but at that point I am not sure the
> callback approach looks simpler anymore. We'd also need separate callbacks
> implementations for the tests, but if we use ImageCursorsCollection, we'd
> be
> able to reuse it in tests. So I am leaning towards using
> ImageCursorsCollection.
> WDYT?
>
> https://codereview.chromium.org/2961403002/
>

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