Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1455793005:
Adding a way to associate Android Activities with PlatformWindowAndroid (Closed)
|
Created:
5 years, 1 month ago by mfomitchev Modified:
4 years, 11 months ago Reviewers:
sadrul CC:
chromium-reviews, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@auraclank_upstream_native_widget Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdding a way to associate Android Activities with PlatformWindowAndroid
Adding kActivityForRootWindow property to aura::Window and using this new property
in WindowTreeHostPlatform::Show to set PlatformWindowAndroid as the content view on
the Activity.
The expectation is that the client code
will set the activity as a property on the root window before WTH::Show() is called.
BUG=507792
Patch Set 1 #
Total comments: 10
Patch Set 2 : Adding WindowTreeHostPlatform.java #Patch Set 3 : Updating upstream #Patch Set 4 : Fleshing out more: WindowTreeHostPlatformAndroid.java #Patch Set 5 : Getting rid of WindowTreeHostPlatformAndroid.java, just using a static method in PlatformWindowAndr… #
Total comments: 6
Patch Set 6 : Addressing feedback #
Total comments: 5
Patch Set 7 : Addressing feedback. #Patch Set 8 : Rebase + using JavaObjectWeakGlobalRef* instead of jobject for the new WindowProperty. #Patch Set 9 : Making the new property "owned" property. #
Depends on Patchset: Messages
Total messages: 25 (5 generated)
https://codereview.chromium.org/1455793005/diff/1/ui/aura/client/aura_constan... File ui/aura/client/aura_constants.h (right): https://codereview.chromium.org/1455793005/diff/1/ui/aura/client/aura_constan... ui/aura/client/aura_constants.h:27: AURA_EXPORT extern const WindowProperty<jobject>* const kActivityForRootWindow; Not sure if this is an appropriate place, but we'd need to use kActivityForRootWindow from multiple places, so it can't be simply defined in window_tree_host_android.cc for example. https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... ui/aura/window_tree_host_platform.cc:81: // Java_PlatformWindowAndroid_createForActivity( We can't call into Java_PWA from here, so we'll need to create a Java class for WTHAndroid, which would call PlatformWindowAndroid.createForActivity https://codereview.chromium.org/1455793005/diff/1/ui/platform_window/android/... File ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java (right): https://codereview.chromium.org/1455793005/diff/1/ui/platform_window/android/... ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java:37: nativeJavaPlatformWindowCreated(nativeViewport); Note this call
mfomitchev@chromium.org changed reviewers: + sadrul@chromium.org
Sadrul, can you PTAL?
https://codereview.chromium.org/1455793005/diff/1/ui/platform_window/android/... File ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java (right): https://codereview.chromium.org/1455793005/diff/1/ui/platform_window/android/... ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java:39: } It's not clear who will be calling createForActivity()
https://codereview.chromium.org/1455793005/diff/1/ui/platform_window/android/... File ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java (right): https://codereview.chromium.org/1455793005/diff/1/ui/platform_window/android/... ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java:39: } Please see my comment in window_tree_host_platform.cc. It can be done through WTHAndroid.java. I didn't create that class - wanted to make sure the approach is ok first, but it should be pretty straightforward.
https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... ui/aura/window_tree_host_platform.cc:81: // Java_PlatformWindowAndroid_createForActivity( On 2015/11/19 20:57:05, mfomitchev wrote: > We can't call into Java_PWA from here, so we'll need to create a Java class for > WTHAndroid, which would call PlatformWindowAndroid.createForActivity It's not clear what we would gain from calling that from here, or from WTHAndroid::ShowImpl(), when the next line is calling into PlatformWindowAndroid::Show(), where this call currently lives in. https://codereview.chromium.org/1455793005/diff/1/ui/platform_window/android/... File ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java (right): https://codereview.chromium.org/1455793005/diff/1/ui/platform_window/android/... ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java:39: } On 2015/11/20 18:27:15, mfomitchev wrote: > Please see my comment in window_tree_host_platform.cc. It can be done through > WTHAndroid.java. I didn't create that class - wanted to make sure the approach > is ok first, but it should be pretty straightforward. Oh sorry, missed that. Added a comment there.
https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... ui/aura/window_tree_host_platform.cc:81: // Java_PlatformWindowAndroid_createForActivity( Well, the problem is that PWA has no reference to the Activity, and it's not clear how it would be able to get it unless we changed PlatformWindow or PlatformWindowDelegate interface. This is what we talked about the other day, and you recommended we move the logic for showing the activity to WTHAndroid instead.
https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... ui/aura/window_tree_host_platform.cc:81: // Java_PlatformWindowAndroid_createForActivity( On 2015/11/20 18:35:01, mfomitchev wrote: > Well, the problem is that PWA has no reference to the Activity, and it's not > clear how it would be able to get it unless we changed PlatformWindow or > PlatformWindowDelegate interface. > > This is what we talked about the other day, and you recommended we move the > logic for showing the activity to WTHAndroid instead. I guess it's still not clear to me how this is going to help with that. Maybe flesh this out a bit more?
https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... ui/aura/window_tree_host_platform.cc:81: // Java_PlatformWindowAndroid_createForActivity( Something like this: - We will have WTHAndroid.java which will have a method createPlatformWindowForActivity, taking the same arguments as PlatformWindowAndroid.createForActivity. - WTHAndroid.createPlatformWindowForActivity will just call the static method PlatformWindowAndroid.createForActivity and return the result. - Right in this spot where the comment is, we will call Java_WTHAndroid_createPlatformWindowForActivity. That's it. Basically, here we want to call into Java_PlatformWindowAndroid_createForActivity, but JNI doesn't let us do this directly: we need to do that through WTHAndroid.java. Does this make sense? If not, I can code up WTHAndroid.java.
On 2015/11/20 20:11:35, mfomitchev wrote: > https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... > File ui/aura/window_tree_host_platform.cc (right): > > https://codereview.chromium.org/1455793005/diff/1/ui/aura/window_tree_host_pl... > ui/aura/window_tree_host_platform.cc:81: // > Java_PlatformWindowAndroid_createForActivity( > Something like this: > - We will have WTHAndroid.java which will have a method > createPlatformWindowForActivity, taking the same arguments as > PlatformWindowAndroid.createForActivity. > - WTHAndroid.createPlatformWindowForActivity will just call the static method > PlatformWindowAndroid.createForActivity and return the result. > - Right in this spot where the comment is, we will call > Java_WTHAndroid_createPlatformWindowForActivity. > > That's it. Basically, here we want to call into > Java_PlatformWindowAndroid_createForActivity, but JNI doesn't let us do this > directly: we need to do that through WTHAndroid.java. > > Does this make sense? If not, I can code up WTHAndroid.java. Sadrul, PTAL: Added WindowTreeHostPlatform.java for flesh out the CL a bit more. The JNI boilerplate code to link up CPP and Java parts of WindowTreeHostPlatform is still missing. Does this help?
Patch Set 4 is the working version where WindowTreeHostPlatform.java is used to create PlatformWindowAndroid. I don't like it though - it's too much boilerplate and a new component (//ui/aura/android) to do one simple thing. I suggest we do something more like the final version of this CL. WDYT?
I think the CL description is a little out of date. Mind updating that? Also, it would be good to include the code that sets the activity as a property on the window() in this patch as well, to give a more complete picture of it. https://codereview.chromium.org/1455793005/diff/80001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1455793005/diff/80001/ui/aura/window_tree_hos... ui/aura/window_tree_host_platform.cc:83: reinterpret_cast<jlong>(window_->GetPlatformImeController())); Curious as to the signature of this function ... why not PlatformWindowAndroid::AttachToActivity(activity) instead? https://codereview.chromium.org/1455793005/diff/80001/ui/aura/window_tree_hos... ui/aura/window_tree_host_platform.cc:84: #endif We shouldn't do this for every call to ::ShowImpl(), just the first one. Right? If you think there will be additional android-specific changes in WindowTreeHostPlatform, then it might make sense to have a separate WindowTreeHostAndroid subclass from WTHPlatform and override ShowImpl(). https://codereview.chromium.org/1455793005/diff/80001/ui/platform_window/andr... File ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java (right): https://codereview.chromium.org/1455793005/diff/80001/ui/platform_window/andr... ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java:37: nativeJavaPlatformWindowCreated(nativeViewport); createForActivity() is already called by native code. What's the point of synchronously calling back into native code?
Description was changed from ========== WIP: Adding an kActivityForRootWindow property to aura::Window This CL is not ready for a full review, just looking for feedback on the approach. The new property is used by WindowTreeHostAndroid to set PlatformWindowAndroid as the content view on the Activity. The expectation is that the client code will set the activity as a property on the root window befor WTH::Show() is called. This CL also changes the logic in PWA: instead of the native PWA instantiating the Java PWA, the Java PWA is now instantiated by WTHAndroid. The CL lacks the implementation of the Java class for WTHAndroid. If the overal approach is fine, I will add it. BUG=507792 ========== to ========== Adding a way to associate Android Activities with PlatformWindowAndroid Adding kActivityForRootWindow property to aura::Window and using this new property in WindowTreeHostAndroid::Show to set PlatformWindowAndroid as the content view on the Activity. The expectation is that the client code will set the activity as a property on the root window before WTH::Show() is called. This CL also changes the logic in PWA: instead of the native PWA instantiating the Java PWA, the Java PWA is now instantiated by WTHAndroid. The CL lacks the implementation of the Java class for WTHAndroid. If the overal approach is fine, I will add it. BUG=507792 ==========
Description was changed from ========== Adding a way to associate Android Activities with PlatformWindowAndroid Adding kActivityForRootWindow property to aura::Window and using this new property in WindowTreeHostAndroid::Show to set PlatformWindowAndroid as the content view on the Activity. The expectation is that the client code will set the activity as a property on the root window before WTH::Show() is called. This CL also changes the logic in PWA: instead of the native PWA instantiating the Java PWA, the Java PWA is now instantiated by WTHAndroid. The CL lacks the implementation of the Java class for WTHAndroid. If the overal approach is fine, I will add it. BUG=507792 ========== to ========== Adding a way to associate Android Activities with PlatformWindowAndroid Adding kActivityForRootWindow property to aura::Window and using this new property in WindowTreeHostAndroid::Show to set PlatformWindowAndroid as the content view on the Activity. The expectation is that the client code will set the activity as a property on the root window before WTH::Show() is called. This CL also changes the logic in PWA: instead of the native PWA instantiating the Java PWA, the Java PWA is now instantiated by WTHAndroid. The CL lacks the implementation of the Java class for WTHAndroid. If the overal approach is fine, I will add it. BUG=507792 ==========
Description was changed from ========== Adding a way to associate Android Activities with PlatformWindowAndroid Adding kActivityForRootWindow property to aura::Window and using this new property in WindowTreeHostAndroid::Show to set PlatformWindowAndroid as the content view on the Activity. The expectation is that the client code will set the activity as a property on the root window before WTH::Show() is called. This CL also changes the logic in PWA: instead of the native PWA instantiating the Java PWA, the Java PWA is now instantiated by WTHAndroid. The CL lacks the implementation of the Java class for WTHAndroid. If the overal approach is fine, I will add it. BUG=507792 ========== to ========== Adding a way to associate Android Activities with PlatformWindowAndroid Adding kActivityForRootWindow property to aura::Window and using this new property in WindowTreeHostAndroid::Show to set PlatformWindowAndroid as the content view on the Activity. The expectation is that the client code will set the activity as a property on the root window before WTH::Show() is called. BUG=507792 ==========
Addressed the feedback except for this part: > Also, it would be good to include the code that sets the activity as a property > on the window() in this patch as well, to give a more complete picture of it. This WIP CL has the content shell setting the new property: https://codereview.chromium.org/1469803006/ Don't bother doing a real review of it yet, but the point is, we need to change multiple files even if we only wanted to outline the general idea. There is only a few lines of code that set activity as property on the root window (the code is in shell_views.cc in Shell::PlatformInitialize), however there's more support code needed for g_global_state, and for getting the Activity from the Shell. And Shell.java for Aura is a new file, and then there's more dependencies from there. So we'd have to bring in a bunch of things which I think would pollute this CL. https://codereview.chromium.org/1455793005/diff/80001/ui/aura/window_tree_hos... File ui/aura/window_tree_host_platform.cc (right): https://codereview.chromium.org/1455793005/diff/80001/ui/aura/window_tree_hos... ui/aura/window_tree_host_platform.cc:83: reinterpret_cast<jlong>(window_->GetPlatformImeController())); On 2015/11/25 20:59:44, sadrul wrote: > Curious as to the signature of this function ... why not > PlatformWindowAndroid::AttachToActivity(activity) instead? Done. https://codereview.chromium.org/1455793005/diff/80001/ui/aura/window_tree_hos... ui/aura/window_tree_host_platform.cc:84: #endif On 2015/11/25 20:59:44, sadrul wrote: > We shouldn't do this for every call to ::ShowImpl(), just the first one. Right? > > If you think there will be additional android-specific changes in > WindowTreeHostPlatform, then it might make sense to have a separate > WindowTreeHostAndroid subclass from WTHPlatform and override ShowImpl(). Oops, yes, forgot to fix this, thanks. Done. I played with creating a new class but we either need to have WTHA duplicate a bunch of code WTHP, or have a new common abstract superclass for WTHA and WTHWinOzone, or have some Android ifdefs in WTHP (in addition to WTHA) and I think the current approach is better than these alternatives. https://codereview.chromium.org/1455793005/diff/80001/ui/platform_window/andr... File ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java (right): https://codereview.chromium.org/1455793005/diff/80001/ui/platform_window/andr... ui/platform_window/android/java/src/org/chromium/ui/PlatformWindowAndroid.java:37: nativeJavaPlatformWindowCreated(nativeViewport); On 2015/11/25 20:59:44, sadrul wrote: > createForActivity() is already called by native code. What's the point of > synchronously calling back into native code? Good point. Done.
*ping*
s/WindowTreeHostAndroid/WindowTreeHostPlatform/ in the CL description. https://codereview.chromium.org/1455793005/diff/100001/ui/platform_window/and... File ui/platform_window/android/platform_window_android.cc (right): https://codereview.chromium.org/1455793005/diff/100001/ui/platform_window/and... ui/platform_window/android/platform_window_android.cc:78: JNIEnv* env = base::android::AttachCurrentThread(); Should this DCHECK(!IsAttachedToActivity())? Or is it possible to change the activity this is attached to? https://codereview.chromium.org/1455793005/diff/100001/ui/platform_window/and... ui/platform_window/android/platform_window_android.cc:177: } Should this set visibility on the android SurfaceView? https://codereview.chromium.org/1455793005/diff/100001/ui/platform_window/and... File ui/platform_window/android/platform_window_android.h (right): https://codereview.chromium.org/1455793005/diff/100001/ui/platform_window/and... ui/platform_window/android/platform_window_android.h:32: bool IsAttachedToActivity(); const
Description was changed from ========== Adding a way to associate Android Activities with PlatformWindowAndroid Adding kActivityForRootWindow property to aura::Window and using this new property in WindowTreeHostAndroid::Show to set PlatformWindowAndroid as the content view on the Activity. The expectation is that the client code will set the activity as a property on the root window before WTH::Show() is called. BUG=507792 ========== to ========== Adding a way to associate Android Activities with PlatformWindowAndroid Adding kActivityForRootWindow property to aura::Window and using this new property in WindowTreeHostPlatform::Show to set PlatformWindowAndroid as the content view on the Activity. The expectation is that the client code will set the activity as a property on the root window before WTH::Show() is called. BUG=507792 ==========
https://codereview.chromium.org/1455793005/diff/100001/ui/platform_window/and... File ui/platform_window/android/platform_window_android.cc (right): https://codereview.chromium.org/1455793005/diff/100001/ui/platform_window/and... ui/platform_window/android/platform_window_android.cc:78: JNIEnv* env = base::android::AttachCurrentThread(); On 2015/12/01 21:29:34, sadrul wrote: > Should this DCHECK(!IsAttachedToActivity())? Or is it possible to change the > activity this is attached to? I'll add the DCHECK for now. If we need to change the attached activity, we can always change this later. https://codereview.chromium.org/1455793005/diff/100001/ui/platform_window/and... ui/platform_window/android/platform_window_android.cc:177: } On 2015/12/01 21:29:34, sadrul wrote: > Should this set visibility on the android SurfaceView? Considering we are not doing anything in Hide(), I don't think we need to be doing anything here? I expect that the SurfaceView state will be updated on the Android site through standard Android mechanisms when the window is minimized, obscured, etc. We might want to hook up Android visibility state to native Show/Hide, but I think that would be a separate CL. This CL just keeps things the way they were in that regard.
*ping*
Based on sievers's feedback in crrev.com/1487123002, I guess we shouldn't be storing a raw jobject as a Window property :/ We should probably change it to a weak reference.
On 2015/12/03 21:04:29, mfomitchev wrote: > Based on sievers's feedback in crrev.com/1487123002, I guess we shouldn't be > storing a raw jobject as a Window property :/ > We should probably change it to a weak reference. Yes, you can use JavaObjectWeakGlobalRef. We can do this as a follow-up but: Eventually we'd want to handle recreating the PWA Java object and activity if it got destroyed. From onDestroy() we could adjust the visibility to hidden (so that all resources get released in case we were destroyed while visible), and when Show() happens the next time we can recreate activity and PWA.
On 2015/12/03 21:44:31, sievers wrote: > On 2015/12/03 21:04:29, mfomitchev wrote: > > Based on sievers's feedback in crrev.com/1487123002, I guess we shouldn't be > > storing a raw jobject as a Window property :/ > > We should probably change it to a weak reference. > > Yes, you can use JavaObjectWeakGlobalRef. > > We can do this as a follow-up but: > Eventually we'd want to handle recreating the PWA Java object and activity if it > got destroyed. > From onDestroy() we could adjust the visibility to hidden (so that all resources > get released in case we were destroyed while visible), and when Show() happens > the next time we can recreate activity and PWA. Actually this didn't make sense, since the activity would get recreated first. So we'd have to figure out if we want the activity destruction to cause the tear-down of all the native parts or not.
On 2015/12/03 21:55:54, sievers wrote: > On 2015/12/03 21:44:31, sievers wrote: > > On 2015/12/03 21:04:29, mfomitchev wrote: > > > Based on sievers's feedback in crrev.com/1487123002, I guess we shouldn't be > > > storing a raw jobject as a Window property :/ > > > We should probably change it to a weak reference. > > > > Yes, you can use JavaObjectWeakGlobalRef. > > > > We can do this as a follow-up but: > > Eventually we'd want to handle recreating the PWA Java object and activity if > it > > got destroyed. > > From onDestroy() we could adjust the visibility to hidden (so that all > resources > > get released in case we were destroyed while visible), and when Show() happens > > the next time we can recreate activity and PWA. > > Actually this didn't make sense, since the activity would get recreated first. > So we'd have to figure out if we want the activity destruction to cause the > tear-down of all the native parts or not. Yeah, I think we'd want to do that. We should call nativeDestroy, which would call WTHA::OnClosed(), which should probably result in WTHA's self destruction. Perhaps we can override onDetachedFromWindow in PWA to be notified when the Activity is destroyed? If all else fails, we could use a special ID for PWA, so that we can find it and call something on it from Activity.onDestroy. In any case, I think that can be done in a separate CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
