New issue
Advanced search Search tips

Issue 887926 link

Starred by 2 users

Issue metadata

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



Sign in to add a comment

Dagger migration

Project Member Reported by peconn@chromium.org, Sep 21

Issue description

A catch-all bug for various migrating to Dagger CLs
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 26

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

commit 4814c9a9efdeda97c8e1edda93e6e5277feafb46
Author: Peter E Conn <peconn@chromium.org>
Date: Wed Sep 26 10:05:31 2018

📩 Move TrustedWebActivityDisclosure over to Dagger.

Bug: 887926
Change-Id: I76f40ec06c7561196fa9cf7d21373dde24b0c30e
Reviewed-on: https://chromium-review.googlesource.com/1238439
Reviewed-by: Michael van Ouwerkerk <mvanouwerkerk@chromium.org>
Commit-Queue: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594265}
[modify] https://crrev.com/4814c9a9efdeda97c8e1edda93e6e5277feafb46/chrome/android/java/src/org/chromium/chrome/browser/browserservices/TrustedWebActivityDisclosure.java
[modify] https://crrev.com/4814c9a9efdeda97c8e1edda93e6e5277feafb46/chrome/android/java/src/org/chromium/chrome/browser/browserservices/TrustedWebActivityUi.java
[modify] https://crrev.com/4814c9a9efdeda97c8e1edda93e6e5277feafb46/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/4814c9a9efdeda97c8e1edda93e6e5277feafb46/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeActivityCommonsModule.java
[modify] https://crrev.com/4814c9a9efdeda97c8e1edda93e6e5277feafb46/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeActivityComponent.java
[modify] https://crrev.com/4814c9a9efdeda97c8e1edda93e6e5277feafb46/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeAppComponent.java
[modify] https://crrev.com/4814c9a9efdeda97c8e1edda93e6e5277feafb46/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeAppModule.java
[add] https://crrev.com/4814c9a9efdeda97c8e1edda93e6e5277feafb46/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/CustomTabActivityComponent.java
[modify] https://crrev.com/4814c9a9efdeda97c8e1edda93e6e5277feafb46/chrome/android/java_sources.gni

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 23

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

commit 1e226935ec330c01866444a988513e030cdd9e8e
Author: Pavel Shmakov <pshmakov@google.com>
Date: Tue Oct 23 10:27:52 2018

Introduce activity lifecycle dispatching

Lifecycle dispatching is important for modularization and decoupling.
Currently we see activities having heaps of code in lifecycle methods
touching lots of different classes. These classes often pass the events
further down to their dependencies. To have less coupling, classes should
be able to subscribe to lifecycle events and handle them on their own.

This is especially important for the destruction event. Having a public
destroy() method and relying on some other object to call it is unreliable
and easily produces memory leaks. See, for example,
https://chromium-review.googlesource.com/c/chromium/src/+/1172425/4/chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java

With DI, this is even more important. Whereas without DI whoever creates an
object "owns" it, and is responsible for destroying it, with DI there
is no such ownership (at least for scoped classes), as the instances are
created and held by Components. At the same time, DI makes implementing
lifecycle dispatching easy.

In this CL I introduce the lifecycle dispatching and provide a simple
usage example in Contextual Suggestions classes. Another example
can be seen in the upcoming CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1281102/1/chrome/android/java/src/org/chromium/chrome/browser/browserservices/TrustedWebActivityUi.java

Bug: 887926
Change-Id: I81a43406df320544bc55732f622704109631a232
Reviewed-on: https://chromium-review.googlesource.com/c/1283256
Commit-Queue: Pavel Shmakov <pshmakov@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601900}
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/contextual_suggestions/ContextualSuggestionsCoordinator.java
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeActivityCommonsModule.java
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeActivityComponent.java
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeAppComponent.java
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/CustomTabActivityComponent.java
[add] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/init/ActivityLifecycleDispatcher.java
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/init/AsyncInitializationActivity.java
[add] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/lifecycle/Destroyable.java
[add] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/lifecycle/InflationObserver.java
[add] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/lifecycle/LifecycleObserver.java
[add] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/lifecycle/NativeInitObserver.java
[add] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/lifecycle/PauseResumeWithNativeObserver.java
[add] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/lifecycle/StartStopWithNativeObserver.java
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java/src/org/chromium/chrome/browser/preferences/ContextualSuggestionsPreference.java
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/java_sources.gni
[modify] https://crrev.com/1e226935ec330c01866444a988513e030cdd9e8e/chrome/android/javatests/src/org/chromium/chrome/browser/contextual_suggestions/ContextualSuggestionsTest.java

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 26

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

commit 090f1f05fd7b8c12f4918325c7a04b5bd147b41d
Author: Pavel Shmakov <pshmakov@google.com>
Date: Fri Oct 26 17:01:59 2018

Migrate TrustedWebActivityUi and related classes to Dagger, decouple CustomTabActivity from it.

As a first step of simplifying CustomTabActivity, I extract all the code related
to TrustedWebActivityUi and use Dagger to organize dependencies for
TrustedWebActivityUi.

I make use of recently introduced activity lifecycle dispatching: https://crrev.com/c/1283256

Bug: 887926
Change-Id: Ia36a026a7f297e7c0f101189945c44454d12fa6e
Reviewed-on: https://chromium-review.googlesource.com/c/1281102
Commit-Queue: Pavel Shmakov <pshmakov@chromium.org>
Reviewed-by: Yusuf Ozuysal <yusufo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603122}
[add] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/AppHooksModule.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/browserservices/ClientAppDataRecorder.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/browserservices/TrustedWebActivityDisclosure.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/browserservices/TrustedWebActivityUi.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabActivity.java
[add] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabBrowserControlsVisibilityDelegate.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/customtabs/CustomTabIntentDataProvider.java
[add] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/customtabs/dependency_injection/CustomTabActivityComponent.java
[add] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/customtabs/dependency_injection/CustomTabActivityModule.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeActivityCommonsModule.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeAppComponent.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeAppModule.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/ChromeCommonQualifiers.java
[delete] https://crrev.com/78b9bb38c353fed0dfd7069129f7828a5af972fb/chrome/android/java/src/org/chromium/chrome/browser/dependency_injection/CustomTabActivityComponent.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/fullscreen/ChromeFullscreenManager.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java/src/org/chromium/chrome/browser/util/UrlUtilities.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/java_sources.gni
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/junit/src/org/chromium/chrome/browser/browserservices/ClientAppDataRecorderTest.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/junit/src/org/chromium/chrome/browser/browserservices/DomainDataCleanerTest.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/junit/src/org/chromium/chrome/browser/browserservices/TrustedWebActivityDisclosureTest.java
[modify] https://crrev.com/090f1f05fd7b8c12f4918325c7a04b5bd147b41d/chrome/android/junit/src/org/chromium/chrome/browser/util/test/ShadowUrlUtilities.java

Status: Available (was: Started)
This issue has been marked as started, but has no owner. Making available.
Status: Untriaged (was: Available)
Does not have a component.

Sign in to add a comment