Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1419843002:
Hold a reference to any kind of context in WindowAndroid. (Closed)
|
Created:
5 years, 2 months ago by gsennton Modified:
5 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHold a reference to any kind of context in WindowAndroid.
Instead of just referencing an application context in WindowAndroid we
now reference any kind of context in WindowAndroid. This is so that we
can reference contexts targeting external displays.
BUG=310763
Committed: https://crrev.com/44b70448c14f6b0341d201c5444e99ee3d8b8357
Cr-Commit-Position: refs/heads/master@{#360066}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Pass WindowAndroid instead of context to resource manager. #Patch Set 3 : Remove keyword explicit from TestResourceManagerImpl ctor. #
Total comments: 6
Patch Set 4 : Pass Context instead of WindowAndroid from native ResourceManager to java side. #
Total comments: 4
Patch Set 5 : Minor fixes according to comments from boliu@. #
Total comments: 9
Patch Set 6 : Move weak context/activity ref from ActivityWA to WA (WindowAndroid) and remove Context ref in Reso… #
Total comments: 13
Patch Set 7 : Cast Context to Activity without check and fix some comments. #Patch Set 8 : Rebase #Patch Set 9 : ResourceManager: Null-check context and other initialization functionality moved to create(..) #
Total comments: 2
Patch Set 10 : Throw exception if Context from WindowAndroid is null in ResourceManager.create. #
Total comments: 1
Patch Set 11 : Fix nit. #Patch Set 12 : Fix failing bot. #Messages
Total messages: 77 (16 generated)
gsennton@chromium.org changed reviewers: + boliu@chromium.org, jdduke@chromium.org
This is the first part of https://codereview.chromium.org/1144333004/
yay smaller patch, looking good https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/compositor_impl_android.cc:235: resource_manager_(root_window->GetJavaDisplayContext().obj()), So continuing the previous conversation, can resource_manager_ just take root_window instead https://codereview.chromium.org/1419843002/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:83: protected WeakReference<Context> mDisplayContextRef; private? Explain this needs to be a weakreference to prevent leaks in webview
https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/compositor_impl_android.cc:235: resource_manager_(root_window->GetJavaDisplayContext().obj()), On 2015/10/21 16:55:38, boliu wrote: > So continuing the previous conversation, can resource_manager_ just take > root_window instead I don't see how that would help (but I might be missing something). We would either have to call GetJavaDisplayContext() from resource_manager.cc, or call WindowAndroid.getDisplayContext() from ResourceManager.java. And we want neither of those to be public if I understand correctly.
https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/compositor_impl_android.cc:235: resource_manager_(root_window->GetJavaDisplayContext().obj()), On 2015/10/21 17:09:08, gsennton wrote: > On 2015/10/21 16:55:38, boliu wrote: > > So continuing the previous conversation, can resource_manager_ just take > > root_window instead > > I don't see how that would help (but I might be missing something). We would > either have to call GetJavaDisplayContext() from resource_manager.cc, or call > WindowAndroid.getDisplayContext() from ResourceManager.java. And we want neither > of those to be public if I understand correctly. Conceptually, window_android.cc and resource_manager.cc are in the same layer (think same package in java). Exposing the GetJavaDisplayContext means exposing it to the content layer too, which isn't strictly needed. For c++, "package visible" probably maps to private + friend though, which is not quite the same thing. But I was wondering if you can actually do this on the java side and actually make use of java package visible.
https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/compositor_impl_android.cc:235: resource_manager_(root_window->GetJavaDisplayContext().obj()), On 2015/10/21 17:13:34, boliu wrote: > On 2015/10/21 17:09:08, gsennton wrote: > > On 2015/10/21 16:55:38, boliu wrote: > > > So continuing the previous conversation, can resource_manager_ just take > > > root_window instead > > > > I don't see how that would help (but I might be missing something). We would > > either have to call GetJavaDisplayContext() from resource_manager.cc, or call > > WindowAndroid.getDisplayContext() from ResourceManager.java. And we want > neither > > of those to be public if I understand correctly. > > Conceptually, window_android.cc and resource_manager.cc are in the same layer > (think same package in java). Exposing the GetJavaDisplayContext means exposing > it to the content layer too, which isn't strictly needed. > > For c++, "package visible" probably maps to private + friend though, which is > not quite the same thing. But I was wondering if you can actually do this on the > java side and actually make use of java package visible. ../../ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:61: error: getDisplayContext() is not public in WindowAndroid; cannot be accessed from outside package Context context = windowAndroid.getDisplayContext(); :/ ResourceManager lives in package org.chromium.ui.resources while WindowAndroid lives in org.chromium.ui.base.
https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/1419843002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/compositor_impl_android.cc:235: resource_manager_(root_window->GetJavaDisplayContext().obj()), On 2015/10/22 10:33:36, gsennton wrote: > On 2015/10/21 17:13:34, boliu wrote: > > On 2015/10/21 17:09:08, gsennton wrote: > > > On 2015/10/21 16:55:38, boliu wrote: > > > > So continuing the previous conversation, can resource_manager_ just take > > > > root_window instead > > > > > > I don't see how that would help (but I might be missing something). We would > > > either have to call GetJavaDisplayContext() from resource_manager.cc, or > call > > > WindowAndroid.getDisplayContext() from ResourceManager.java. And we want > > neither > > > of those to be public if I understand correctly. > > > > Conceptually, window_android.cc and resource_manager.cc are in the same layer > > (think same package in java). Exposing the GetJavaDisplayContext means > exposing > > it to the content layer too, which isn't strictly needed. > > > > For c++, "package visible" probably maps to private + friend though, which is > > not quite the same thing. But I was wondering if you can actually do this on > the > > java side and actually make use of java package visible. > > ../../ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:61: > error: getDisplayContext() is not public in WindowAndroid; cannot be accessed > from outside package > Context context = windowAndroid.getDisplayContext(); > > :/ > ResourceManager lives in package org.chromium.ui.resources while WindowAndroid > lives in org.chromium.ui.base. Jared, any thoughts here? A bit weird these WindowAndroid and ResourceManager are not in the same package? Anyway, even if it's public, only accessed by ui things, it's still better than this patch.
On 2015/10/22 14:12:59, boliu wrote: > Jared, any thoughts here? A bit weird these WindowAndroid and ResourceManager > are not in the same package? > > Anyway, even if it's public, only accessed by ui things, it's still better than > this patch. Hmm, good question. I believe when ResourceManager was upstreamed it was just following the package convention for folders in org/chromium/ui. That does make "package visible" not quite as useful, I suppose, as there's really no concept of "sub" packages in Java.
On 2015/10/22 15:13:01, jdduke wrote: > On 2015/10/22 14:12:59, boliu wrote: > > Jared, any thoughts here? A bit weird these WindowAndroid and ResourceManager > > are not in the same package? > > > > Anyway, even if it's public, only accessed by ui things, it's still better > than > > this patch. > > Hmm, good question. I believe when ResourceManager was upstreamed it was just > following the package convention for folders in org/chromium/ui. That does make > "package visible" not quite as useful, I suppose, as there's really no concept > of "sub" packages in Java. If you're suggesting we move ResourceManager into ui/base, I wouldn't be opposed to that if it improves the situation. It's used in a good number of places outside of ui/.
On 2015/10/22 15:14:12, jdduke wrote: > On 2015/10/22 15:13:01, jdduke wrote: > > On 2015/10/22 14:12:59, boliu wrote: > > > Jared, any thoughts here? A bit weird these WindowAndroid and > ResourceManager > > > are not in the same package? > > > > > > Anyway, even if it's public, only accessed by ui things, it's still better > > than > > > this patch. > > > > Hmm, good question. I believe when ResourceManager was upstreamed it was just > > following the package convention for folders in org/chromium/ui. That does > make > > "package visible" not quite as useful, I suppose, as there's really no concept > > of "sub" packages in Java. > > If you're suggesting we move ResourceManager into ui/base, I wouldn't be opposed > to that if it improves the situation. It's used in a good number of places > outside of ui/. Nah, out of scope for this CL. So for this CL, just make that java method public, but leave a comment it's public only for ResourceManager
Had to create an extra ctor in ResourceManagerImpl just for testing... https://codereview.chromium.org/1419843002/diff/1/ui/android/java/src/org/chr... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/1/ui/android/java/src/org/chr... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:83: protected WeakReference<Context> mDisplayContextRef; On 2015/10/21 16:55:38, boliu wrote: > private? > > Explain this needs to be a weakreference to prevent leaks in webview Done.
lgtm
https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:62: WindowAndroid windowAndroid, long staticResourceManagerPtr) { It's not really clear why we need both of these methods. Can't the native side pass in the appropriate context? Is that not why you added the native implementation of WindowAndroid::GetJavaDisplayContext? https://codereview.chromium.org/1419843002/diff/40001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.h (right): https://codereview.chromium.org/1419843002/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.h:52: explicit ResourceManagerImpl(jobject Context); Nit: lowercase context
https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:62: WindowAndroid windowAndroid, long staticResourceManagerPtr) { On 2015/10/23 16:47:27, jdduke wrote: > It's not really clear why we need both of these methods. Can't the native side > pass in the appropriate context? Is that not why you added the native > implementation of WindowAndroid::GetJavaDisplayContext? Right, so if we use the getter from the java side I guess we can remove the native getter (it was used in the original bigger CL to fetch DeviceDisplayInfo as well but that is not needed in this patch). So the choice for fetching the display context is between having a public java-getter together with this extra testing function or having a public native getter... which option do you prefer? :P
https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:62: WindowAndroid windowAndroid, long staticResourceManagerPtr) { On 2015/10/23 17:01:06, gsennton wrote: > On 2015/10/23 16:47:27, jdduke wrote: > > It's not really clear why we need both of these methods. Can't the native side > > pass in the appropriate context? Is that not why you added the native > > implementation of WindowAndroid::GetJavaDisplayContext? > > Right, so if we use the getter from the java side I guess we can remove the > native getter (it was used in the original bigger CL to fetch DeviceDisplayInfo > as well but that is not needed in this patch). > > So the choice for fetching the display context is between having a public > java-getter together with this extra testing function or having a public native > getter... which option do you prefer? :P Hmm, looks like native getter is better in this case. Where are the tests here? Can they be fixed to use windowandroid instead though?
On 2015/10/23 17:06:33, boliu wrote: > https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... > File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): > > https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... > ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:62: > WindowAndroid windowAndroid, long staticResourceManagerPtr) { > On 2015/10/23 17:01:06, gsennton wrote: > > On 2015/10/23 16:47:27, jdduke wrote: > > > It's not really clear why we need both of these methods. Can't the native > side > > > pass in the appropriate context? Is that not why you added the native > > > implementation of WindowAndroid::GetJavaDisplayContext? > > > > Right, so if we use the getter from the java side I guess we can remove the > > native getter (it was used in the original bigger CL to fetch > DeviceDisplayInfo > > as well but that is not needed in this patch). > > > > So the choice for fetching the display context is between having a public > > java-getter together with this extra testing function or having a public > native > > getter... which option do you prefer? :P > > Hmm, looks like native getter is better in this case. > > Where are the tests here? Can they be fixed to use windowandroid instead though? ui/android/resources/resource_manager_impl_unittest.cc I don't know we would create a new java-WindowAndroid without creating a new create-method in WindowAndroid.java that the test could call?
On 2015/10/23 17:13:35, gsennton wrote: > On 2015/10/23 17:06:33, boliu wrote: > > > https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... > > File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java > (right): > > > > > https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... > > ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:62: > > WindowAndroid windowAndroid, long staticResourceManagerPtr) { > > On 2015/10/23 17:01:06, gsennton wrote: > > > On 2015/10/23 16:47:27, jdduke wrote: > > > > It's not really clear why we need both of these methods. Can't the native > > side > > > > pass in the appropriate context? Is that not why you added the native > > > > implementation of WindowAndroid::GetJavaDisplayContext? > > > > > > Right, so if we use the getter from the java side I guess we can remove the > > > native getter (it was used in the original bigger CL to fetch > > DeviceDisplayInfo > > > as well but that is not needed in this patch). > > > > > > So the choice for fetching the display context is between having a public > > > java-getter together with this extra testing function or having a public > > native > > > getter... which option do you prefer? :P > > > > Hmm, looks like native getter is better in this case. > > > > Where are the tests here? Can they be fixed to use windowandroid instead > though? > > ui/android/resources/resource_manager_impl_unittest.cc > > I don't know we would create a new java-WindowAndroid without creating a new > create-method in WindowAndroid.java that the test could call? Hmm. Is it possible to create a WindowAndroid from the application context in the test? It could be totally useful for other tests too I imagine. That code might only need to live in a test file or something like "test_window_android.cc" or something..
On 2015/10/23 17:48:39, boliu wrote: > On 2015/10/23 17:13:35, gsennton wrote: > > On 2015/10/23 17:06:33, boliu wrote: > > > > > > https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... > > > File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java > > (right): > > > > > > > > > https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... > > > ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:62: > > > WindowAndroid windowAndroid, long staticResourceManagerPtr) { > > > On 2015/10/23 17:01:06, gsennton wrote: > > > > On 2015/10/23 16:47:27, jdduke wrote: > > > > > It's not really clear why we need both of these methods. Can't the > native > > > side > > > > > pass in the appropriate context? Is that not why you added the native > > > > > implementation of WindowAndroid::GetJavaDisplayContext? > > > > > > > > Right, so if we use the getter from the java side I guess we can remove > the > > > > native getter (it was used in the original bigger CL to fetch > > > DeviceDisplayInfo > > > > as well but that is not needed in this patch). > > > > > > > > So the choice for fetching the display context is between having a public > > > > java-getter together with this extra testing function or having a public > > > native > > > > getter... which option do you prefer? :P > > > > > > Hmm, looks like native getter is better in this case. > > > > > > Where are the tests here? Can they be fixed to use windowandroid instead > > though? > > > > ui/android/resources/resource_manager_impl_unittest.cc > > > > I don't know we would create a new java-WindowAndroid without creating a new > > create-method in WindowAndroid.java that the test could call? > > Hmm. Is it possible to create a WindowAndroid from the application context in > the test? It could be totally useful for other tests too I imagine. That code > might only need to live in a test file or something like > "test_window_android.cc" or something.. I'd say that can come as a clean up after this CL. And just leave a TODO in this CL. But I'm not ui owner :p
Don't have any strong opinions here, and I won't be a ui/ owner after today =/, but I'd rather we try to expose a minimal subset of utility/test methods here. https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/40001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:593: // java package (preferably this method would be package private). Nit: Javadoc style
Ok, so now we pass a WindowAndroid to the native ResourceManager(Impl) and from there pass a java Context to the java side. https://codereview.chromium.org/1419843002/diff/40001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.h (right): https://codereview.chromium.org/1419843002/diff/40001/ui/android/resources/re... ui/android/resources/resource_manager_impl.h:52: explicit ResourceManagerImpl(jobject Context); On 2015/10/23 16:47:27, jdduke wrote: > Nit: lowercase context Done.
https://codereview.chromium.org/1419843002/diff/60001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/1419843002/diff/60001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:25: : host_(nullptr) { This constructorcan call through to the one below, which is allowed for c++11 now. https://codereview.chromium.org/1419843002/diff/60001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1419843002/diff/60001/ui/android/window_andro... ui/android/window_android.h:72: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); say this is public for ResourceManager only
gsennton@chromium.org changed reviewers: + newt@chromium.org - jdduke@chromium.org
Since jdduke@ said he wouldn't be owning this anymore I'm adding newt@ here instead for ui/android/ https://codereview.chromium.org/1419843002/diff/60001/ui/android/resources/re... File ui/android/resources/resource_manager_impl.cc (right): https://codereview.chromium.org/1419843002/diff/60001/ui/android/resources/re... ui/android/resources/resource_manager_impl.cc:25: : host_(nullptr) { On 2015/10/26 16:31:49, boliu wrote: > This constructorcan call through to the one below, which is allowed for c++11 > now. Done. https://codereview.chromium.org/1419843002/diff/60001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1419843002/diff/60001/ui/android/window_andro... ui/android/window_android.h:72: base::android::ScopedJavaLocalRef<jobject> GetJavaDisplayContext(); On 2015/10/26 16:31:49, boliu wrote: > say this is public for ResourceManager only Done.
gsennton@chromium.org changed reviewers: + sievers@chromium.org
Adding sievers@ for content/browser/renderer_host/ ptal :)
On 2015/10/27 10:09:02, gsennton wrote: > Adding sievers@ for content/browser/renderer_host/ > > > > ptal :) lgtm
newt@chromium.org changed reviewers: + tedchoc@chromium.org
This seems reasonable. Ted: any thoughts on this? You're more familiar with WindowAndroid than I am.
https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:80: // To render properly on an external display through a Presentation we need to fetch For activity window androids, there will now a weak ref in the ActivityWindowAndroid class and now one here. If we want to go with this route, I think we should keep just a weak ref to the context in the base class and just expose it in all cases. Maybe if we want to get fancy, it would be public WeakReference<? extends Context> getContext() { } then in ActivityWindowAndroid, it would be <Activity> instead If we go that route, I think we are getting close to making ActivityWindowAndroid useless and something that should be replaced with a delegate on the WindowAndroid directly. Because I assume the existing model is correct, you should actually be adding an PresentationWindowAndroid instead of adding this ref here. https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:145: // context could be an Activity Context so we can't hold a strong reference to it. this comment is probably the same if it's a presentation right? We want to avoid strong references to anything that does not have the application lifetime. https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:59: private static ResourceManager create(Context context, long staticResourceManagerPtr) { why not make this take a WindowAndroid? I would rather make that change then expose the context in native at all. https://codereview.chromium.org/1419843002/diff/80001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1419843002/diff/80001/ui/android/window_andro... ui/android/window_android.h:72: // This is public only for use from ResourceManager We specifically wanted to avoid exposing the context to anyone as it is extremely easy to get the lifecycle wrong. Despite having this comment, having this method here allows easy abuse and I doubt it will stop anyone. I think to keep it consistent with the ActivityWindowAndroid implementation, it should return a JavaObjectWeakGlobalRef with very clear documentation that it should never be held on to longer than the lifetime of the context (whatever that may be). For example, I know nothing of a Presentation context and do not know if the resource manager lifecycle will match it exactly. If it outlives it, then we're going to hold onto that and leak everything attached to it.
https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:80: // To render properly on an external display through a Presentation we need to fetch On 2015/10/29 22:39:19, Ted C wrote: > For activity window androids, there will now a weak ref in the > ActivityWindowAndroid class and now one here. > > If we want to go with this route, I think we should keep just a weak ref to the > context in the base class and just expose it in all cases. That works too I guess. It's just weird that ActivityWindowAndroid will have to do a cast to Activity, which is a bit more cognative burden > > Maybe if we want to get fancy, it would be > > public WeakReference<? extends Context> getContext() { > } > > then in ActivityWindowAndroid, it would be <Activity> instead > > If we go that route, I think we are getting close to making > ActivityWindowAndroid useless and something that should be replaced with a > delegate on the WindowAndroid directly. Because I assume the existing model is > correct, you should actually be adding an PresentationWindowAndroid instead of > adding this ref here. Not quite for the PresentationWindowAndroid bit (iiuc). "Display" context is no different from other context. It's just the activity, which in android can be tied to an external display instead of the main display. So the display metrics should come from the activity (if possible), not the application context. https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:59: private static ResourceManager create(Context context, long staticResourceManagerPtr) { On 2015/10/29 22:39:19, Ted C wrote: > why not make this take a WindowAndroid? I would rather make that change then > expose the context in native at all. #15 is where I decided this is slightly less evil: https://codereview.chromium.org/1419843002/#msg15 (Doing the absolute right thing and fixing the test is hard and out of scope for this CL imo)
On 2015/10/29 23:25:42, boliu wrote: > https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... > File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): > > https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... > ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:80: // To render > properly on an external display through a Presentation we need to fetch > On 2015/10/29 22:39:19, Ted C wrote: > > For activity window androids, there will now a weak ref in the > > ActivityWindowAndroid class and now one here. > > > > If we want to go with this route, I think we should keep just a weak ref to > the > > context in the base class and just expose it in all cases. > > That works too I guess. It's just weird that ActivityWindowAndroid will have to > do a cast to Activity, which is a bit more cognative burden > > > > > Maybe if we want to get fancy, it would be > > > > public WeakReference<? extends Context> getContext() { > > } > > > > then in ActivityWindowAndroid, it would be <Activity> instead > > > > If we go that route, I think we are getting close to making > > ActivityWindowAndroid useless and something that should be replaced with a > > delegate on the WindowAndroid directly. Because I assume the existing model > is > > correct, you should actually be adding an PresentationWindowAndroid instead of > > adding this ref here. > > Not quite for the PresentationWindowAndroid bit (iiuc). "Display" context is no > different from other context. It's just the activity, which in android can be > tied to an external display instead of the main display. So the display metrics > should come from the activity (if possible), not the application context. I understand the need to use the context, but the presentation context is a special beast from other context (actually it's just a ContextThemeWrapper with some presentation glue), but it is decidedly not an activity. With that said, it's not strictly useful in any regards while the Activity is, so I guess having a separate class isn't really helpful here. But this change is still moving the main differentiator (i.e. an ability to get the non-application context) of ActivityWindowAndroid to WindowAndroid. > > https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... > File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): > > https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... > ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:59: private > static ResourceManager create(Context context, long staticResourceManagerPtr) { > On 2015/10/29 22:39:19, Ted C wrote: > > why not make this take a WindowAndroid? I would rather make that change then > > expose the context in native at all. > > #15 is where I decided this is slightly less evil: > https://codereview.chromium.org/1419843002/#msg15 > > (Doing the absolute right thing and fixing the test is hard and out of scope for > this CL imo) Why is fixing the test hard? Don't you just have to expose the ability to construct a WindowAndroid from native? You already have the application context available (as used in the test changes above). I'm suggesting removing the ability to access the context from native entirely and adding a new native method to allow constructing a WindowAndroid. Seems like to me it is a pretty trivial change unless I'm missing other things about that test.
On 2015/10/29 23:39:04, Ted C wrote: > On 2015/10/29 23:25:42, boliu wrote: > > > https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... > > File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): > > > > > https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... > > ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:80: // To render > > properly on an external display through a Presentation we need to fetch > > On 2015/10/29 22:39:19, Ted C wrote: > > > For activity window androids, there will now a weak ref in the > > > ActivityWindowAndroid class and now one here. > > > > > > If we want to go with this route, I think we should keep just a weak ref to > > the > > > context in the base class and just expose it in all cases. > > > > That works too I guess. It's just weird that ActivityWindowAndroid will have > to > > do a cast to Activity, which is a bit more cognative burden > > > > > > > > Maybe if we want to get fancy, it would be > > > > > > public WeakReference<? extends Context> getContext() { > > > } > > > > > > then in ActivityWindowAndroid, it would be <Activity> instead > > > > > > If we go that route, I think we are getting close to making > > > ActivityWindowAndroid useless and something that should be replaced with a > > > delegate on the WindowAndroid directly. Because I assume the existing model > > is > > > correct, you should actually be adding an PresentationWindowAndroid instead > of > > > adding this ref here. > > > > Not quite for the PresentationWindowAndroid bit (iiuc). "Display" context is > no > > different from other context. It's just the activity, which in android can be > > tied to an external display instead of the main display. So the display > metrics > > should come from the activity (if possible), not the application context. > > I understand the need to use the context, but the presentation context is > a special beast from other context (actually it's just a ContextThemeWrapper > with some presentation glue), but it is decidedly not an activity. > > With that said, it's not strictly useful in any regards while the Activity is, > so I guess having a separate class isn't really helpful here. But this change > is still moving the main differentiator (i.e. an ability to get the > non-application context) of ActivityWindowAndroid to WindowAndroid. Meh, turns out I still don't know everything. So... that raises a new question. Should the display metrics be retrieved from the wrapper or the underlying activity (because we do dig out the underlying activity in webview) > > > > > > https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... > > File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java > (right): > > > > > https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... > > ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:59: private > > static ResourceManager create(Context context, long staticResourceManagerPtr) > { > > On 2015/10/29 22:39:19, Ted C wrote: > > > why not make this take a WindowAndroid? I would rather make that change > then > > > expose the context in native at all. > > > > #15 is where I decided this is slightly less evil: > > https://codereview.chromium.org/1419843002/#msg15 > > > > (Doing the absolute right thing and fixing the test is hard and out of scope > for > > this CL imo) > > Why is fixing the test hard? Don't you just have to expose the ability to > construct > a WindowAndroid from native? Yep. And I thought that's out of scope. I'm not owner here, so do what owner says :) > You already have the application context available > (as used in the test changes above). > > I'm suggesting removing the ability to access the context from native entirely > and > adding a new native method to allow constructing a WindowAndroid. Seems like > to me it is a pretty trivial change unless I'm missing other things about that > test.
Right, I will have to look into what the ResourceManager lifetime is (or maybe pass a DeviceDisplayInfo object to it for when it needs to display metrics).
On 2015/10/30 17:55:49, gsennton wrote: > Right, I will have to look into what the ResourceManager lifetime is (or maybe > pass a DeviceDisplayInfo object to it for when it needs to display metrics). The ResourceManager only uses the given Context for two things: Rersources (Context.getResources()). DeviceDisplayInfo (this is used in SystemResourceLoader but it is only used to calculate the minimum side of the display -- min(displayHeight,displayWidth)) so we could just pass that value instead. I.e. we could just pass the Resources + minimum display side length to the ResourceManager instead of passing a Context (so that we don't have to worry about leaking the Context). Do we feel better about exposing Resources rather than a Context or do we have similar concerns about leaking that?
On 2015/11/03 15:54:54, gsennton wrote: > Do we feel better about exposing Resources rather than a Context or do we have > similar concerns about leaking that? Dunno. Does Resources hold a reference back to the context? Doesn't look like it from the interface, so maybe holding Resources is safer?
tedchoc@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor as he had opinions about WindowAndroid/Context ownership many moons ago. FWIW, I'm not against holding a context in WindowAndroid btw. I just don't want to expose it to native. I'm also not too keen on there being a getContext and getActivity method on the class as it will be really unclear when to use which one, but there might simply not be a better option. It will also be weird if you can construct a WindowAndroid (specifically not ActivityWindowAndroid) with an Activity reference. It muddles the distinction greatly in my mind. Maybe we should combine the classes and do something smarter about how we handle the passed in context. I am also not against having the resource manager have a window android ref. I do suspect resources are more dependent on a theme reference than a context one, but without digging into the Resources class I wouldn't be sure.
Talked with Ted about this a bit. We both agreed that it's not really in the scope of this change (and why you need it) to refactor WindowAndroid in this CL. Let's just expose a getContext() on WindowAndroid and have ActivityWindowAndroid have a getActivity() that returns the Context as an Activity if it actually is one. I wouldn't call it displayContext because it's use/behavior doesn't really rely on it being a display context. I would just store the weak ref to the context in the base class.
On 2015/11/04 18:03:43, David Trainor wrote: > Talked with Ted about this a bit. We both agreed that it's not really in the > scope of this change (and why you need it) to refactor WindowAndroid in this CL. > Let's just expose a getContext() on WindowAndroid and have > ActivityWindowAndroid have a getActivity() that returns the Context as an > Activity if it actually is one. Haven't looked at this in detail, but just wanted to note that in WebView, the Context objects we use are *never* Activity instances, they are always a ContextWrapper (to handle WebView resources). The underlying wrapped context might be an Activity, but we mustn't use that Activity directly because webview resource references will fail and crash the app. Not sure what that implies for this change :/
On 2015/11/05 15:55:08, Torne wrote: > On 2015/11/04 18:03:43, David Trainor wrote: > > Talked with Ted about this a bit. We both agreed that it's not really in the > > scope of this change (and why you need it) to refactor WindowAndroid in this > CL. > > Let's just expose a getContext() on WindowAndroid and have > > ActivityWindowAndroid have a getActivity() that returns the Context as an > > Activity if it actually is one. > > Haven't looked at this in detail, but just wanted to note that in WebView, the > Context objects we use are *never* Activity instances, they are always a > ContextWrapper (to handle WebView resources). The underlying wrapped context > might be an Activity, but we mustn't use that Activity directly because webview > resource references will fail and crash the app. Not sure what that implies for > this change :/ Good point. That should be okay. WebView doesn't build the ActivityWindowAndroid, so it wouldn't expose the getActivity() method that does the casting AFAIK. Either way that method should do some nice checking and return null if it's not an Activity. It's a WeakRef so that could happen in valid use cases anyway :).
On 2015/11/05 16:11:33, David Trainor wrote: > On 2015/11/05 15:55:08, Torne wrote: > > On 2015/11/04 18:03:43, David Trainor wrote: > > > Talked with Ted about this a bit. We both agreed that it's not really in > the > > > scope of this change (and why you need it) to refactor WindowAndroid in this > > CL. > > > Let's just expose a getContext() on WindowAndroid and have > > > ActivityWindowAndroid have a getActivity() that returns the Context as an > > > Activity if it actually is one. > > > > Haven't looked at this in detail, but just wanted to note that in WebView, the > > Context objects we use are *never* Activity instances, Not quite: Webview does dig the activity out: https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... So if an activity is needed, we have one. But if only a context is needed, we should return the wrapped context. Super complicated.. > they are always a > > ContextWrapper (to handle WebView resources). The underlying wrapped context > > might be an Activity, but we mustn't use that Activity directly because > webview > > resource references will fail and crash the app. Not sure what that implies > for > > this change :/ > > Good point. That should be okay. WebView doesn't build the > ActivityWindowAndroid, So same thing, webview does construct ActivityWindowAndroid objects. > so it wouldn't expose the getActivity() method that does > the casting AFAIK. Either way that method should do some nice checking and > return null if it's not an Activity. It's a WeakRef so that could happen in > valid use cases anyway :).
On 2015/11/05 17:29:26, boliu wrote: > On 2015/11/05 16:11:33, David Trainor wrote: > > On 2015/11/05 15:55:08, Torne wrote: > > > On 2015/11/04 18:03:43, David Trainor wrote: > > > > Talked with Ted about this a bit. We both agreed that it's not really in > > the > > > > scope of this change (and why you need it) to refactor WindowAndroid in > this > > > CL. > > > > Let's just expose a getContext() on WindowAndroid and have > > > > ActivityWindowAndroid have a getActivity() that returns the Context as an > > > > Activity if it actually is one. > > > > > > Haven't looked at this in detail, but just wanted to note that in WebView, > the > > > Context objects we use are *never* Activity instances, > > Not quite: Webview does dig the activity out: > https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... > > So if an activity is needed, we have one. But if only a context is needed, we > should return the wrapped context. Super complicated.. Ugh. Oh dear. This is not really okay, I think? The problem is that when an activity is needed, if we dig the activity out and *then* whatever we needed an activity for ends up referencing resources it will use the wrong context and fail, but if we *don't* dig the activity out then we can't do activity things with it :( Seems like there might be cases where this might be impossible. > > they are always a > > > ContextWrapper (to handle WebView resources). The underlying wrapped context > > > might be an Activity, but we mustn't use that Activity directly because > > webview > > > resource references will fail and crash the app. Not sure what that implies > > for > > > this change :/ > > > > Good point. That should be okay. WebView doesn't build the > > ActivityWindowAndroid, > > So same thing, webview does construct ActivityWindowAndroid objects. > > > so it wouldn't expose the getActivity() method that does > > the casting AFAIK. Either way that method should do some nice checking and > > return null if it's not an Activity. It's a WeakRef so that could happen in > > valid use cases anyway :).
On 2015/11/05 17:31:33, Torne wrote: > On 2015/11/05 17:29:26, boliu wrote: > > On 2015/11/05 16:11:33, David Trainor wrote: > > > On 2015/11/05 15:55:08, Torne wrote: > > > > On 2015/11/04 18:03:43, David Trainor wrote: > > > > > Talked with Ted about this a bit. We both agreed that it's not really > in > > > the > > > > > scope of this change (and why you need it) to refactor WindowAndroid in > > this > > > > CL. > > > > > Let's just expose a getContext() on WindowAndroid and have > > > > > ActivityWindowAndroid have a getActivity() that returns the Context as > an > > > > > Activity if it actually is one. > > > > > > > > Haven't looked at this in detail, but just wanted to note that in WebView, > > the > > > > Context objects we use are *never* Activity instances, > > > > Not quite: Webview does dig the activity out: > > > https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja... > > > > So if an activity is needed, we have one. But if only a context is needed, we > > should return the wrapped context. Super complicated.. > > Ugh. Oh dear. This is not really okay, I think? The problem is that when an > activity is needed, if we dig the activity out and *then* whatever we needed an > activity for ends up referencing resources it will use the wrong context and > fail, but if we *don't* dig the activity out then we can't do activity things > with it :( > > Seems like there might be cases where this might be impossible. Your concern here was definitely valid. Maybe a compromise is never return Activity (since client can use it as context). But can expose a getContext class that returns the wrapped context, and if something needs to dig the activity out of it, it still can. > > > > they are always a > > > > ContextWrapper (to handle WebView resources). The underlying wrapped > context > > > > might be an Activity, but we mustn't use that Activity directly because > > > webview > > > > resource references will fail and crash the app. Not sure what that > implies > > > for > > > > this change :/ > > > > > > Good point. That should be okay. WebView doesn't build the > > > ActivityWindowAndroid, > > > > So same thing, webview does construct ActivityWindowAndroid objects. > > > > > so it wouldn't expose the getActivity() method that does > > > the casting AFAIK. Either way that method should do some nice checking and > > > return null if it's not an Activity. It's a WeakRef so that could happen in > > > valid use cases anyway :).
Just want to note that Torne's concern about using a dug out Activity is valid but we do not aim to fix this concern in this CL (instead, regarding that concern we simply aim to not worsen the current situation). https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:145: // context could be an Activity Context so we can't hold a strong reference to it. On 2015/10/29 22:39:19, Ted C wrote: > this comment is probably the same if it's a presentation right? > > We want to avoid strong references to anything that does not have the > application lifetime. Done. https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1419843002/diff/80001/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:59: private static ResourceManager create(Context context, long staticResourceManagerPtr) { On 2015/10/29 22:39:19, Ted C wrote: > why not make this take a WindowAndroid? I would rather make that change then > expose the context in native at all. Done. https://codereview.chromium.org/1419843002/diff/80001/ui/android/window_andro... File ui/android/window_android.h (right): https://codereview.chromium.org/1419843002/diff/80001/ui/android/window_andro... ui/android/window_android.h:72: // This is public only for use from ResourceManager On 2015/10/29 22:39:19, Ted C wrote: > We specifically wanted to avoid exposing the context to anyone as it is > extremely easy to get the lifecycle wrong. > > Despite having this comment, having this method here allows easy abuse and I > doubt it will stop anyone. > > I think to keep it consistent with the ActivityWindowAndroid implementation, it > should return a JavaObjectWeakGlobalRef with very clear documentation that it > should never be held on to longer than the lifetime of the context (whatever > that may be). For example, I know nothing of a Presentation context and do not > know if the resource manager lifecycle will match it exactly. > > If it outlives it, then we're going to hold onto that and leak everything > attached to it. Done.
lgtm, but wait for ted and boliu.
lgtm https://chromiumcodereview.appspot.com/1419843002/diff/100001/ui/android/java... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://chromiumcodereview.appspot.com/1419843002/diff/100001/ui/android/java... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:212: if (context instanceof Activity) return new WeakReference<Activity>((Activity) context); I would just add a warning suppression and do the cast without the if check. If this is ever not the case, then it is a programming mistake that should crash early and obviously https://chromiumcodereview.appspot.com/1419843002/diff/100001/ui/android/java... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://chromiumcodereview.appspot.com/1419843002/diff/100001/ui/android/java... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:80: // To render properly on an external display through a Presentation we need to fetch This comment seems more like the CL description than what should be in this file. This is just the reference to the context and it is a weak ref to avoid leaking. Why it is needed seems like something that will quickly go out of date when the next caller uses getContext() for something else. Then they're left wondering if it should only be used if it's a Presentation, etc..., etc... https://chromiumcodereview.appspot.com/1419843002/diff/100001/ui/android/java... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:598: public WeakReference<Context> getContext() { add javadoc (probably should add a warning that makes it clear that the caller should make no assumptions what type of context this is i.e. an activity, etc...) https://chromiumcodereview.appspot.com/1419843002/diff/100001/ui/android/java... File ui/android/java/src/org/chromium/ui/resources/system/SystemResourceLoader.java (right): https://chromiumcodereview.appspot.com/1419843002/diff/100001/ui/android/java... ui/android/java/src/org/chromium/ui/resources/system/SystemResourceLoader.java:31: * @param minScreenSideLength The length of the smallest side of the screen. I would suffix this with either Dp or Px depending on what it is
https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:212: if (context instanceof Activity) return new WeakReference<Activity>((Activity) context); On 2015/11/11 00:19:45, Ted C (OOO 11.11 - 11.12) wrote: > I would just add a warning suppression and do the cast without the if check. > > If this is ever not the case, then it is a programming mistake that should crash > early and obviously Hmm, not as simple as that. What if context is null? In that case, instanceof returns false, and the "Should never happen" actually happens. *However*, I do second crashing if it is non-null and non-Activity, and also suggest we do that check early in the constructor, rather than here.
https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:212: if (context instanceof Activity) return new WeakReference<Activity>((Activity) context); On 2015/11/11 00:34:08, boliu wrote: > On 2015/11/11 00:19:45, Ted C (OOO 11.11 - 11.12) wrote: > > I would just add a warning suppression and do the cast without the if check. > > > > If this is ever not the case, then it is a programming mistake that should > crash > > early and obviously > > Hmm, not as simple as that. What if context is null? In that case, instanceof > returns false, and the "Should never happen" actually happens. We specifically don't care about the null case though right? You can cast null to any object type without failure, so null being cast to an Activity (even if the original context was incorrectly the application context for whatever reason) will still work. > > *However*, I do second crashing if it is non-null and non-Activity, and also > suggest we do that check early in the constructor, rather than here.
https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:212: if (context instanceof Activity) return new WeakReference<Activity>((Activity) context); On 2015/11/11 00:44:25, Ted C (OOO 11.11 - 11.12) wrote: > On 2015/11/11 00:34:08, boliu wrote: > > On 2015/11/11 00:19:45, Ted C (OOO 11.11 - 11.12) wrote: > > > I would just add a warning suppression and do the cast without the if check. > > > > > > > If this is ever not the case, then it is a programming mistake that should > > crash > > > early and obviously > > > > Hmm, not as simple as that. What if context is null? In that case, instanceof > > returns false, and the "Should never happen" actually happens. > > We specifically don't care about the null case though right? > > You can cast null to any object type without failure, so null being cast to an > Activity (even if the original context was incorrectly the application context > for whatever reason) will still work. Sure. But in that case, should getActivity return null, or WeakReference<Activity>(null)? I think it should the latter one. > > > > > *However*, I do second crashing if it is non-null and non-Activity, and also > > suggest we do that check early in the constructor, rather than here. >
https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:212: if (context instanceof Activity) return new WeakReference<Activity>((Activity) context); On 2015/11/11 00:46:29, boliu wrote: > On 2015/11/11 00:44:25, Ted C (OOO 11.11 - 11.12) wrote: > > On 2015/11/11 00:34:08, boliu wrote: > > > On 2015/11/11 00:19:45, Ted C (OOO 11.11 - 11.12) wrote: > > > > I would just add a warning suppression and do the cast without the if > check. > > > > > > > > > > If this is ever not the case, then it is a programming mistake that should > > > crash > > > > early and obviously > > > > > > Hmm, not as simple as that. What if context is null? In that case, > instanceof > > > returns false, and the "Should never happen" actually happens. > > > > We specifically don't care about the null case though right? > > > > You can cast null to any object type without failure, so null being cast to an > > Activity (even if the original context was incorrectly the application context > > for whatever reason) will still work. > > Sure. But in that case, should getActivity return null, or > WeakReference<Activity>(null)? I think it should the latter one. Definitely the latter. But my original suggestion was that this method simply be: return new WeakReference<Activity>((Activity) getContext().get()); That will handle either getContext() referencing an activity or null whatever and crash otherwise. > > > > > > > > > *However*, I do second crashing if it is non-null and non-Activity, and also > > > suggest we do that check early in the constructor, rather than here. > > >
https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:212: if (context instanceof Activity) return new WeakReference<Activity>((Activity) context); On 2015/11/11 00:48:41, Ted C (OOO 11.11 - 11.12) wrote: > On 2015/11/11 00:46:29, boliu wrote: > > On 2015/11/11 00:44:25, Ted C (OOO 11.11 - 11.12) wrote: > > > On 2015/11/11 00:34:08, boliu wrote: > > > > On 2015/11/11 00:19:45, Ted C (OOO 11.11 - 11.12) wrote: > > > > > I would just add a warning suppression and do the cast without the if > > check. > > > > > > > > > > > > > If this is ever not the case, then it is a programming mistake that > should > > > > crash > > > > > early and obviously > > > > > > > > Hmm, not as simple as that. What if context is null? In that case, > > instanceof > > > > returns false, and the "Should never happen" actually happens. > > > > > > We specifically don't care about the null case though right? > > > > > > You can cast null to any object type without failure, so null being cast to > an > > > Activity (even if the original context was incorrectly the application > context > > > for whatever reason) will still work. > > > > Sure. But in that case, should getActivity return null, or > > WeakReference<Activity>(null)? I think it should the latter one. > > Definitely the latter. > > But my original suggestion was that this method simply be: > > return new WeakReference<Activity>((Activity) getContext().get()); that sgtm > > That will handle either getContext() referencing an activity or null whatever > and crash otherwise. > > > > > > > > > > > > > > *However*, I do second crashing if it is non-null and non-Activity, and > also > > > > suggest we do that check early in the constructor, rather than here. > > > > > >
https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/ActivityWindowAndroid.java:212: if (context instanceof Activity) return new WeakReference<Activity>((Activity) context); On 2015/11/11 01:01:20, boliu wrote: > On 2015/11/11 00:48:41, Ted C (OOO 11.11 - 11.12) wrote: > > On 2015/11/11 00:46:29, boliu wrote: > > > On 2015/11/11 00:44:25, Ted C (OOO 11.11 - 11.12) wrote: > > > > On 2015/11/11 00:34:08, boliu wrote: > > > > > On 2015/11/11 00:19:45, Ted C (OOO 11.11 - 11.12) wrote: > > > > > > I would just add a warning suppression and do the cast without the if > > > check. > > > > > > > > > > > > > > > > If this is ever not the case, then it is a programming mistake that > > should > > > > > crash > > > > > > early and obviously > > > > > > > > > > Hmm, not as simple as that. What if context is null? In that case, > > > instanceof > > > > > returns false, and the "Should never happen" actually happens. > > > > > > > > We specifically don't care about the null case though right? > > > > > > > > You can cast null to any object type without failure, so null being cast > to > > an > > > > Activity (even if the original context was incorrectly the application > > context > > > > for whatever reason) will still work. > > > > > > Sure. But in that case, should getActivity return null, or > > > WeakReference<Activity>(null)? I think it should the latter one. > > > > Definitely the latter. > > > > But my original suggestion was that this method simply be: > > > > return new WeakReference<Activity>((Activity) getContext().get()); > > that sgtm > > > > > That will handle either getContext() referencing an activity or null whatever > > and crash otherwise. > > > > > > > > > > > > > > > > > > > *However*, I do second crashing if it is non-null and non-Activity, and > > also > > > > > suggest we do that check early in the constructor, rather than here. > > > > > > > > > > Done. https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right): https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:80: // To render properly on an external display through a Presentation we need to fetch On 2015/11/11 00:19:45, Ted C (OOO 11.11 - 11.12) wrote: > This comment seems more like the CL description than what should be in this > file. > > This is just the reference to the context and it is a weak ref to avoid leaking. > > Why it is needed seems like something that will quickly go out of date when the > next caller uses getContext() for something else. Then they're left wondering > if it should only be used if it's a Presentation, etc..., etc... Done. https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:598: public WeakReference<Context> getContext() { On 2015/11/11 00:19:45, Ted C (OOO 11.11 - 11.12) wrote: > add javadoc (probably should add a warning that makes it clear that the caller > should make no assumptions what type of context this is i.e. an activity, > etc...) Done. https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/system/SystemResourceLoader.java (right): https://codereview.chromium.org/1419843002/diff/100001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/system/SystemResourceLoader.java:31: * @param minScreenSideLength The length of the smallest side of the screen. On 2015/11/11 00:19:45, Ted C (OOO 11.11 - 11.12) wrote: > I would suffix this with either Dp or Px depending on what it is Done.
lgtm still
The CQ bit was checked by gsennton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, tedchoc@chromium.org, dtrainor@chromium.org Link to the patchset: https://codereview.chromium.org/1419843002/#ps120001 (title: "Cast Context to Activity without check and fix some comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1419843002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1419843002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by gsennton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, dtrainor@chromium.org, sievers@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/1419843002/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1419843002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1419843002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
Hey again, so the failing bot was complaining about the dereferencing of a weak pointer not being null-checked (the Context in ResourceManager). So I have moved the dereferencing out to ResourceManager.create(...) which will now return null if the incoming Context is null. I also moved the logic for getting the minimum screen side length out from the ctor to ResourceManager.create so that we now don't pass the Context to the ctor (to make it less likely for someone to use the Context in the ctor in the future). I am still using DeviceDisplayInfo (in ResourceManager) instead of resources.getDisplayMetrics since the DeviceDisplayInfo methods check the Android version we are running: https://code.google.com/p/chromium/codesearch#chromium/src/ui/android/java/sr... to determine whether to fetch getSize or getRealSize. We could duplicate that logic in ResourceManager and fetch resources.getDisplayMetrics or resources.getRealDisplayMetrics depending on Android version...
https://codereview.chromium.org/1419843002/diff/160001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1419843002/diff/160001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:72: if (context == null) return null; from my brief glance, it doesn't look like native supports/handles this returning null I think throwing an IllegalStateException here would be clearer and make it easier to figure out what went wrong.
https://codereview.chromium.org/1419843002/diff/160001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1419843002/diff/160001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:72: if (context == null) return null; On 2015/11/16 16:51:54, Ted C wrote: > from my brief glance, it doesn't look like native supports/handles this > returning null > > I think throwing an IllegalStateException here would be clearer and make it > easier to figure out what went wrong. Done.
lgtm w/ nit https://codereview.chromium.org/1419843002/diff/180001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/resources/ResourceManager.java (right): https://codereview.chromium.org/1419843002/diff/180001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/resources/ResourceManager.java:72: if (context == null) braces always required in javaland (unless the condition and statement can all fit on a single line)
The CQ bit was checked by gsennton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dtrainor@chromium.org, sievers@chromium.org, boliu@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/1419843002/#ps200001 (title: "Fix nit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1419843002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1419843002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
The CQ bit was checked by gsennton@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, dtrainor@chromium.org, sievers@chromium.org, boliu@chromium.org Link to the patchset: https://codereview.chromium.org/1419843002/#ps220001 (title: "Fix failing bot.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1419843002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1419843002/220001
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/44b70448c14f6b0341d201c5444e99ee3d8b8357 Cr-Commit-Position: refs/heads/master@{#360066} |
