Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(8)

Issue 1879013002: 🍈 Unify application context usage. (Closed)

Created:
4 years, 8 months ago by Peter Wen
Modified:
4 years, 8 months ago
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Unify application context usage. Separate out ContextUtils initializing the global application context on java-side vs native-side. Have LibraryLoader initialize native-side application side and initialize java-side application context as early as possible. This allows callers to be certain that they are interacting with the application context instead of a local context, and will allow consolidating application-wide context-based globals like SharedPreferences in subsequent CLs. BUG=552419 Committed: https://crrev.com/6b5b7b5d2a1d2cdfa58e8e06f9a350ea4d1b286a Cr-Commit-Position: refs/heads/master@{#389782}

Patch Set 1 #

Patch Set 2 : Maintain backward compatibility. #

Patch Set 3 : Fix junit asserts. #

Patch Set 4 : Reduce scope again. #

Total comments: 6

Patch Set 5 : Better initialization. #

Patch Set 6 : Go back to using BaseChromiumApplication. #

Patch Set 7 : Fix webview tests. #

Patch Set 8 : More shuffling. #

Patch Set 9 : Fix content tests. :( #

Total comments: 9

Patch Set 10 : Consolidate some changes for webview. #

Patch Set 11 : Fix ChildProcessLauncherTest. #

Patch Set 12 : Rebase. #

Patch Set 13 : Fix mojo test. #

Total comments: 10

Patch Set 14 : Make it clear which setters silently fail. #

Patch Set 15 : Fix webview tests. #

Patch Set 16 : Initialize native in webview test when necessary. #

Total comments: 4

Patch Set 17 : Better initialization for browser process. #

Patch Set 18 : Switch child processes to init. #

Patch Set 19 : Relax strictness, allow resetting same application instance. #

Patch Set 20 : Switch back to initApplicationContext in non-junit tests. #

Patch Set 21 : Remove unnecessary init in BlimpLibraryLoader. #

Total comments: 14

Patch Set 22 : g cl try #

Patch Set 23 : Remove @CalledByNative #

Total comments: 6

Patch Set 24 : Remove native InitApplicationContext #

Total comments: 2

Patch Set 25 : Fix caller/callee relationship in resource_provider. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -82 lines) Patch
M android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java View 1 2 3 4 5 6 7 8 9 7 chunks +8 lines, -19 lines 0 comments Download
M android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 4 chunks +9 lines, -5 lines 0 comments Download
M android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +4 lines, -2 lines 0 comments Download
M android_webview/javatests/src/org/chromium/android_webview/test/HttpCacheTest.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +2 lines, -0 lines 0 comments Download
M android_webview/test/shell/src/org/chromium/android_webview/test/AwTestRunnerActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +2 lines, -0 lines 0 comments Download
M base/android/context_utils.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +0 lines, -8 lines 0 comments Download
M base/android/context_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +0 lines, -5 lines 0 comments Download
M base/android/java/src/org/chromium/base/ContextUtils.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +31 lines, -13 lines 0 comments Download
M base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +8 lines, -11 lines 0 comments Download
M blimp/client/app/android/java/src/org/chromium/blimp/BlimpLibraryLoader.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +0 lines, -2 lines 0 comments Download
M chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +3 lines, -0 lines 0 comments Download
M components/cronet/android/java/src/org/chromium/net/CronetLibraryLoader.java View 1 2 3 4 14 15 16 17 18 19 20 21 2 chunks +2 lines, -1 line 0 comments Download
M components/resource_provider/android/android_hooks.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 1 chunk +0 lines, -1 line 0 comments Download
M components/resource_provider/android/java/org/chromium/resource_provider/Main.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +2 lines, -0 lines 0 comments Download
M components/web_restrictions/browser/junit/src/org/chromium/components/webrestrictions/WebRestrictionsClientTest.java View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -1 line 0 comments Download
M content/public/android/java/src/org/chromium/content/app/ChildProcessService.java View 1 2 3 4 5 6 7 14 15 16 17 2 chunks +2 lines, -1 line 0 comments Download
M content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java View 1 2 3 4 5 6 8 9 10 11 12 13 3 chunks +0 lines, -4 lines 0 comments Download
M content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherTest.java View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M mojo/android/javatests/src/org/chromium/mojo/MojoTestCase.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +4 lines, -3 lines 0 comments Download
M net/android/junit/src/org/chromium/net/HttpNegotiateAuthenticatorTest.java View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M remoting/android/host/src/org/chromium/chromoting/host/jni/Host.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +1 line, -1 line 0 comments Download
M remoting/android/java/src/org/chromium/chromoting/jni/JniInterface.java View 1 2 3 4 5 6 7 8 9 10 11 14 15 16 17 18 19 20 21 1 chunk +1 line, -2 lines 0 comments Download
M testing/android/native_test/java/src/org/chromium/native_test/NativeTestActivity.java View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 2 chunks +2 lines, -0 lines 0 comments Download
M testing/android/native_test/native_test_launcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 1 chunk +0 lines, -1 line 0 comments Download

Messages

Total messages: 53 (11 generated)
Peter Wen
Took a lot of tweaking around, but this is what I ended up with. If ...
4 years, 8 months ago (2016-04-12 21:28:33 UTC) #3
Peter Wen
On 2016/04/12 21:28:33, Peter Wen wrote: > Took a lot of tweaking around, but this ...
4 years, 8 months ago (2016-04-12 21:30:05 UTC) #4
Torne
https://codereview.chromium.org/1879013002/diff/60001/base/android/java/src/org/chromium/base/BaseChromiumApplication.java File base/android/java/src/org/chromium/base/BaseChromiumApplication.java (right): https://codereview.chromium.org/1879013002/diff/60001/base/android/java/src/org/chromium/base/BaseChromiumApplication.java#newcode116 base/android/java/src/org/chromium/base/BaseChromiumApplication.java:116: ContextUtils.setApplicationContext(this); I really don't like depending on BaseChromiumApplication here. ...
4 years, 8 months ago (2016-04-13 15:38:38 UTC) #5
Peter Wen
Yaron mentioned that we may not have to merge this after all. Making all the ...
4 years, 8 months ago (2016-04-13 18:49:21 UTC) #6
Peter Wen
Also, left debug logs to figure out instrumentation tests failures (if any).
4 years, 8 months ago (2016-04-13 18:52:31 UTC) #7
Peter Wen
On 2016/04/13 18:52:31, Peter Wen wrote: > Also, left debug logs to figure out instrumentation ...
4 years, 8 months ago (2016-04-13 21:18:13 UTC) #8
Torne
On 2016/04/13 21:18:13, Peter Wen wrote: > On 2016/04/13 18:52:31, Peter Wen wrote: > > ...
4 years, 8 months ago (2016-04-14 16:52:39 UTC) #9
Torne
On 2016/04/13 21:18:13, Peter Wen wrote: > On 2016/04/13 18:52:31, Peter Wen wrote: > > ...
4 years, 8 months ago (2016-04-14 16:52:40 UTC) #10
Peter Wen
Agreed. I can move initialization up to ChromeApplication. Is there an analogous initialization path for ...
4 years, 8 months ago (2016-04-14 17:02:09 UTC) #11
Torne
On 2016/04/14 17:02:09, Peter Wen wrote: > Agreed. I can move initialization up to ChromeApplication. ...
4 years, 8 months ago (2016-04-14 17:08:43 UTC) #12
Peter Wen
PTAL. This is fine for everything using ChromeApplication. When I do the shared preferences migration ...
4 years, 8 months ago (2016-04-14 19:23:01 UTC) #13
Torne
Sorry this is so difficult, but: https://codereview.chromium.org/1879013002/diff/160001/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/1879013002/diff/160001/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java#newcode49 android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:49: libraryLoader.initialize(); It doesn't ...
4 years, 8 months ago (2016-04-15 14:22:43 UTC) #14
Peter Wen
Thanks for the fast review cycle, Torne. I'm learning a lot about various initialization paths ...
4 years, 8 months ago (2016-04-18 18:49:58 UTC) #15
Torne
https://codereview.chromium.org/1879013002/diff/160001/base/android/java/src/org/chromium/base/ContextUtils.java File base/android/java/src/org/chromium/base/ContextUtils.java (right): https://codereview.chromium.org/1879013002/diff/160001/base/android/java/src/org/chromium/base/ContextUtils.java#newcode47 base/android/java/src/org/chromium/base/ContextUtils.java:47: assert sApplicationContext == null || sApplicationContext == appContext; On ...
4 years, 8 months ago (2016-04-19 16:55:58 UTC) #16
Peter Wen
Making it clear which calls can silently fail and we now have exceptions to help ...
4 years, 8 months ago (2016-04-19 18:47:58 UTC) #17
Torne
Just one nit about an assert and otherwise LGTM. https://codereview.chromium.org/1879013002/diff/240001/android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java File android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java (right): https://codereview.chromium.org/1879013002/diff/240001/android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java#newcode91 android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java:91: ...
4 years, 8 months ago (2016-04-20 14:58:41 UTC) #18
Peter Wen
Some more fixes, will follow-up with the TODO I added in LibraryLoader, as that belongs ...
4 years, 8 months ago (2016-04-20 17:29:13 UTC) #19
Torne
LGTM, thanks for all the effort here! Much appreciated.
4 years, 8 months ago (2016-04-21 12:33:14 UTC) #20
Peter Wen
Thanks for the thorough review, Torne! Ended up switching back to the assert conditions we ...
4 years, 8 months ago (2016-04-21 13:50:46 UTC) #21
Torne
OK, still LGTM.
4 years, 8 months ago (2016-04-21 13:56:13 UTC) #22
Peter Wen
+davidben@ for net/ OWNERS. +yfriedman@ for chrome/android and content/ OWNERS. +nyquist@ for blimp/ OWNERS. +caitkp@ ...
4 years, 8 months ago (2016-04-21 18:11:12 UTC) #26
davidben
net lgtm
4 years, 8 months ago (2016-04-21 18:39:06 UTC) #27
Sergey Ulanov
remoting lgtm
4 years, 8 months ago (2016-04-21 18:42:34 UTC) #28
Yaron
https://codereview.chromium.org/1879013002/diff/400001/base/android/java/src/org/chromium/base/ContextUtils.java File base/android/java/src/org/chromium/base/ContextUtils.java (right): https://codereview.chromium.org/1879013002/diff/400001/base/android/java/src/org/chromium/base/ContextUtils.java#newcode47 base/android/java/src/org/chromium/base/ContextUtils.java:47: if (sApplicationContext != null && sApplicationContext != appContext) { ...
4 years, 8 months ago (2016-04-21 19:29:50 UTC) #29
Cait (Slow)
components/ lgtm
4 years, 8 months ago (2016-04-21 19:39:57 UTC) #30
Peter Wen
Haha, opps, interesting eager patch name. :D https://codereview.chromium.org/1879013002/diff/400001/base/android/java/src/org/chromium/base/ContextUtils.java File base/android/java/src/org/chromium/base/ContextUtils.java (right): https://codereview.chromium.org/1879013002/diff/400001/base/android/java/src/org/chromium/base/ContextUtils.java#newcode47 base/android/java/src/org/chromium/base/ContextUtils.java:47: if (sApplicationContext ...
4 years, 8 months ago (2016-04-21 20:07:58 UTC) #31
Yaron
https://codereview.chromium.org/1879013002/diff/400001/base/android/java/src/org/chromium/base/ContextUtils.java File base/android/java/src/org/chromium/base/ContextUtils.java (right): https://codereview.chromium.org/1879013002/diff/400001/base/android/java/src/org/chromium/base/ContextUtils.java#newcode90 base/android/java/src/org/chromium/base/ContextUtils.java:90: @CalledByNative On 2016/04/21 20:07:58, Peter Wen wrote: > On ...
4 years, 8 months ago (2016-04-21 20:22:03 UTC) #32
sky
I'm not familiar with the mojo java class. Could you find a better owner?
4 years, 8 months ago (2016-04-21 22:40:48 UTC) #33
Peter Wen
-sky
4 years, 8 months ago (2016-04-25 13:20:23 UTC) #35
Peter Wen
+amistry@ for mojo/ OWNERS https://codereview.chromium.org/1879013002/diff/400001/base/android/java/src/org/chromium/base/ContextUtils.java File base/android/java/src/org/chromium/base/ContextUtils.java (right): https://codereview.chromium.org/1879013002/diff/400001/base/android/java/src/org/chromium/base/ContextUtils.java#newcode90 base/android/java/src/org/chromium/base/ContextUtils.java:90: @CalledByNative On 2016/04/21 20:22:02, Yaron ...
4 years, 8 months ago (2016-04-25 13:22:02 UTC) #37
Yaron
https://codereview.chromium.org/1879013002/diff/440001/android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java File android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java (right): https://codereview.chromium.org/1879013002/diff/440001/android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java#newcode34 android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java:34: ContextUtils.initApplicationContext(appContext); I'm not familiar enough with aw setup but ...
4 years, 8 months ago (2016-04-25 14:09:45 UTC) #38
Peter Wen
Time to fix some tests. :) https://codereview.chromium.org/1879013002/diff/440001/android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java File android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java (right): https://codereview.chromium.org/1879013002/diff/440001/android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java#newcode34 android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java:34: ContextUtils.initApplicationContext(appContext); On 2016/04/25 ...
4 years, 8 months ago (2016-04-25 15:30:30 UTC) #39
Yaron
https://codereview.chromium.org/1879013002/diff/460001/components/resource_provider/android/java/org/chromium/resource_provider/Main.java File components/resource_provider/android/java/org/chromium/resource_provider/Main.java (right): https://codereview.chromium.org/1879013002/diff/460001/components/resource_provider/android/java/org/chromium/resource_provider/Main.java#newcode24 components/resource_provider/android/java/org/chromium/resource_provider/Main.java:24: @CalledByNative err, you deleted the caller in android_hooks.cc.. is ...
4 years, 8 months ago (2016-04-25 15:40:50 UTC) #40
Peter Wen
+sky again for components/resource_provider/android/. Is this code still used? I can't find a suitable GN ...
4 years, 8 months ago (2016-04-25 18:07:35 UTC) #42
sky
I suspect you're right that components/resource_provider isn't used for android.
4 years, 8 months ago (2016-04-25 18:28:37 UTC) #43
Yaron
ok lgtm now - resource provider cleanup can be done separately
4 years, 8 months ago (2016-04-25 18:52:56 UTC) #44
nyquist
blimp lgtm
4 years, 8 months ago (2016-04-25 21:50:54 UTC) #45
Anand Mistry (off Chromium)
mojo lgtm
4 years, 8 months ago (2016-04-26 00:19:58 UTC) #46
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1879013002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1879013002/480001
4 years, 8 months ago (2016-04-26 15:13:35 UTC) #49
commit-bot: I haz the power
Committed patchset #25 (id:480001)
4 years, 8 months ago (2016-04-26 15:18:01 UTC) #50
commit-bot: I haz the power
Patchset 25 (id:??) landed as https://crrev.com/6b5b7b5d2a1d2cdfa58e8e06f9a350ea4d1b286a Cr-Commit-Position: refs/heads/master@{#389782}
4 years, 8 months ago (2016-04-26 15:19:09 UTC) #52
Peter Wen
4 years, 8 months ago (2016-04-26 18:17:32 UTC) #53
Message was sent while issue was closed.
A revert of this CL (patchset #25 id:480001) has been created in
https://codereview.chromium.org/1917193003/ by wnwen@chromium.org.

The reason for reverting is: Broke CQ unittests as they did not set native
application context properly. See https://codereview.chromium.org/1919283002/
for context..

Powered by Google App Engine
This is Rietveld 408576698