Should gfx::NativeWindow be defined to ViewRoot or WindowAndroid?

[Bo Liu]

This came up in Jinsuk's CL to make WindowAndroid no longer inherit from ViewAndroid, we had to add this method:

https://codereview.chromium.org/2595263002/diff/270001/ui/android/display_android_manager.h#newcode24

.. because Screen's version takes a ViewAndroid, and WindowAndroid is no longer an instance of ViewAndroid.

I think if we had infinite time and energy to refactor the world, then the ideal end result in my head would be:

* ViewRoot is defined as gfx::NativeWindow

* Everywhere that WindowAndroid is used today, use ViewRoot instead, since ViewRoot has a getter for WindowAndroid

* Rename WindowAndroid to something else (ContextHolder?), since it's no longer gfx::NativeWindow

That's a massive refactor and I personally don't see much gain from that. (Actually we really wanted to do that refactor, it's probably easier to pull the non-ViewRoot bits out of WindowAndroid, rather than pull ViewRoot out of WindowAndroid.)

So instead, how about fix all the places that make the assumption that NativeWindow is an instance of NativeView? That's a really odd assumption, that Window is a subclass of View. It holds for aura, but not for mac and ios. So for that CL, we'll just add a method to Screen that takes NativeWindow.

Thoughts?

[Jinsuk Kim]

It's confusing to me why NativeView is passed in the first place to this interface which clearly says 'NearestWindow' in its name. Maybe we first replace it with a method that takes NativeWindow (rather than adding it), see how it breaks on other platforms, and fix them if possible?

For record's sake, NativeWindow becomes NativeView last july https://codereview.chromium.org/2136373002/ on Android, and now on its way to effectively being reverted.

This is the only CL I ran into that makes use of their inheritance relationship  https://codereview.chromium.org/2428383006 in snapshot_android.cc. Not sure if there is any more out there.

--
You received this message because you are subscribed to the Google Groups "Slimming Clank" group.
To unsubscribe from this group and stop receiving emails from it, send an email to slimming-clank+unsubscribe@chromium.org.
To post to this group, send email to slimmin...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/slimming-clank/CAPCJOUpL42%2B2ygeXMhc_b32j_Afypm60e61j5xJ87-hNw%2Bv9rw%40mail.gmail.com.

[Bo Liu]

On Thu, Jan 5, 2017 at 4:28 PM, Jinsuk Kim <jins...@google.com> wrote:

It's confusing to me why NativeView is passed in the first place to this interface which clearly says 'NearestWindow' in its name. Maybe we first replace it with a method that takes NativeWindow (rather than adding it), see how it breaks on other platforms, and fix them if possible?

Looks like a mistake to me: https://codereview.chromium.org/8205018/. ScreenAura had NativeWindow, but the Screen base class got NativeView and no one caught it at the time. Now it's spread to almost everything. Actually it's probably not that hard to fix everywhere. Most use cases are on aura, where Window and View are defined to the same thing. I can give it a crack..

 

For record's sake, NativeWindow becomes NativeView last july https://codereview.chromium.org/2136373002/ on Android, and now on its way to effectively being reverted.

This is the only CL I ran into that makes use of their inheritance relationship  https://codereview.chromium.org/2428383006 in snapshot_android.cc. Not sure if there is any more out there.

I'm adding more: https://codereview.chromium.org/2615793003/

[Bo Liu]

On Thu, Jan 5, 2017 at 4:40 PM, Bo Liu <bo...@chromium.org> wrote:

On Thu, Jan 5, 2017 at 4:28 PM, Jinsuk Kim <jins...@google.com> wrote:

It's confusing to me why NativeView is passed in the first place to this interface which clearly says 'NearestWindow' in its name. Maybe we first replace it with a method that takes NativeWindow (rather than adding it), see how it breaks on other platforms, and fix them if possible?

Looks like a mistake to me: https://codereview.chromium.org/8205018/. ScreenAura had NativeWindow, but the Screen base class got NativeView and no one caught it at the time. Now it's spread to almost everything. Actually it's probably not that hard to fix everywhere. Most use cases are on aura, where Window and View are defined to the same thing. I can give it a crack..

Ok I was wrong. There's lots of cross-platform code (which can't assume relationship between View and Window) that uses View, enough that I don't think it's worthwhile anymore.

Guess we'll just add a new method instead.

[Ted Choc]

On Thu, Jan 5, 2017 at 3:25 PM, Bo Liu <bo...@chromium.org> wrote:

This came up in Jinsuk's CL to make WindowAndroid no longer inherit from ViewAndroid, we had to add this method:

https://codereview.chromium.org/2595263002/diff/270001/ui/android/display_android_manager.h#newcode24

.. because Screen's version takes a ViewAndroid, and WindowAndroid is no longer an instance of ViewAndroid.

I think if we had infinite time and energy to refactor the world, then the ideal end result in my head would be:

* ViewRoot is defined as gfx::NativeWindow

* Everywhere that WindowAndroid is used today, use ViewRoot instead, since ViewRoot has a getter for WindowAndroid

* Rename WindowAndroid to something else (ContextHolder?), since it's no longer gfx::NativeWindow

That's a massive refactor and I personally don't see much gain from that. (Actually we really wanted to do that refactor, it's probably easier to pull the non-ViewRoot bits out of WindowAndroid, rather than pull ViewRoot out of WindowAndroid.) 

WindowAndroid in java is looks like 90% of the code should go to ContextHolder.  WindowAndroid natively is almost all gfx::NativeWindow.  While the refactor might not be immediately worth it, I think splitting the gfx::NativeWindow concept and the dumping ground for Android context interactions seems like a net win.  In my mind, gfx::NativeWindow in Chrome is essentially the CompositorView/SurfaceView.  The closer those can actually be related is better to me.

 

So instead, how about fix all the places that make the assumption that NativeWindow is an instance of NativeView? That's a really odd assumption, that Window is a subclass of View. It holds for aura, but not for mac and ios. So for that CL, we'll just add a method to Screen that takes NativeWindow.

That sounds reasonable to me.

 

Thoughts?

[David Trainor]

On Fri, Jan 6, 2017 at 8:43 AM Ted Choc <ted...@chromium.org> wrote:

On Thu, Jan 5, 2017 at 3:25 PM, Bo Liu <bo...@chromium.org> wrote:

This came up in Jinsuk's CL to make WindowAndroid no longer inherit from ViewAndroid, we had to add this method:

https://codereview.chromium.org/2595263002/diff/270001/ui/android/display_android_manager.h#newcode24

.. because Screen's version takes a ViewAndroid, and WindowAndroid is no longer an instance of ViewAndroid.

I think if we had infinite time and energy to refactor the world, then the ideal end result in my head would be:

* ViewRoot is defined as gfx::NativeWindow

* Everywhere that WindowAndroid is used today, use ViewRoot instead, since ViewRoot has a getter for WindowAndroid

* Rename WindowAndroid to something else (ContextHolder?), since it's no longer gfx::NativeWindow

That's a massive refactor and I personally don't see much gain from that. (Actually we really wanted to do that refactor, it's probably easier to pull the non-ViewRoot bits out of WindowAndroid, rather than pull ViewRoot out of WindowAndroid.) 

WindowAndroid in java is looks like 90% of the code should go to ContextHolder.  WindowAndroid natively is almost all gfx::NativeWindow.  While the refactor might not be immediately worth it, I think splitting the gfx::NativeWindow concept and the dumping ground for Android context interactions seems like a net win.  In my mind, gfx::NativeWindow in Chrome is essentially the CompositorView/SurfaceView.  The closer those can actually be related is better to me.

+1