New issue
Advanced search Search tips

Issue 841403 link

Starred by 3 users

Issue metadata

Status: Untriaged
Owner: ----
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Cleanup code that does native library loading / startup

Project Member Reported by dskiba@chromium.org, May 9 2018

Issue description

I've found that some API decisions that we made in LibraryLoader and BrowserStartupController complicate their refactoring / prototyping.

For example both LibraryLoader.get() and BrowserStartupController.get() want you to specify LibraryProcessType, while in reality that argument is used in the context of a couple of methods, the rest of the time specifying LibraryProcessType is just a noise (example: all BrowserStartupController.isStartupSuccessfullyCompleted calls).

This is an umbrella issue for all related changes.
 
Project Member

Comment 1 by bugdroid1@chromium.org, May 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3eae9afaa75672a5e85771f42bed48e0fd2b4013

commit 3eae9afaa75672a5e85771f42bed48e0fd2b4013
Author: Dmitry Skiba <dskiba@chromium.org>
Date: Tue May 15 20:03:01 2018

Remove argument from BrowserStartupController.StartupCallback.onSuccess().

BrowserStartupController.StartupCallback.onSuccess() has 'alreadyStarted'
argument, which is not used anywhere.

Bug: 841403
Change-Id: I8faf23c3b1d61d217460c0e3943dadeb6111ee05
Reviewed-on: https://chromium-review.googlesource.com/1055796
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Dmitry Skiba <dskiba@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558819}
[modify] https://crrev.com/3eae9afaa75672a5e85771f42bed48e0fd2b4013/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
[modify] https://crrev.com/3eae9afaa75672a5e85771f42bed48e0fd2b4013/chrome/android/java/src/org/chromium/chrome/browser/ntp/ContentSuggestionsNotifier.java
[modify] https://crrev.com/3eae9afaa75672a5e85771f42bed48e0fd2b4013/chrome/android/java/src/org/chromium/chrome/browser/offlinepages/prefetch/PrefetchedPagesNotifier.java
[modify] https://crrev.com/3eae9afaa75672a5e85771f42bed48e0fd2b4013/chrome/android/java/src/org/chromium/chrome/browser/provider/ChromeBrowserProvider.java
[modify] https://crrev.com/3eae9afaa75672a5e85771f42bed48e0fd2b4013/chrome/android/java/src/org/chromium/chrome/browser/services/gcm/GcmUma.java
[modify] https://crrev.com/3eae9afaa75672a5e85771f42bed48e0fd2b4013/content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
[modify] https://crrev.com/3eae9afaa75672a5e85771f42bed48e0fd2b4013/content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
[modify] https://crrev.com/3eae9afaa75672a5e85771f42bed48e0fd2b4013/content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java
[modify] https://crrev.com/3eae9afaa75672a5e85771f42bed48e0fd2b4013/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java

Project Member

Comment 2 by bugdroid1@chromium.org, May 23 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0857478da4db286f26a261b41b6297841327ef37

commit 0857478da4db286f26a261b41b6297841327ef37
Author: Dmitry Skiba <dskiba@chromium.org>
Date: Wed May 23 06:21:34 2018

Directly pass LibraryProcessType to the native code.

LibraryManager indirectly passes LibraryProcessType value to the native
code via getLibraryProcessType() static method, which is only invoked
in the context of nativeLibraryLoaded() call. I.e. the indirection is
unnecessary and only makes data flow harder to understand.

This CL removes getLibraryProcessType() method and passes process type
value directly to nativeLibraryLoaded().

Bug: 841403
Change-Id: Ib15d765f968029d46d1603cdc7a5d5e7d280bb06
Reviewed-on: https://chromium-review.googlesource.com/1067578
Commit-Queue: Dmitry Skiba <dskiba@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Maria Khomenko <mariakhomenko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560963}
[modify] https://crrev.com/0857478da4db286f26a261b41b6297841327ef37/android_webview/lib/webview_entry_point.cc
[modify] https://crrev.com/0857478da4db286f26a261b41b6297841327ef37/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
[modify] https://crrev.com/0857478da4db286f26a261b41b6297841327ef37/base/android/library_loader/library_loader_hooks.cc
[modify] https://crrev.com/0857478da4db286f26a261b41b6297841327ef37/base/android/library_loader/library_loader_hooks.h
[modify] https://crrev.com/0857478da4db286f26a261b41b6297841327ef37/chrome/browser/android/chrome_entry_point.cc
[modify] https://crrev.com/0857478da4db286f26a261b41b6297841327ef37/chrome/browser/android/chrome_entry_point_for_test.cc
[modify] https://crrev.com/0857478da4db286f26a261b41b6297841327ef37/chrome/browser/android/chrome_sync_shell_entry_point.cc
[modify] https://crrev.com/0857478da4db286f26a261b41b6297841327ef37/chrome/browser/android/monochrome_entry_point.cc

Project Member

Comment 3 by bugdroid1@chromium.org, May 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/11303973f90ff0241871b010820c99e5c5142456

commit 11303973f90ff0241871b010820c99e5c5142456
Author: Dmitry Skiba <dskiba@chromium.org>
Date: Thu May 24 18:35:55 2018

Don't require LibraryProcessType for getting LibraryLoader instance.

LibraryLoader is a singleton, but to get its instance one has to pass
LibraryProcessType value. However out of all LibraryLoader methods only
two (initialize() and ensureInitialized()) actually use the process type
value.

Also, LibraryLoader.get(LibraryProcessType) stores process type on the
first call (which creates the instance), and throws an exception if
subsequent calls don't use the same value. This puts additional burden
on callers, which have to handle ProcessInitException from get() even
when they are calling non-initialization methods (e.g. preloadNow()).

This CL moves LibraryProcessType argument from get() to initialization
methods (initialize() and ensureInitialized()), and also renames get()
to getInstance().

Bug: 841403
Change-Id: I74b311316f10831c23915a7f2189f38c660c4982
Reviewed-on: https://chromium-review.googlesource.com/1067847
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Cait Phillips <caitkp@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Sergey Volk <servolk@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Yaron Friedman <yfriedman@chromium.org>
Commit-Queue: Dmitry Skiba <dskiba@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561563}
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumAwInit.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/android_webview/java/src/org/chromium/android_webview/AwCookieManager.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/base/android/javatests/src/org/chromium/base/EarlyTraceEventTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/base/android/javatests/src/org/chromium/base/metrics/RecordHistogramTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/base/test/android/java/src/org/chromium/base/MultiprocessTestClientServiceDelegate.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitTaskRunner.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/java/src/org/chromium/chrome/browser/init/ChromeBrowserInitializer.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/java/src/org/chromium/chrome/browser/init/NativeInitializationController.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/java/src/org/chromium/chrome/browser/photo_picker/DecoderService.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/javatests/src/org/chromium/chrome/browser/RestoreHistogramTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/DetachedResourceRequestTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/TrustedCdnPublisherUrlTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chrome/android/junit/src/org/chromium/chrome/browser/init/AsyncInitTaskRunnerTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/chromecast/browser/android/apk/src/org/chromium/chromecast/shell/CastBrowserHelper.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/components/invalidation/impl/android/javatests/src/org/chromium/components/invalidation/InvalidationClientServiceTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/content/public/android/java/src/org/chromium/content/app/ContentChildProcessServiceDelegate.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/content/public/android/javatests/src/org/chromium/content/browser/ChildProcessLauncherHelperTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/content/public/test/android/javatests/src/org/chromium/content/browser/test/NativeLibraryTestRule.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/content/shell/android/browsertests/src/org/chromium/content_shell/browsertests/ContentShellBrowserTestActivity.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/content/shell/android/linker_test_apk/src/org/chromium/chromium_linker_test_apk/ChromiumLinkerTestActivity.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ChildProcessLauncherTestHelperService.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/ContentShellActivity.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/content/shell/android/shell_apk/src/org/chromium/content_shell_apk/TestChildProcessService.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/mojo/android/javatests/src/org/chromium/mojo/MojoTestRule.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java
[modify] https://crrev.com/11303973f90ff0241871b010820c99e5c5142456/net/test/android/javatests/src/org/chromium/net/test/EmbeddedTestServerImpl.java

Project Member

Comment 4 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/clank/internal/apps/+/aefdb6646b8efbf308c22f83395255db30ff4526

commit aefdb6646b8efbf308c22f83395255db30ff4526
Author: Dmitry Skiba <dskiba@google.com>
Date: Wed May 30 17:24:36 2018

Project Member

Comment 5 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c

commit d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c
Author: Dmitry Skiba <dskiba@chromium.org>
Date: Wed May 30 20:09:02 2018

Turn LibraryLoader into proper singleton.

LibraryLoader's state and API is split between static/instance variables
and methods. This is messy and needlessly complicates things.

This CL changes static state and methods to instance ones thus turning
LibraryLoader into a proper singleton. Additionally this CL adds missing
locking / 'AlreadyLocked' suffixes to some functions.

Bug: 841403
Change-Id: I694b51ae9ec1dad6f7b94d8e6e9cbed5afd5eba0
Reviewed-on: https://chromium-review.googlesource.com/1069326
Reviewed-by: David Trainor <dtrainor@chromium.org>
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: agrieve <agrieve@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Dmitry Skiba <dskiba@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562971}
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/base/android/java/src/org/chromium/base/metrics/CachedMetrics.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/ChromeStrictMode.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/MonochromeApplication.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/WarmupManager.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/download/DownloadNotificationUmaHelper.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/locale/LocaleManager.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/media/PictureInPictureController.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/media/remote/RecordCastAction.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/notifications/NotificationUmaTracker.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchBoxDataProvider.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/searchwidget/SearchWidgetProvider.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/superviseduser/SupervisedUserContentProvider.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabbedModeTabPersistencePolicy.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkActivity.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/javatests/src/org/chromium/chrome/browser/incognito/IncognitoNotificationServiceTest.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/chrome/android/javatests/src/org/chromium/chrome/browser/notifications/NotificationPlatformBridgeIntentTest.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/components/background_task_scheduler/android/java/src/org/chromium/components/background_task_scheduler/BackgroundTaskSchedulerUma.java
[modify] https://crrev.com/d52e8a97ab2f153f1244ae0a79f0cf6ec59b687c/components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java

Comment 6 by dskiba@chromium.org, May 30 2018

Cc: yfried...@chromium.org
Owner: ----
Status: Available (was: Started)
Thing to do here: remove LibraryProcessType from BrowserStartupController.get(), and rename it to 'getInstance()'. The reasoning is the same as with LibraryLoader change above - only 2 methods actually need LibraryProcessType (startBrowserProcesses{Sync, Async}), and those two methods only used in couple of places. Majority of BrowserStartupController usages call either isStartupSuccessfullyCompleted() or addStartupCompletedObserver(), neither of which needs to know LibraryProcessType.

I have a CL that does the change, but it doesn't apply anymore because of a recent refactoring that added BrowserStartupControllerImpl: https://chromium-review.googlesource.com/c/chromium/src/+/1070611

It would be nice if someone could rebase land the CL.
Status: Untriaged (was: Available)
Available, but no owner or component? Please find a component, as no one will ever find this without one.

Sign in to add a comment