Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1378653002:
[Android] Enable ApplicationStatus for WebView. (Closed)
|
Created:
5 years, 2 months ago by timvolodine Modified:
5 years, 1 month ago CC:
android-webview-reviews_chromium.org, chromium-reviews, Bernhard Bauer Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Android] Enable ApplicationStatus for WebView.
Currently ApplicationStatus does not work with WebView. This patch
introduces a 'webview' mode to allow for a webview specific usage of
the ApplicationStatus class.
The 'webview' mode is needed because of the following:
- Current ApplicationStatus initialization is assumed to happen
inside the OnCreate method of the Application, in webview this can
happen anytime relative to the embedding application.
- ApplicationStatus.initialize takes a BaseChromiumApplication
as a parameter, while this should be Application in case of webview.
- In this patch webview mode does not listen to window focus events,
but this may be considered in the future.
- getApplicationContext should return wrapped application context in
case of webview.
BUG=470582
Patch Set 1 #
Total comments: 14
Patch Set 2 : address comments #
Total comments: 10
Patch Set 3 : address Bo's comments #
Total comments: 16
Patch Set 4 : add intial sCachedApplicationState #
Depends on Patchset: Messages
Total messages: 35 (4 generated)
timvolodine@chromium.org changed reviewers: + boliu@chromium.org
timvolodine@chromium.org changed reviewers: + dtrainor@chromium.org
+dtrainor@ as the original author of ApplicationStatus
an attempt to make it work on WebView. wdyt? in particular: - should this be part of ApplicationStatus in base or should be create a webview-specific ApplicationStatus class? - do we need to listen to focus changes in webview or can this be omitted for now?
On 2015/09/29 16:20:34, timvolodine wrote: > an attempt to make it work on WebView. wdyt? > > in particular: > - should this be part of ApplicationStatus in base or should be create a > webview-specific ApplicationStatus class? Base is fine imo. But I am a bit concerned that now chromium listens to *every single activity* in the app. Maybe we should have a webview implementation that only listens to activities that contains webviews. AwContents.sActivityWindowMap already does something like that. > - do we need to listen to focus changes in webview or can this be omitted for > now? So afaict, that's used only to support getLastTrackedFocusedActivity. Most uses of that is in chrome, but there is one usage from net. I think if webview wants to implement this, we should just override View.onWindowFocusChanged for the signal instead of doing what chrome does now. https://codereview.chromium.org/1378653002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/1378653002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:251: ApplicationStatus.initializeForWebView(mWebViewDelegate.getApplication()); Let's move the ApplicationStatus call to out of the glue layer. Mainly so that the instrumentatin shell can also initialize ApplicationStatus as well, eg maybe this should be merged into AwBrowserProcess.start. https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:73: new ConcurrentHashMap<Activity, ActivityInfo>(); dtrainor: why ConcurrentHashMap? Is this class not single threaded? Also this honestly scares me for potential of leaks, but ehh...I guess it's fine if listeners are working fine. https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:207: sActivityInfo.putIfAbsent(activity, new ActivityInfo()); This creates a new ActivityInfo object even if one is not needed. https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:278: return sApplication != null ? sApplication.getApplicationContext() : null; This is a problem. All of webview's Context objects are wrapped, to include webview resources. So it's not legal to do this here. Should pass in the wrapped context as well, and return that instead, and need to be careful never to use sApplication for context.
https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:73: new ConcurrentHashMap<Activity, ActivityInfo>(); On 2015/09/29 21:34:02, boliu wrote: > dtrainor: why ConcurrentHashMap? Is this class not single threaded? > > Also this honestly scares me for potential of leaks, but ehh...I guess it's fine > if listeners are working fine. IIRC callers don't have to be on the UI thread. The InvalidationService uses it for example (see crbug.com/403951).
https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:73: new ConcurrentHashMap<Activity, ActivityInfo>(); On 2015/09/29 23:02:11, David Trainor wrote: > On 2015/09/29 21:34:02, boliu wrote: > > dtrainor: why ConcurrentHashMap? Is this class not single threaded? > > > > Also this honestly scares me for potential of leaks, but ehh...I guess it's > fine > > if listeners are working fine. > > IIRC callers don't have to be on the UI thread. The InvalidationService uses it > for example (see crbug.com/403951). so this class is partially thread safe? The part about accessing the state of an activity is thread safe, but the listeners are not o_O (Base has a global thread safe object that doesn't work on android webview, :( )
dgn@chromium.org changed reviewers: + dgn@chromium.org
Thanks for your work on that. Our CLs can land independently without any issue so that's great. https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:67: private static boolean sWebViewMode = false; Could you please add more explanations on how WebView mode is different? Why the distinction is needed, what would not work, etc. Maybe add checks to throw in methods not supported in webview mode? If it's only going to be used only for a single thing, it's maybe better to have a more descriptive/focused name? (it could be useful beyond webview some day?) https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:185: initializeLifecycleCallbacks(application); This is going to miss a cycle of Created/Resumed events at registration, right? Shouldn't they be explicitly called instead of using the flag to only simulate the CREATED event on the first activity? This would allow other stuff to run in those hooks. But maybe entirely skipping them is better than calling them at the wrong moment.
https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:73: new ConcurrentHashMap<Activity, ActivityInfo>(); On 2015/09/29 23:21:45, boliu wrote: > On 2015/09/29 23:02:11, David Trainor wrote: > > On 2015/09/29 21:34:02, boliu wrote: > > > dtrainor: why ConcurrentHashMap? Is this class not single threaded? > > > > > > Also this honestly scares me for potential of leaks, but ehh...I guess it's > > fine > > > if listeners are working fine. > > > > IIRC callers don't have to be on the UI thread. The InvalidationService uses > it > > for example (see crbug.com/403951). > > so this class is partially thread safe? The part about accessing the state of an > activity is thread safe, but the listeners are not o_O Sadly yeah :(. I think we made the smallest change we had to to fix a bug before release but this apparently fell off my radar and I didn't follow up to make the whole class thread safe :(. > > (Base has a global thread safe object that doesn't work on android webview, :( ) ?
addressed comments, PTAL! https://codereview.chromium.org/1378653002/diff/1/android_webview/glue/java/s... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/1378653002/diff/1/android_webview/glue/java/s... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:251: ApplicationStatus.initializeForWebView(mWebViewDelegate.getApplication()); On 2015/09/29 21:34:02, boliu wrote: > Let's move the ApplicationStatus call to out of the glue layer. Mainly so that > the instrumentatin shell can also initialize ApplicationStatus as well, eg maybe > this should be merged into AwBrowserProcess.start. ok, I've moved this to a static method in AwBrowserProcess, seems most flexible that way. https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:67: private static boolean sWebViewMode = false; On 2015/09/30 11:02:55, dgn wrote: > Could you please add more explanations on how WebView mode is different? Why the > distinction is needed, what would not work, etc. Maybe add checks to throw in > methods not supported in webview mode? If it's only going to be used only for a > single thing, it's maybe better to have a more descriptive/focused name? (it > could be useful beyond webview some day?) I've added explanations of differences in the initializeForWebView method and made this comment point there. Also added some asserts specific to webview mode. https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:185: initializeLifecycleCallbacks(application); On 2015/09/30 11:02:55, dgn wrote: > This is going to miss a cycle of Created/Resumed events at registration, right? > Shouldn't they be explicitly called instead of using the flag to only simulate > the CREATED event on the first activity? This would allow other stuff to run in > those hooks. But maybe entirely skipping them is better than calling them at the > wrong moment. hmm not sure what you mean here. Basically in webview mode the 'created' event can be skipped because the activity may already have been created before the webview is started. https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:207: sActivityInfo.putIfAbsent(activity, new ActivityInfo()); On 2015/09/29 21:34:02, boliu wrote: > This creates a new ActivityInfo object even if one is not needed. right, my bad, done. https://codereview.chromium.org/1378653002/diff/1/base/android/java/src/org/c... base/android/java/src/org/chromium/base/ApplicationStatus.java:278: return sApplication != null ? sApplication.getApplicationContext() : null; On 2015/09/29 21:34:02, boliu wrote: > This is a problem. > > All of webview's Context objects are wrapped, to include webview resources. So > it's not legal to do this here. > > Should pass in the wrapped context as well, and return that instead, and need to > be careful never to use sApplication for context. passing the wrapped context in 'webview' mode now as you suggested. sApplication is only really used in this method so should not be mixed up as context anywhere. Done.
> > - should this be part of ApplicationStatus in base or should be create a > > webview-specific ApplicationStatus class? > > Base is fine imo. But I am a bit concerned that now chromium listens to *every > single activity* in the app. Maybe we should have a webview implementation that > only listens to activities that contains webviews. AwContents.sActivityWindowMap > already does something like that. > Yes having a signal when there are actual webviews is (even more) useful. However I think we still need the base functionality of this class for webview. In particular some 'global' functionality like NetworkChangeNotifier do not seem to be designed with webview instances in mind. Perhaps the webview signals can then be implemented on top of ApplicationStatus where necessary. > > - do we need to listen to focus changes in webview or can this be omitted for > > now? > > So afaict, that's used only to support getLastTrackedFocusedActivity. Most uses > of that is in chrome, but there is one usage from net. > > I think if webview wants to implement this, we should just override > View.onWindowFocusChanged for the signal instead of doing what chrome does now. ok I've found two non-chrome/ classes that use getLastTrackedFocuesActivity: HttpNegotiateAuthenticator.java and ScreenOrientationProvider.java. This looks like a separate issue and I've filed a crbug.com/538175 to keep track of that. In the meantime getLastTrackedFocuesActivity will keep returning null.
timvolodine@chromium.org changed reviewers: + rmcilroy@chromium.org
+rmcilroy@ as base/android/ owner
On 2015/10/01 15:53:53, timvolodine wrote: > +rmcilroy@ as base/android/ owner base/android lgtm.
Haven't really addressed the issue that all activities are monitored, not just the ones that contain webviews. But that might involve a brand new ApplicationStatus implementation for webview, so... dunno. Debatable. https://codereview.chromium.org/1378653002/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/1378653002/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:250: AwBrowserProcess.initApplicationStatus(mWebViewDelegate.getApplication(), merge into start? AwBrowserProcess.start? :) https://codereview.chromium.org/1378653002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/1378653002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:49: public static void initApplicationStatus(Application application, Context wrappedContext) { actually moving this into start was a *really* good idea, revealed that this might not be called on the UI thread o_O https://codereview.chromium.org/1378653002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:70: private static boolean sWebViewMode = false; +1 to a more contained name, instead of "isWebView". We generally avoid having any code know explicitly it's running webview vs chrome, because the more things will start depending on it, webview will diverge from chrome. I'm bad at names, maybe ehh... IsApplicationBoundAfterOnCreate, or something like that..? https://codereview.chromium.org/1378653002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:71: private static Context sWrappedApplicationContext; "Wrapped" is meaningless in base, so just sApplicationContext is fine. But do leave a comment that only use this as context, instead of casting from sApplication, since they may not always be the same object https://codereview.chromium.org/1378653002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:299: return sWebViewMode ? sWrappedApplicationContext : sApplication.getApplicationContext(); See, webview mode is already creeping in. Chrome should just set sApplicationContext to to sApplication.getApplicationContext in init. Then can just return sApplicationContext here.
thanks for the comments Bo, please take another look https://codereview.chromium.org/1378653002/diff/20001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/1378653002/diff/20001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:250: AwBrowserProcess.initApplicationStatus(mWebViewDelegate.getApplication(), On 2015/10/01 16:59:01, boliu wrote: > merge into start? AwBrowserProcess.start? :) Done. https://codereview.chromium.org/1378653002/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/1378653002/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:49: public static void initApplicationStatus(Application application, Context wrappedContext) { On 2015/10/01 16:59:01, boliu wrote: > actually moving this into start was a *really* good idea, revealed that this > might not be called on the UI thread o_O Done. https://codereview.chromium.org/1378653002/diff/20001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:70: private static boolean sWebViewMode = false; On 2015/10/01 16:59:02, boliu wrote: > +1 to a more contained name, instead of "isWebView". We generally avoid having > any code know explicitly it's running webview vs chrome, because the more things > will start depending on it, webview will diverge from chrome. > > I'm bad at names, maybe ehh... IsApplicationBoundAfterOnCreate, or something > like that..? went for sActivityLifecycleIndependentMode https://codereview.chromium.org/1378653002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:71: private static Context sWrappedApplicationContext; On 2015/10/01 16:59:02, boliu wrote: > "Wrapped" is meaningless in base, so just sApplicationContext is fine. But do > leave a comment that only use this as context, instead of casting from > sApplication, since they may not always be the same object Done. https://codereview.chromium.org/1378653002/diff/20001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:299: return sWebViewMode ? sWrappedApplicationContext : sApplication.getApplicationContext(); On 2015/10/01 16:59:02, boliu wrote: > See, webview mode is already creeping in. > > Chrome should just set sApplicationContext to to > sApplication.getApplicationContext in init. Then can just return > sApplicationContext here. right, done.
lgtm, but wait for boliu.
https://codereview.chromium.org/1378653002/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/1378653002/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:78: public static void start(final Context context) { Fix all the callers instead? It's not like this needs to coordinate with downstream glue or anything.. https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { since my non-inline comment were ignored twice, inline this time.. What's the story for chromium listening to the lifetime events of every single activity in the app, instead of just the ones that has webview created in them? Do we believe this is ok? Also does all listeners deal well with say, not seeing the CREATE state of an activity?
https://codereview.chromium.org/1378653002/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/1378653002/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:78: public static void start(final Context context) { On 2015/10/02 15:51:32, boliu (slow to review) wrote: > Fix all the callers instead? It's not like this needs to coordinate with > downstream glue or anything.. yes can do that, just thought start(.., null) in test classes would be a bit ugly https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { On 2015/10/02 15:51:32, boliu (slow to review) wrote: > since my non-inline comment were ignored twice, inline this time.. > > What's the story for chromium listening to the lifetime events of every single > activity in the app, instead of just the ones that has webview created in them? > Do we believe this is ok? > Bo, I didn't ignore your comments and I thought I answered the first one already. Basically I think the webview-specific 'application status' could be implemented in android_webview on top of this class. This could use the AwWebViewLifecycleObserver for example. Also it appears some functionality like NetworkChangeNotifier would not work out of the box with an 'ApplicationStatus' that would depend on straightforward webview creation (see crrev.com/1297943003/). > Also does all listeners deal well with say, not seeing the CREATE state of an > activity? Regarding missing the ActivityState.CREATED is not really a problem as it is followed by STARTED and RESUMED which will make sure the ApplicationState is set accordingly. The listeners would then correctly receive application state changes. However I did some further experimentation regarding the possibility of missing all of the CREATED, STARTED, RESUMED activity signals which appears to be possible using this somewhat far-fetched snippet. @Override protected void onResume() { super.onResume(); setContentView(R.layout.activity_webview_browser); } Which means in that particular case that on startup webview will think the Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To address this possibility I think we can set sCachedApplicationState to HAS_RUNNING_ACTIVITIES inside the initializeActivityIndependent() method. The assumption is that when webview is initialized the Activity is either running or will be running very soon. wdyt?
https://codereview.chromium.org/1378653002/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/1378653002/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:78: public static void start(final Context context) { On 2015/10/08 16:13:55, timvolodine wrote: > On 2015/10/02 15:51:32, boliu (slow to review) wrote: > > Fix all the callers instead? It's not like this needs to coordinate with > > downstream glue or anything.. > > yes can do that, just thought start(.., null) in test classes would be a bit > ugly having production code be pretty is a good tradeoff for testing code be ugly Also, why does it need to be null? Instrumentation tests have an application, no? https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { On 2015/10/08 16:13:55, timvolodine wrote: > On 2015/10/02 15:51:32, boliu (slow to review) wrote: > > since my non-inline comment were ignored twice, inline this time.. > > > > What's the story for chromium listening to the lifetime events of every single > > activity in the app, instead of just the ones that has webview created in > them? > > Do we believe this is ok? > > > > Bo, I didn't ignore your comments and I thought I answered the first one > already. Basically I think the webview-specific 'application status' could be > implemented in android_webview on top of this class. This could use the > AwWebViewLifecycleObserver for example. > Also it appears some functionality like NetworkChangeNotifier would not work out > of the box with an 'ApplicationStatus' that would depend on straightforward > webview creation (see crrev.com/1297943003/). > This doesn't really answer whether this patch is safe, ie what kind of thing does observers do right now that could have a negative impact to apps? It's probably ok, but want to check. Regarding webview specific implementation, I was thinking ApplicationStatus becomes an interface, and there's the default implementation chrome uses, and then there's the webview-specific implementation that webview sets on init. > > Also does all listeners deal well with say, not seeing the CREATE state of an > > activity? > > Regarding missing the ActivityState.CREATED is not really a problem as it is > followed by STARTED and RESUMED which will make sure the ApplicationState is set > accordingly. The listeners would then correctly receive application state > changes. > > However I did some further experimentation regarding the possibility of missing > all of the CREATED, STARTED, RESUMED activity signals which appears to be > possible using this somewhat far-fetched snippet. > > @Override > protected void onResume() { > super.onResume(); > setContentView(R.layout.activity_webview_browser); > } By this example, You mean that something _real important_ happens in onResume, right? Do we already know nothing _real important_ happens in onCreate/onStart? > > Which means in that particular case that on startup webview will think the > Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To address this > possibility I think we can set sCachedApplicationState to HAS_RUNNING_ACTIVITIES > inside the initializeActivityIndependent() method. The assumption is that when > webview is initialized the Activity is either running or will be running very > soon. wdyt? This is a question for base owner. But my thought is that's a bad idea since that's by definition inconsistent, since sActivity is still null. My thought would be directly insert the activity used to create a webview, as soon as the webview is inserted. Then you can transition to a non-destroyed but still consistent state, and maybe also send fake notifications like onCreate so the observers are consistent too.
https://codereview.chromium.org/1378653002/diff/40001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/1378653002/diff/40001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:78: public static void start(final Context context) { On 2015/10/08 18:01:30, boliu (slow to review) wrote: > On 2015/10/08 16:13:55, timvolodine wrote: > > On 2015/10/02 15:51:32, boliu (slow to review) wrote: > > > Fix all the callers instead? It's not like this needs to coordinate with > > > downstream glue or anything.. > > > > yes can do that, just thought start(.., null) in test classes would be a bit > > ugly > > having production code be pretty is a good tradeoff for testing code be ugly > > Also, why does it need to be null? Instrumentation tests have an application, > no? right, it's currently disabled but I guess we could enable it in tests. https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { On 2015/10/08 18:01:30, boliu (slow to review) wrote: > On 2015/10/08 16:13:55, timvolodine wrote: > > On 2015/10/02 15:51:32, boliu (slow to review) wrote: > > > since my non-inline comment were ignored twice, inline this time.. > > > > > > What's the story for chromium listening to the lifetime events of every > single > > > activity in the app, instead of just the ones that has webview created in > > them? > > > Do we believe this is ok? > > > > > > > Bo, I didn't ignore your comments and I thought I answered the first one > > already. Basically I think the webview-specific 'application status' could be > > implemented in android_webview on top of this class. This could use the > > AwWebViewLifecycleObserver for example. > > Also it appears some functionality like NetworkChangeNotifier would not work > out > > of the box with an 'ApplicationStatus' that would depend on straightforward > > webview creation (see crrev.com/1297943003/). > > > > This doesn't really answer whether this patch is safe, ie what kind of thing > does observers do right now that could have a negative impact to apps? It's > probably ok, but want to check. yeah, I have a list of classes using it, need to have a closer look. > > Regarding webview specific implementation, I was thinking ApplicationStatus > becomes an interface, and there's the default implementation chrome uses, and > then there's the webview-specific implementation that webview sets on init. yes, but I don't think it is essential here. We can always make an interface and set implementations instead of sActivityLifecycleIndependentMode boolean. The question of correct initialization still stands. > > > Also does all listeners deal well with say, not seeing the CREATE state of > an > > > activity? > > > > Regarding missing the ActivityState.CREATED is not really a problem as it is > > followed by STARTED and RESUMED which will make sure the ApplicationState is > set > > accordingly. The listeners would then correctly receive application state > > changes. > > > > However I did some further experimentation regarding the possibility of > missing > > all of the CREATED, STARTED, RESUMED activity signals which appears to be > > possible using this somewhat far-fetched snippet. > > > > @Override > > protected void onResume() { > > super.onResume(); > > setContentView(R.layout.activity_webview_browser); > > } > > By this example, You mean that something _real important_ happens in onResume, > right? Do we already know nothing _real important_ happens in onCreate/onStart? setContentView triggers AwBrowserProcess.start and hence ApplicationStatus.initializeActivityIndependent() but at this stage no activity state changes will be observed as it happens after CREATED,STARTED,RESUMED have already happened. If this happens in onStart, or onCreate there is no such problem. > > > > > Which means in that particular case that on startup webview will think the > > Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To address > this > > possibility I think we can set sCachedApplicationState to > HAS_RUNNING_ACTIVITIES > > inside the initializeActivityIndependent() method. The assumption is that when > > webview is initialized the Activity is either running or will be running very > > soon. wdyt? > > This is a question for base owner. But my thought is that's a bad idea since > that's by definition inconsistent, since sActivity is still null. > > My thought would be directly insert the activity used to create a webview, as > soon as the webview is inserted. Then you can transition to a non-destroyed but > still consistent state, and maybe also send fake notifications like onCreate so > the observers are consistent too. Hmm, the problem is that at initialization of ApplicationStatus we don't really have an Activity available to init with. Maybe this can be resolved using ActivityThread but that seems a bit hacky (but maybe the only option?). Also, making ApplicationStatus depend on actual webview creation (i.e. AwContents) is not exactly the same and would actually be something different than 'ApplicationStatus'.
https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { On 2015/10/09 15:46:36, timvolodine wrote: > On 2015/10/08 18:01:30, boliu (slow to review) wrote: > > On 2015/10/08 16:13:55, timvolodine wrote: > > > On 2015/10/02 15:51:32, boliu (slow to review) wrote: > > > > since my non-inline comment were ignored twice, inline this time.. > > > > > > > > What's the story for chromium listening to the lifetime events of every > > single > > > > activity in the app, instead of just the ones that has webview created in > > > them? > > > > Do we believe this is ok? > > > > > > > > > > Bo, I didn't ignore your comments and I thought I answered the first one > > > already. Basically I think the webview-specific 'application status' could > be > > > implemented in android_webview on top of this class. This could use the > > > AwWebViewLifecycleObserver for example. > > > Also it appears some functionality like NetworkChangeNotifier would not work > > out > > > of the box with an 'ApplicationStatus' that would depend on straightforward > > > webview creation (see crrev.com/1297943003/). > > > > > > > This doesn't really answer whether this patch is safe, ie what kind of thing > > does observers do right now that could have a negative impact to apps? It's > > probably ok, but want to check. > > yeah, I have a list of classes using it, need to have a closer look. > > > > > Regarding webview specific implementation, I was thinking ApplicationStatus > > becomes an interface, and there's the default implementation chrome uses, and > > then there's the webview-specific implementation that webview sets on init. > > yes, but I don't think it is essential here. We can always make an interface and > set implementations instead of sActivityLifecycleIndependentMode boolean. The > question of correct initialization still stands. > > > > > Also does all listeners deal well with say, not seeing the CREATE state of > > an > > > > activity? > > > > > > Regarding missing the ActivityState.CREATED is not really a problem as it is > > > followed by STARTED and RESUMED which will make sure the ApplicationState is > > set > > > accordingly. The listeners would then correctly receive application state > > > changes. > > > > > > However I did some further experimentation regarding the possibility of > > missing > > > all of the CREATED, STARTED, RESUMED activity signals which appears to be > > > possible using this somewhat far-fetched snippet. > > > > > > @Override > > > protected void onResume() { > > > super.onResume(); > > > setContentView(R.layout.activity_webview_browser); > > > } > > > > By this example, You mean that something _real important_ happens in onResume, > > right? Do we already know nothing _real important_ happens in > onCreate/onStart? > > setContentView triggers AwBrowserProcess.start and hence > ApplicationStatus.initializeActivityIndependent() but at this stage no activity > state changes will be observed as it happens after CREATED,STARTED,RESUMED have > already happened. If this happens in onStart, or onCreate there is no such > problem. Ahh ok. That can definitely happen with real apps too (ie they create webview much later after activity has been active, think which only creates webview when you open a thread) > > > > > > > > > Which means in that particular case that on startup webview will think the > > > Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To address > > this > > > possibility I think we can set sCachedApplicationState to > > HAS_RUNNING_ACTIVITIES > > > inside the initializeActivityIndependent() method. The assumption is that > when > > > webview is initialized the Activity is either running or will be running > very > > > soon. wdyt? > > > > This is a question for base owner. But my thought is that's a bad idea since > > that's by definition inconsistent, since sActivity is still null. > > > > My thought would be directly insert the activity used to create a webview, as > > soon as the webview is inserted. Then you can transition to a non-destroyed > but > > still consistent state, and maybe also send fake notifications like onCreate > so > > the observers are consistent too. > > Hmm, the problem is that at initialization of ApplicationStatus we don't really > have an Activity available to init with. Maybe this can be resolved using > ActivityThread but that seems a bit hacky (but maybe the only option?). Also, > making ApplicationStatus depend on actual webview creation (i.e. AwContents) is > not exactly the same and would actually be something different than > 'ApplicationStatus'. Right. I mean supply the activity in AwContents constructor, which is after initializeActivityIndependent. The state will be DESTROYED immediately after initializeActivityIndependent. But presumably AwContents constructor will be called soon, and if that has an activity, then give that to ApplicationStatus to track.
https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { On 2015/10/09 15:58:57, boliu (slow to review) wrote: > On 2015/10/09 15:46:36, timvolodine wrote: > > On 2015/10/08 18:01:30, boliu (slow to review) wrote: > > > On 2015/10/08 16:13:55, timvolodine wrote: > > > > On 2015/10/02 15:51:32, boliu (slow to review) wrote: > > > > > since my non-inline comment were ignored twice, inline this time.. > > > > > > > > > > What's the story for chromium listening to the lifetime events of every > > > single > > > > > activity in the app, instead of just the ones that has webview created > in > > > > them? > > > > > Do we believe this is ok? > > > > > > > > > > > > > Bo, I didn't ignore your comments and I thought I answered the first one > > > > already. Basically I think the webview-specific 'application status' could > > be > > > > implemented in android_webview on top of this class. This could use the > > > > AwWebViewLifecycleObserver for example. > > > > Also it appears some functionality like NetworkChangeNotifier would not > work > > > out > > > > of the box with an 'ApplicationStatus' that would depend on > straightforward > > > > webview creation (see crrev.com/1297943003/). > > > > > > > > > > This doesn't really answer whether this patch is safe, ie what kind of thing > > > does observers do right now that could have a negative impact to apps? It's > > > probably ok, but want to check. > > > > yeah, I have a list of classes using it, need to have a closer look. > > > > > > > > Regarding webview specific implementation, I was thinking ApplicationStatus > > > becomes an interface, and there's the default implementation chrome uses, > and > > > then there's the webview-specific implementation that webview sets on init. > > > > yes, but I don't think it is essential here. We can always make an interface > and > > set implementations instead of sActivityLifecycleIndependentMode boolean. The > > question of correct initialization still stands. > > > > > > > Also does all listeners deal well with say, not seeing the CREATE state > of > > > an > > > > > activity? > > > > > > > > Regarding missing the ActivityState.CREATED is not really a problem as it > is > > > > followed by STARTED and RESUMED which will make sure the ApplicationState > is > > > set > > > > accordingly. The listeners would then correctly receive application state > > > > changes. > > > > > > > > However I did some further experimentation regarding the possibility of > > > missing > > > > all of the CREATED, STARTED, RESUMED activity signals which appears to be > > > > possible using this somewhat far-fetched snippet. > > > > > > > > @Override > > > > protected void onResume() { > > > > super.onResume(); > > > > setContentView(R.layout.activity_webview_browser); > > > > } > > > > > > By this example, You mean that something _real important_ happens in > onResume, > > > right? Do we already know nothing _real important_ happens in > > onCreate/onStart? > > > > setContentView triggers AwBrowserProcess.start and hence > > ApplicationStatus.initializeActivityIndependent() but at this stage no > activity > > state changes will be observed as it happens after CREATED,STARTED,RESUMED > have > > already happened. If this happens in onStart, or onCreate there is no such > > problem. > > Ahh ok. That can definitely happen with real apps too (ie they create webview > much later after activity has been active, think which only creates webview when > you open a thread) > > > > > > > > > > > > > > Which means in that particular case that on startup webview will think the > > > > Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To > address > > > this > > > > possibility I think we can set sCachedApplicationState to > > > HAS_RUNNING_ACTIVITIES > > > > inside the initializeActivityIndependent() method. The assumption is that > > when > > > > webview is initialized the Activity is either running or will be running > > very > > > > soon. wdyt? > > > > > > This is a question for base owner. But my thought is that's a bad idea since > > > that's by definition inconsistent, since sActivity is still null. > > > > > > My thought would be directly insert the activity used to create a webview, > as > > > soon as the webview is inserted. Then you can transition to a non-destroyed > > but > > > still consistent state, and maybe also send fake notifications like onCreate > > so > > > the observers are consistent too. > > > > Hmm, the problem is that at initialization of ApplicationStatus we don't > really > > have an Activity available to init with. Maybe this can be resolved using > > ActivityThread but that seems a bit hacky (but maybe the only option?). Also, > > making ApplicationStatus depend on actual webview creation (i.e. AwContents) > is > > not exactly the same and would actually be something different than > > 'ApplicationStatus'. > > Right. I mean supply the activity in AwContents constructor, which is after > initializeActivityIndependent. The state will be DESTROYED immediately after > initializeActivityIndependent. But presumably AwContents constructor will be > called soon, and if that has an activity, then give that to ApplicationStatus to > track. Right, I was actually trying to avoid this scenario as the constructor of AwContents would be able to trigger an ApplicationState change which can result in a concurrency issue with NetworkChangeNotifier (which listens to ApplicationStatus). This is basically crbug.com/520088, but with this approach I see only one potential problem now, i.e. if network state changes between lets say AwBrowserProcess.start and AwContents contructor, so this is very close..
https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { > > Which means in that particular case that on startup webview will think the > > Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To address > this > > possibility I think we can set sCachedApplicationState to > HAS_RUNNING_ACTIVITIES > > inside the initializeActivityIndependent() method. The assumption is that when > > webview is initialized the Activity is either running or will be running very > > soon. wdyt? > > This is a question for base owner. But my thought is that's a bad idea since > that's by definition inconsistent, since sActivity is still null. > Actually, regarding your earlier comment about inconsistency of setting sCachedApplicationState: currently we have an invariant that sActivityLifecycleIndependentMode implies sActivity == null. So there is no inconsistency w.r.t. sActivity.
https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { On 2015/10/09 16:52:38, timvolodine wrote: > > > Which means in that particular case that on startup webview will think the > > > Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To address > > this > > > possibility I think we can set sCachedApplicationState to > > HAS_RUNNING_ACTIVITIES > > > inside the initializeActivityIndependent() method. The assumption is that > when > > > webview is initialized the Activity is either running or will be running > very > > > soon. wdyt? > > > > This is a question for base owner. But my thought is that's a bad idea since > > that's by definition inconsistent, since sActivity is still null. > > > > Actually, regarding your earlier comment about inconsistency of setting > sCachedApplicationState: currently we have an invariant that > sActivityLifecycleIndependentMode implies sActivity == null. So there is no > inconsistency w.r.t. sActivity. Good point. Which means that deserves a huge comment, probably next to sActivity, that it can be null at these "unexpected" times. And make sure all callers can handle null. https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { On 2015/10/09 16:50:36, timvolodine wrote: > On 2015/10/09 15:58:57, boliu (slow to review) wrote: > > On 2015/10/09 15:46:36, timvolodine wrote: > > > On 2015/10/08 18:01:30, boliu (slow to review) wrote: > > > > On 2015/10/08 16:13:55, timvolodine wrote: > > > > > On 2015/10/02 15:51:32, boliu (slow to review) wrote: > > > > > > since my non-inline comment were ignored twice, inline this time.. > > > > > > > > > > > > What's the story for chromium listening to the lifetime events of > every > > > > single > > > > > > activity in the app, instead of just the ones that has webview created > > in > > > > > them? > > > > > > Do we believe this is ok? > > > > > > > > > > > > > > > > Bo, I didn't ignore your comments and I thought I answered the first one > > > > > already. Basically I think the webview-specific 'application status' > could > > > be > > > > > implemented in android_webview on top of this class. This could use the > > > > > AwWebViewLifecycleObserver for example. > > > > > Also it appears some functionality like NetworkChangeNotifier would not > > work > > > > out > > > > > of the box with an 'ApplicationStatus' that would depend on > > straightforward > > > > > webview creation (see crrev.com/1297943003/). > > > > > > > > > > > > > This doesn't really answer whether this patch is safe, ie what kind of > thing > > > > does observers do right now that could have a negative impact to apps? > It's > > > > probably ok, but want to check. > > > > > > yeah, I have a list of classes using it, need to have a closer look. > > > > > > > > > > > Regarding webview specific implementation, I was thinking > ApplicationStatus > > > > becomes an interface, and there's the default implementation chrome uses, > > and > > > > then there's the webview-specific implementation that webview sets on > init. > > > > > > yes, but I don't think it is essential here. We can always make an interface > > and > > > set implementations instead of sActivityLifecycleIndependentMode boolean. > The > > > question of correct initialization still stands. > > > > > > > > > Also does all listeners deal well with say, not seeing the CREATE > state > > of > > > > an > > > > > > activity? > > > > > > > > > > Regarding missing the ActivityState.CREATED is not really a problem as > it > > is > > > > > followed by STARTED and RESUMED which will make sure the > ApplicationState > > is > > > > set > > > > > accordingly. The listeners would then correctly receive application > state > > > > > changes. > > > > > > > > > > However I did some further experimentation regarding the possibility of > > > > missing > > > > > all of the CREATED, STARTED, RESUMED activity signals which appears to > be > > > > > possible using this somewhat far-fetched snippet. > > > > > > > > > > @Override > > > > > protected void onResume() { > > > > > super.onResume(); > > > > > setContentView(R.layout.activity_webview_browser); > > > > > } > > > > > > > > By this example, You mean that something _real important_ happens in > > onResume, > > > > right? Do we already know nothing _real important_ happens in > > > onCreate/onStart? > > > > > > setContentView triggers AwBrowserProcess.start and hence > > > ApplicationStatus.initializeActivityIndependent() but at this stage no > > activity > > > state changes will be observed as it happens after CREATED,STARTED,RESUMED > > have > > > already happened. If this happens in onStart, or onCreate there is no such > > > problem. > > > > Ahh ok. That can definitely happen with real apps too (ie they create webview > > much later after activity has been active, think which only creates webview > when > > you open a thread) > > > > > > > > > > > > > > > > > > > Which means in that particular case that on startup webview will think > the > > > > > Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To > > address > > > > this > > > > > possibility I think we can set sCachedApplicationState to > > > > HAS_RUNNING_ACTIVITIES > > > > > inside the initializeActivityIndependent() method. The assumption is > that > > > when > > > > > webview is initialized the Activity is either running or will be running > > > very > > > > > soon. wdyt? > > > > > > > > This is a question for base owner. But my thought is that's a bad idea > since > > > > that's by definition inconsistent, since sActivity is still null. > > > > > > > > My thought would be directly insert the activity used to create a webview, > > as > > > > soon as the webview is inserted. Then you can transition to a > non-destroyed > > > but > > > > still consistent state, and maybe also send fake notifications like > onCreate > > > so > > > > the observers are consistent too. > > > > > > Hmm, the problem is that at initialization of ApplicationStatus we don't > > really > > > have an Activity available to init with. Maybe this can be resolved using > > > ActivityThread but that seems a bit hacky (but maybe the only option?). > Also, > > > making ApplicationStatus depend on actual webview creation (i.e. AwContents) > > is > > > not exactly the same and would actually be something different than > > > 'ApplicationStatus'. > > > > Right. I mean supply the activity in AwContents constructor, which is after > > initializeActivityIndependent. The state will be DESTROYED immediately after > > initializeActivityIndependent. But presumably AwContents constructor will be > > called soon, and if that has an activity, then give that to ApplicationStatus > to > > track. > > Right, I was actually trying to avoid this scenario as the constructor of > AwContents would be able to trigger an ApplicationState change which can result > in a concurrency issue with NetworkChangeNotifier (which listens to > ApplicationStatus). This is basically crbug.com/520088, but with this approach I > see only one potential problem now, i.e. if network state changes between lets > say AwBrowserProcess.start and AwContents contructor, so this is very close.. I skimmed through the bug and still don't quite understand the issue wrt to changes here. Can this be explained simplier terms for someone who don't know the network stack, or should I just fire up cs look at all the code mentioned in https://code.google.com/p/chromium/issues/detail?id=520088#c27 ?
https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... File base/android/java/src/org/chromium/base/ApplicationStatus.java (right): https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { On 2015/10/09 19:29:11, boliu (slow to review) wrote: > On 2015/10/09 16:52:38, timvolodine wrote: > > > > Which means in that particular case that on startup webview will think the > > > > Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To > address > > > this > > > > possibility I think we can set sCachedApplicationState to > > > HAS_RUNNING_ACTIVITIES > > > > inside the initializeActivityIndependent() method. The assumption is that > > when > > > > webview is initialized the Activity is either running or will be running > > very > > > > soon. wdyt? > > > > > > This is a question for base owner. But my thought is that's a bad idea since > > > that's by definition inconsistent, since sActivity is still null. > > > > > > > Actually, regarding your earlier comment about inconsistency of setting > > sCachedApplicationState: currently we have an invariant that > > sActivityLifecycleIndependentMode implies sActivity == null. So there is no > > inconsistency w.r.t. sActivity. > > Good point. Which means that deserves a huge comment, probably next to > sActivity, that it can be null at these "unexpected" times. And make sure all > callers can handle null. It's even stronger than that, what I meant by invariant is that sActivity is always null (i.e. at anytime) if we are in sActivityLifecycleIndependentMode. I've added some comments regarding this in getLastTrackedFocusedActivity(). Classes that use it do handle 'null' as it is what it currently returns in webview case. https://codereview.chromium.org/1378653002/diff/40001/base/android/java/src/o... base/android/java/src/org/chromium/base/ApplicationStatus.java:221: if (sActivityLifecycleIndependentMode) { On 2015/10/09 19:29:11, boliu (slow to review) wrote: > On 2015/10/09 16:50:36, timvolodine wrote: > > On 2015/10/09 15:58:57, boliu (slow to review) wrote: > > > On 2015/10/09 15:46:36, timvolodine wrote: > > > > On 2015/10/08 18:01:30, boliu (slow to review) wrote: > > > > > On 2015/10/08 16:13:55, timvolodine wrote: > > > > > > On 2015/10/02 15:51:32, boliu (slow to review) wrote: > > > > > > > since my non-inline comment were ignored twice, inline this time.. > > > > > > > > > > > > > > What's the story for chromium listening to the lifetime events of > > every > > > > > single > > > > > > > activity in the app, instead of just the ones that has webview > created > > > in > > > > > > them? > > > > > > > Do we believe this is ok? > > > > > > > > > > > > > > > > > > > Bo, I didn't ignore your comments and I thought I answered the first > one > > > > > > already. Basically I think the webview-specific 'application status' > > could > > > > be > > > > > > implemented in android_webview on top of this class. This could use > the > > > > > > AwWebViewLifecycleObserver for example. > > > > > > Also it appears some functionality like NetworkChangeNotifier would > not > > > work > > > > > out > > > > > > of the box with an 'ApplicationStatus' that would depend on > > > straightforward > > > > > > webview creation (see crrev.com/1297943003/). > > > > > > > > > > > > > > > > This doesn't really answer whether this patch is safe, ie what kind of > > thing > > > > > does observers do right now that could have a negative impact to apps? > > It's > > > > > probably ok, but want to check. > > > > > > > > yeah, I have a list of classes using it, need to have a closer look. > > > > > > > > > > > > > > Regarding webview specific implementation, I was thinking > > ApplicationStatus > > > > > becomes an interface, and there's the default implementation chrome > uses, > > > and > > > > > then there's the webview-specific implementation that webview sets on > > init. > > > > > > > > yes, but I don't think it is essential here. We can always make an > interface > > > and > > > > set implementations instead of sActivityLifecycleIndependentMode boolean. > > The > > > > question of correct initialization still stands. > > > > > > > > > > > Also does all listeners deal well with say, not seeing the CREATE > > state > > > of > > > > > an > > > > > > > activity? > > > > > > > > > > > > Regarding missing the ActivityState.CREATED is not really a problem as > > it > > > is > > > > > > followed by STARTED and RESUMED which will make sure the > > ApplicationState > > > is > > > > > set > > > > > > accordingly. The listeners would then correctly receive application > > state > > > > > > changes. > > > > > > > > > > > > However I did some further experimentation regarding the possibility > of > > > > > missing > > > > > > all of the CREATED, STARTED, RESUMED activity signals which appears to > > be > > > > > > possible using this somewhat far-fetched snippet. > > > > > > > > > > > > @Override > > > > > > protected void onResume() { > > > > > > super.onResume(); > > > > > > setContentView(R.layout.activity_webview_browser); > > > > > > } > > > > > > > > > > By this example, You mean that something _real important_ happens in > > > onResume, > > > > > right? Do we already know nothing _real important_ happens in > > > > onCreate/onStart? > > > > > > > > setContentView triggers AwBrowserProcess.start and hence > > > > ApplicationStatus.initializeActivityIndependent() but at this stage no > > > activity > > > > state changes will be observed as it happens after CREATED,STARTED,RESUMED > > > have > > > > already happened. If this happens in onStart, or onCreate there is no such > > > > problem. > > > > > > Ahh ok. That can definitely happen with real apps too (ie they create > webview > > > much later after activity has been active, think which only creates webview > > when > > > you open a thread) > > > > > > > > > > > > > > > > > > > > > > > > Which means in that particular case that on startup webview will think > > the > > > > > > Application state is HAS_DESTROYED_ACTIVITIES (sort of default). To > > > address > > > > > this > > > > > > possibility I think we can set sCachedApplicationState to > > > > > HAS_RUNNING_ACTIVITIES > > > > > > inside the initializeActivityIndependent() method. The assumption is > > that > > > > when > > > > > > webview is initialized the Activity is either running or will be > running > > > > very > > > > > > soon. wdyt? > > > > > > > > > > This is a question for base owner. But my thought is that's a bad idea > > since > > > > > that's by definition inconsistent, since sActivity is still null. > > > > > > > > > > My thought would be directly insert the activity used to create a > webview, > > > as > > > > > soon as the webview is inserted. Then you can transition to a > > non-destroyed > > > > but > > > > > still consistent state, and maybe also send fake notifications like > > onCreate > > > > so > > > > > the observers are consistent too. > > > > > > > > Hmm, the problem is that at initialization of ApplicationStatus we don't > > > really > > > > have an Activity available to init with. Maybe this can be resolved using > > > > ActivityThread but that seems a bit hacky (but maybe the only option?). > > Also, > > > > making ApplicationStatus depend on actual webview creation (i.e. > AwContents) > > > is > > > > not exactly the same and would actually be something different than > > > > 'ApplicationStatus'. > > > > > > Right. I mean supply the activity in AwContents constructor, which is after > > > initializeActivityIndependent. The state will be DESTROYED immediately after > > > initializeActivityIndependent. But presumably AwContents constructor will be > > > called soon, and if that has an activity, then give that to > ApplicationStatus > > to > > > track. > > > > Right, I was actually trying to avoid this scenario as the constructor of > > AwContents would be able to trigger an ApplicationState change which can > result > > in a concurrency issue with NetworkChangeNotifier (which listens to > > ApplicationStatus). This is basically crbug.com/520088, but with this approach > I > > see only one potential problem now, i.e. if network state changes between lets > > say AwBrowserProcess.start and AwContents contructor, so this is very close.. > > I skimmed through the bug and still don't quite understand the issue wrt to > changes here. Can this be explained simplier terms for someone who don't know > the network stack, or should I just fire up cs look at all the code mentioned in > https://code.google.com/p/chromium/issues/detail?id=520088#c27 ? Yeah, it's a long story I'll try to summarize. There is apparently a concurrency issue with the host_resolver which happens when we enable NetworkChangeNotifier when the AwContents constructor is called. Basically the NETWORK_CHANGE event propagates via multiple post tasks induces a DNS_CHANGE event and forces the host_resolver to abort pending jobs. Seemingly this happens because an app (i.e. twitter) tries to load a page shortly after creation of webview and ends up thinking that network has changed during page load (or more precisely host_resolution). So given current code it seems that activating NetworkChangeNotifier at the time of webview creation is not a great idea.
Also updated crbug.com/470582 with the findings regarding impact of enabling ApplicationStatus in webview. Looks like the only 'non-trivial' impact is on SimpleIndex (src/net/disk_cache/simple/simple_index.cc).
no new patch set? On 2015/10/12 16:20:58, timvolodine wrote: > Also updated crbug.com/470582 with the findings regarding impact of enabling > ApplicationStatus in webview. Looks like the only 'non-trivial' impact is on > SimpleIndex (src/net/disk_cache/simple/simple_index.cc). So how does this all that apply to the question """ What's the story for chromium listening to the lifetime events of every single activity in the app, instead of just the ones that has webview created in them? Do we believe this is ok? """ Does every look ok? No negative perf impact. Not holding onto strong refs or something?
https://codereview.chromium.org/1378653002/diff/40001/android_webview/glue/ja... File android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java (right): https://codereview.chromium.org/1378653002/diff/40001/android_webview/glue/ja... android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java:250: AwBrowserProcess.start(context, mWebViewDelegate.getApplication()); I think this is needed AwShellActivity too, since it doesn't use glue.
Ping. Is this going to land some day?
On 2015/10/27 21:35:49, dgn wrote: > Ping. Is this going to land some day? I've uploaded a patch with initial cached state for the sake of completeness. After chatting to torne@ about this patch it still appears that knowing the correct initial state of the app during webview initialization is problematic. The current patch implicitly assumes that there cannot be two 'resumed' (aka running) activities at the same time. However going forward android may allow for this to actually happen by using multiple 'windows' for example.
On 2015/10/28 19:11:41, timvolodine wrote: > On 2015/10/27 21:35:49, dgn wrote: > > Ping. Is this going to land some day? > > I've uploaded a patch with initial cached state for the sake of completeness. > > After chatting to torne@ about this patch it still appears that knowing the > correct initial state of the app during webview initialization is problematic. > > The current patch implicitly assumes that there cannot be two 'resumed' (aka > running) activities at the same time. However going forward android may allow > for this to actually happen by using multiple 'windows' for example. also have put some more detailed explanations in crbug.com/470582.
Message was sent while issue was closed.
On 2015/10/28 19:34:26, timvolodine (OOO 26 Nov) wrote: > On 2015/10/28 19:11:41, timvolodine wrote: > > On 2015/10/27 21:35:49, dgn wrote: > > > Ping. Is this going to land some day? > > > > I've uploaded a patch with initial cached state for the sake of completeness. > > > > After chatting to torne@ about this patch it still appears that knowing the > > correct initial state of the app during webview initialization is problematic. > > > > The current patch implicitly assumes that there cannot be two 'resumed' (aka > > running) activities at the same time. However going forward android may allow > > for this to actually happen by using multiple 'windows' for example. > > also have put some more detailed explanations in crbug.com/470582. Closed in favor of https://codereview.chromium.org/1407233017/ |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
