Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1528733002:
Pull the Activity context from WindowAndroid if possible (Closed)
DescriptionPull the Activity context from WindowAndroid if possible
WebView uses ContentViewCore's context for displaying dialogs and
wraps it at construction time. This change makes sure that if there
is an activity that can be reached through WindowAndroid, we use
that as the context inside ColorChooser. Also make sure if there is no activity, we
dont attempt to create a color chooser and try to show it.
Update ActivityWindowAndroid to take a Context type so
that in Android WebView, the activity context can remain wrapped.
BUG=550410, 570429
Committed: https://crrev.com/fdf17ed2da4f4554400c25993aa688cb9355c004
Cr-Commit-Position: refs/heads/master@{#366125}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Using getContext() #
Total comments: 1
Patch Set 3 : Moved all handling to ColorChooser #Patch Set 4 : Changed AwContents logic and moved the static call to WindowAndroid #
Messages
Total messages: 47 (17 generated)
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528733002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528733002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
yusufo@chromium.org changed reviewers: + boliu@chromium.org, torne@chromium.org
Adding you both mostly because I am scared to be missing some context(ha!) here. Will getWindowAndroid in WebView work the way it normally works in Chrome?
https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:636: if (activity != null) return activity; This break's webview's other concern that all context (regardless of whether it's an Application or Activity context) are wrapped with ContextWrapper. If you only need a context, should always use the wrapped context, and getting an Activity reference necessarily means unwrapping the context. But I think you previously said that in chrome, mContext will be the application context, and if chrome wants this to work, then it has to get it from WindowAndroid? Am I understanding that part correctly?
https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:636: if (activity != null) return activity; On 2015/12/15 19:29:00, boliu wrote: > This break's webview's other concern that all context (regardless of whether > it's an Application or Activity context) are wrapped with ContextWrapper. If you > only need a context, should always use the wrapped context, and getting an > Activity reference necessarily means unwrapping the context. > > But I think you previously said that in chrome, mContext will be the application > context, and if chrome wants this to work, then it has to get it from > WindowAndroid? Am I understanding that part correctly? "then it has to get *the context* from WindowAndroid?" Anyway, assuming I understood that correctly.. Here, just return WindowAndroid.getContext here (if it's not null) Then in ColorChooserAndroid, it needs to check if the context is really an activity with ContentViewCore. activityFromContext, but then continue using the wrapped context. Actually probably better to have a bool isActivity(Context) method instead for these things..
https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:636: if (activity != null) return activity; On 2015/12/15 19:29:00, boliu wrote: > This break's webview's other concern that all context (regardless of whether > it's an Application or Activity context) are wrapped with ContextWrapper. If you > only need a context, should always use the wrapped context, and getting an > Activity reference necessarily means unwrapping the context. > > But I think you previously said that in chrome, mContext will be the application > context, and if chrome wants this to work, then it has to get it from > WindowAndroid? Am I understanding that part correctly? Yes, to the second part. Although we have plugged all the code that was using mContext hoping it is an Activity, this "fixes" things so we don't fall on the same trap. But for webview, is there any other way of making sure that the wrapped context wraps this Activity if WindowAndroid returns one? That was my intention here.
https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:636: if (activity != null) return activity; On 2015/12/15 19:37:58, Yusuf wrote: > On 2015/12/15 19:29:00, boliu wrote: > > This break's webview's other concern that all context (regardless of whether > > it's an Application or Activity context) are wrapped with ContextWrapper. If > you > > only need a context, should always use the wrapped context, and getting an > > Activity reference necessarily means unwrapping the context. > > > > But I think you previously said that in chrome, mContext will be the > application > > context, and if chrome wants this to work, then it has to get it from > > WindowAndroid? Am I understanding that part correctly? > > Yes, to the second part. Although we have plugged all the code that was using > mContext hoping it is an Activity, this "fixes" things so we don't fall on the > same trap. > > But for webview, is there any other way of making sure that the wrapped context > wraps this Activity if WindowAndroid returns one? That was my intention here. In webview, WindowAndroid.getContext() is always the wrapped context. (And since webview is not supporting swapping ContentViewCore to a different WindowAndroid yet, mContext here is also the wrapped context, so in webview, mContext and WindowAndroid.getContext are the same thing) fwiw, long term, we should get rid of WindowAndroid.getActivity, since it unwraps the context possibly too early.
https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:636: if (activity != null) return activity; On 2015/12/15 19:46:37, boliu wrote: > On 2015/12/15 19:37:58, Yusuf wrote: > > On 2015/12/15 19:29:00, boliu wrote: > > > This break's webview's other concern that all context (regardless of whether > > > it's an Application or Activity context) are wrapped with ContextWrapper. If > > you > > > only need a context, should always use the wrapped context, and getting an > > > Activity reference necessarily means unwrapping the context. > > > > > > But I think you previously said that in chrome, mContext will be the > > application > > > context, and if chrome wants this to work, then it has to get it from > > > WindowAndroid? Am I understanding that part correctly? > > > > Yes, to the second part. Although we have plugged all the code that was using > > mContext hoping it is an Activity, this "fixes" things so we don't fall on the > > same trap. > > > > But for webview, is there any other way of making sure that the wrapped > context > > wraps this Activity if WindowAndroid returns one? That was my intention here. > > In webview, WindowAndroid.getContext() is always the wrapped context. > > (And since webview is not supporting swapping ContentViewCore to a different > WindowAndroid yet, mContext here is also the wrapped context, so in webview, > mContext and WindowAndroid.getContext are the same thing) > > fwiw, long term, we should get rid of WindowAndroid.getActivity, since it > unwraps the context possibly too early. OK, I will upload another patch soon. As to the long term plan we will be using WindowAndroid.getActivity for all Activity related needs of Tab pretty soon. So I don't foresee that happening :)
https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1528733002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:636: if (activity != null) return activity; On 2015/12/15 19:50:13, Yusuf wrote: > On 2015/12/15 19:46:37, boliu wrote: > > On 2015/12/15 19:37:58, Yusuf wrote: > > > On 2015/12/15 19:29:00, boliu wrote: > > > > This break's webview's other concern that all context (regardless of > whether > > > > it's an Application or Activity context) are wrapped with ContextWrapper. > If > > > you > > > > only need a context, should always use the wrapped context, and getting an > > > > Activity reference necessarily means unwrapping the context. > > > > > > > > But I think you previously said that in chrome, mContext will be the > > > application > > > > context, and if chrome wants this to work, then it has to get it from > > > > WindowAndroid? Am I understanding that part correctly? > > > > > > Yes, to the second part. Although we have plugged all the code that was > using > > > mContext hoping it is an Activity, this "fixes" things so we don't fall on > the > > > same trap. > > > > > > But for webview, is there any other way of making sure that the wrapped > > context > > > wraps this Activity if WindowAndroid returns one? That was my intention > here. > > > > In webview, WindowAndroid.getContext() is always the wrapped context. > > > > (And since webview is not supporting swapping ContentViewCore to a different > > WindowAndroid yet, mContext here is also the wrapped context, so in webview, > > mContext and WindowAndroid.getContext are the same thing) > > > > fwiw, long term, we should get rid of WindowAndroid.getActivity, since it > > unwraps the context possibly too early. > > OK, I will upload another patch soon. > > As to the long term plan we will be using WindowAndroid.getActivity for all > Activity related needs of Tab pretty soon. So I don't foresee that > happening :) Done.
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528733002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528733002/20001
lgtm although I don't own any of this code :p https://codereview.chromium.org/1528733002/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1528733002/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:634: if (getWindowAndroid() == null) return mContext; comment maybe?
yusufo@chromium.org changed reviewers: + tedchoc@chromium.org
Moved all this handling to ColorChooser only since we actually want mContext to be returned for contentviewcore.getContext() to make the theming to work inside all tab related views.
Description was changed from ========== Pull the Activity context from WindowAndroid if possible WebView uses ContentViewCore's context for displaying dialogs and wraps it at construction time. This change makes sure that if there is an activity that can be reached through WindowAndroid, we use that as the context. Also make sure if there is no activity, we dont attempt to create a color chooser and try to show it. BUG=550410 ========== to ========== Pull the Activity context from WindowAndroid if possible WebView uses ContentViewCore's context for displaying dialogs and wraps it at construction time. This change makes sure that if there is an activity that can be reached through WindowAndroid, we use that as the context inside ColorChooser. Also make sure if there is no activity, we dont attempt to create a color chooser and try to show it. BUG=550410 ==========
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528733002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528733002/40001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
torne@chromium.org changed reviewers: + tobiasjs@chromium.org
This seems to have become quite a complex discussion. Adding Toby who has looked at context bugs in WebView previously. Just some general comments: 1) Have you tested this in WebView? Does it work in an app that uses the application context for WebView? (not sure what examples of this we have, but Toby might know). 2) Isn't it likely that other places in the code need to be doing this same logic, or will need to in future? Having this fairly complex process inline in the colour chooser doesn't seem ideal.
On 2015/12/16 14:57:42, Torne wrote: > This seems to have become quite a complex discussion. Adding Toby who has looked > at context bugs in WebView previously. > > Just some general comments: > > 1) Have you tested this in WebView? Does it work in an app that uses the > application context for WebView? (not sure what examples of this we have, but > Toby might know). > > 2) Isn't it likely that other places in the code need to be doing this same > logic, or will need to in future? Having this fairly complex process inline in > the colour chooser doesn't seem ideal. About the other places, one way to fix this in a more long term manner would be to change activityFromContext to activityFromContentViewCore and carry out what I did inside ColorChooser to there. This way any future users in need of an activity would use the same method as well. WDYT?
On 2015/12/16 19:38:27, Yusuf wrote: > On 2015/12/16 14:57:42, Torne wrote: > > This seems to have become quite a complex discussion. Adding Toby who has > looked > > at context bugs in WebView previously. > > > > Just some general comments: > > > > 1) Have you tested this in WebView? Does it work in an app that uses the > > application context for WebView? (not sure what examples of this we have, but > > Toby might know). Oh wait.. Application context case should work (and not crash). But I think this patch basically reverted this patch from torne: https://codereview.chromium.org/1156883004/ which means color picker is going to break webviews again. I just filed crbug.com/570429 for the root cause. Sorry Yusuf, I was wrong when I said ContentViewCore.mContext and getWindowAndroid().getContext() are always the same in webview :( they are not: the one from WindowAndroid is not wrapped :( So torne landed a workaround for color picker that broke chrome. At this moment, there is no good way to do this without adding in some hacks :( Sorry Yusuf > > > > 2) Isn't it likely that other places in the code need to be doing this same > > logic, or will need to in future? Having this fairly complex process inline in > > the colour chooser doesn't seem ideal. > > About the other places, one way to fix this in a more long term manner would be > to change > activityFromContext to activityFromContentViewCore and carry out what I did > inside > ColorChooser to there. This way any future users in need of an activity would > use the same > method as well. WDYT? You mean public static Activity activityFromContentViewCore(ContentViewCore cvc); ? But that would necessarily mean unwrapping the context. How about ContentViewCore.getContext, and ContentViewCore.getActivityContext, both returning Context type. getActivityContext may return null, so need to be null checked before use. Then to support tab dragging, getContext will return the application context in chrome.
On 2015/12/16 20:21:49, boliu wrote: > On 2015/12/16 19:38:27, Yusuf wrote: > > On 2015/12/16 14:57:42, Torne wrote: > > > This seems to have become quite a complex discussion. Adding Toby who has > > looked > > > at context bugs in WebView previously. > > > > > > Just some general comments: > > > > > > 1) Have you tested this in WebView? Does it work in an app that uses the > > > application context for WebView? (not sure what examples of this we have, > but > > > Toby might know). > > Oh wait.. Application context case should work (and not crash). > > But I think this patch basically reverted this patch from torne: > https://codereview.chromium.org/1156883004/ which means color picker is going to > break webviews again. I just filed crbug.com/570429 for the root cause. Sorry > Yusuf, I was wrong when I said ContentViewCore.mContext and > getWindowAndroid().getContext() are always the same in webview :( they are not: > the one from WindowAndroid is not wrapped :( So torne landed a workaround for > color picker that broke chrome. No, it didn't break chrome at the time, both worked, because that was long before yusuf's change that altered the context behaviour. Reverting *either* Yusuf's change or mine fixes Chrome, but reverting mine breaks WebView instead. > At this moment, there is no good way to do this > without adding in some hacks :( > > Sorry Yusuf > > > > > > > 2) Isn't it likely that other places in the code need to be doing this same > > > logic, or will need to in future? Having this fairly complex process inline > in > > > the colour chooser doesn't seem ideal. > > > > About the other places, one way to fix this in a more long term manner would > be > > to change > > activityFromContext to activityFromContentViewCore and carry out what I did > > inside > > ColorChooser to there. This way any future users in need of an activity would > > use the same > > method as well. WDYT? > > You mean public static Activity activityFromContentViewCore(ContentViewCore > cvc); ? But that would necessarily mean unwrapping the context. > > How about ContentViewCore.getContext, and ContentViewCore.getActivityContext, > both returning Context type. getActivityContext may return null, so need to be > null checked before use. Then to support tab dragging, getContext will return > the application context in chrome.
yusufo@chromium.org changed reviewers: + newt@chromium.org
OK, updated AwContents based on Bo's patch. PTAL. Adding newt@ to take a final look if it looks good to you.
aw lgtm
On 2015/12/17 22:48:46, boliu wrote: > aw lgtm Oh can you update the description and add 570429 Maybe something like: Update ActivityWindowAndroid to take a Context type so that in Android WebView, the activity context can remain wrapped.
Description was changed from ========== Pull the Activity context from WindowAndroid if possible WebView uses ContentViewCore's context for displaying dialogs and wraps it at construction time. This change makes sure that if there is an activity that can be reached through WindowAndroid, we use that as the context inside ColorChooser. Also make sure if there is no activity, we dont attempt to create a color chooser and try to show it. BUG=550410 ========== to ========== Pull the Activity context from WindowAndroid if possible WebView uses ContentViewCore's context for displaying dialogs and wraps it at construction time. This change makes sure that if there is an activity that can be reached through WindowAndroid, we use that as the context inside ColorChooser. Also make sure if there is no activity, we dont attempt to create a color chooser and try to show it. Update ActivityWindowAndroid to take a Context type so that in Android WebView, the activity context can remain wrapped. BUG=550410, 570429 ==========
lgtm
The CQ bit was checked by yusufo@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528733002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528733002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
lgtm
The CQ bit was checked by yusufo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/1528733002/#ps60001 (title: "Changed AwContents logic and moved the static call to WindowAndroid")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1528733002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1528733002/60001
Message was sent while issue was closed.
Description was changed from ========== Pull the Activity context from WindowAndroid if possible WebView uses ContentViewCore's context for displaying dialogs and wraps it at construction time. This change makes sure that if there is an activity that can be reached through WindowAndroid, we use that as the context inside ColorChooser. Also make sure if there is no activity, we dont attempt to create a color chooser and try to show it. Update ActivityWindowAndroid to take a Context type so that in Android WebView, the activity context can remain wrapped. BUG=550410, 570429 ========== to ========== Pull the Activity context from WindowAndroid if possible WebView uses ContentViewCore's context for displaying dialogs and wraps it at construction time. This change makes sure that if there is an activity that can be reached through WindowAndroid, we use that as the context inside ColorChooser. Also make sure if there is no activity, we dont attempt to create a color chooser and try to show it. Update ActivityWindowAndroid to take a Context type so that in Android WebView, the activity context can remain wrapped. BUG=550410, 570429 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Pull the Activity context from WindowAndroid if possible WebView uses ContentViewCore's context for displaying dialogs and wraps it at construction time. This change makes sure that if there is an activity that can be reached through WindowAndroid, we use that as the context inside ColorChooser. Also make sure if there is no activity, we dont attempt to create a color chooser and try to show it. Update ActivityWindowAndroid to take a Context type so that in Android WebView, the activity context can remain wrapped. BUG=550410, 570429 ========== to ========== Pull the Activity context from WindowAndroid if possible WebView uses ContentViewCore's context for displaying dialogs and wraps it at construction time. This change makes sure that if there is an activity that can be reached through WindowAndroid, we use that as the context inside ColorChooser. Also make sure if there is no activity, we dont attempt to create a color chooser and try to show it. Update ActivityWindowAndroid to take a Context type so that in Android WebView, the activity context can remain wrapped. BUG=550410, 570429 Committed: https://crrev.com/fdf17ed2da4f4554400c25993aa688cb9355c004 Cr-Commit-Position: refs/heads/master@{#366125} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fdf17ed2da4f4554400c25993aa688cb9355c004 Cr-Commit-Position: refs/heads/master@{#366125}
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1541473002/ by dtu@chromium.org. The reason for reverting is: Failing on perf Android Builders. https://build.chromium.org/p/chromium.perf/builders/Android%20Builder/builds/... https://build.chromium.org/p/chromium.perf/builders/Android%20arm64%20Builder.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
