Untangle display logic from control logic in AppBannerInfoBarDelegateAndroid |
|||
Issue descriptionIntroducing a modal install flow means that we need to separate out display logic in AppBannerInfoBarDelegateAndroid from control logic in that same class. Right now the two are mixed together, e.g.: - monitoring when a native app is installed to update the infobar button - monitoring WebAPK installation to update the infobar button Unfortunately, modals are controlled entirely from Java, while infobars are controlled from native C++. WebAPK installation bounces between Java and native, while native app installation is entirely in Java. As we transition, we're going to need to be able to wire the control logic to both Java and native endpoints. Concretely: 1. all of the Java-side code in AppBannerInfoBarDelegateAndroid should be pushed to a new class (e.g. InstallerDelegate). This new class has a both a Java and native side. 2. both Java and native sides define an observer interface, which modal dialogs (Java) and infobar delegates (native) can implement to listen to updates on the installation state 3. once we've fully transitioned we remove any unnecessary duplication
,
Jul 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2f01994e136846a905b7842ff0133ad8682de1c commit b2f01994e136846a905b7842ff0133ad8682de1c Author: Dominick Ng <dominickn@chromium.org> Date: Wed Jul 19 00:00:13 2017 Consolidate native app installation monitoring into InstallerDelegate. Currently, the Java-side AppBannerInfoBarDelegateAndroid contains significant logic for monitoring the progress of a native app or WebAPK installation. This logic would have to be duplicated in order to implement a non-infobar app banner. This CL consolidates installation and state-based logic out of AppBannerInfoBarDelegateAndroid and AppBannerInfoBar and into InstallerDelegate. The InfoBarDelegate becomes an observer of the InstallerDelegate. This is a first step to enabling a modal app install banner, which will also observe the InstallerDelegate in order to update UI as installation proceeds. The way WebAPK installation is handled in AppBannerInfoBarDelegateAndroid is also simplified and consolidated in this CL to make it easier to understand. BUG= 739257 Change-Id: Iaaf5d23e8798e3d213a714cd3194bc83d674d919 Reviewed-on: https://chromium-review.googlesource.com/566350 Reviewed-by: Matthew Jones <mdjones@chromium.org> Reviewed-by: Xi Han <hanxi@chromium.org> Commit-Queue: Dominick Ng <dominickn@chromium.org> Cr-Commit-Position: refs/heads/master@{#487674} [modify] https://crrev.com/b2f01994e136846a905b7842ff0133ad8682de1c/chrome/android/java/src/org/chromium/chrome/browser/banners/InstallerDelegate.java [modify] https://crrev.com/b2f01994e136846a905b7842ff0133ad8682de1c/chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java [modify] https://crrev.com/b2f01994e136846a905b7842ff0133ad8682de1c/chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java [modify] https://crrev.com/b2f01994e136846a905b7842ff0133ad8682de1c/chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java [modify] https://crrev.com/b2f01994e136846a905b7842ff0133ad8682de1c/chrome/android/javatests/src/org/chromium/chrome/browser/banners/InstallerDelegateTest.java [modify] https://crrev.com/b2f01994e136846a905b7842ff0133ad8682de1c/chrome/browser/android/banners/app_banner_infobar_delegate_android.cc [modify] https://crrev.com/b2f01994e136846a905b7842ff0133ad8682de1c/chrome/browser/android/banners/app_banner_infobar_delegate_android.h
,
Jul 19 2017
The following revision refers to this bug: https://chrome-internal.googlesource.com/clank/internal/apps/+/fe27936e12c0842ea700d230c4b3c5861fcb8276 commit fe27936e12c0842ea700d230c4b3c5861fcb8276 Author: Dominick Ng <dominickn@google.com> Date: Wed Jul 19 07:07:16 2017
,
Jul 19 2017
The following revision refers to this bug: https://chrome-internal.googlesource.com/clank/internal/apps/+/8da2be371ff81bd722030437a6938846015d1e77 commit 8da2be371ff81bd722030437a6938846015d1e77 Author: Dominick Ng <dominickn@google.com> Date: Wed Jul 19 07:37:02 2017
,
Jul 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ed841f251871b26ad5d40382455e6bc73b0121fb commit ed841f251871b26ad5d40382455e6bc73b0121fb Author: Mounir Lamouri <mlamouri@chromium.org> Date: Wed Jul 19 10:17:58 2017 Revert "Consolidate native app installation monitoring into InstallerDelegate." This reverts commit b2f01994e136846a905b7842ff0133ad8682de1c. Reason for revert: breaking downstream builds and roller bot Original change's description: > Consolidate native app installation monitoring into InstallerDelegate. > > Currently, the Java-side AppBannerInfoBarDelegateAndroid contains > significant logic for monitoring the progress of a native app or WebAPK > installation. This logic would have to be duplicated in order to > implement a non-infobar app banner. > > This CL consolidates installation and state-based logic out of > AppBannerInfoBarDelegateAndroid and AppBannerInfoBar and into > InstallerDelegate. The InfoBarDelegate becomes an observer of the > InstallerDelegate. This is a first step to enabling a modal app install > banner, which will also observe the InstallerDelegate in order to update > UI as installation proceeds. > > The way WebAPK installation is handled in AppBannerInfoBarDelegateAndroid > is also simplified and consolidated in this CL to make it easier to > understand. > > BUG= 739257 > > Change-Id: Iaaf5d23e8798e3d213a714cd3194bc83d674d919 > Reviewed-on: https://chromium-review.googlesource.com/566350 > Reviewed-by: Matthew Jones <mdjones@chromium.org> > Reviewed-by: Xi Han <hanxi@chromium.org> > Commit-Queue: Dominick Ng <dominickn@chromium.org> > Cr-Commit-Position: refs/heads/master@{#487674} TBR=hanxi@chromium.org,mdjones@chromium.org,pkotwicz@chromium.org,dominickn@chromium.org,piotrs@chromium.org Change-Id: Iaa22572178f2f04fa7a21dfaefcb582042362a3b No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 739257 Reviewed-on: https://chromium-review.googlesource.com/575239 Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Commit-Queue: Mounir Lamouri <mlamouri@chromium.org> Cr-Commit-Position: refs/heads/master@{#487813} [modify] https://crrev.com/ed841f251871b26ad5d40382455e6bc73b0121fb/chrome/android/java/src/org/chromium/chrome/browser/banners/InstallerDelegate.java [modify] https://crrev.com/ed841f251871b26ad5d40382455e6bc73b0121fb/chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java [modify] https://crrev.com/ed841f251871b26ad5d40382455e6bc73b0121fb/chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java [modify] https://crrev.com/ed841f251871b26ad5d40382455e6bc73b0121fb/chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java [modify] https://crrev.com/ed841f251871b26ad5d40382455e6bc73b0121fb/chrome/android/javatests/src/org/chromium/chrome/browser/banners/InstallerDelegateTest.java [modify] https://crrev.com/ed841f251871b26ad5d40382455e6bc73b0121fb/chrome/browser/android/banners/app_banner_infobar_delegate_android.cc [modify] https://crrev.com/ed841f251871b26ad5d40382455e6bc73b0121fb/chrome/browser/android/banners/app_banner_infobar_delegate_android.h
,
Jul 19 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4b7e3eabc822afcccac2da0b935f276b59c9595b commit 4b7e3eabc822afcccac2da0b935f276b59c9595b Author: Dominick Ng <dominickn@chromium.org> Date: Wed Jul 19 13:26:32 2017 Reland "Consolidate native app installation monitoring into InstallerDelegate." This is a reland of b2f01994e136846a905b7842ff0133ad8682de1c Original change's description: > Consolidate native app installation monitoring into InstallerDelegate. > > Currently, the Java-side AppBannerInfoBarDelegateAndroid contains > significant logic for monitoring the progress of a native app or WebAPK > installation. This logic would have to be duplicated in order to > implement a non-infobar app banner. > > This CL consolidates installation and state-based logic out of > AppBannerInfoBarDelegateAndroid and AppBannerInfoBar and into > InstallerDelegate. The InfoBarDelegate becomes an observer of the > InstallerDelegate. This is a first step to enabling a modal app install > banner, which will also observe the InstallerDelegate in order to update > UI as installation proceeds. > > The way WebAPK installation is handled in AppBannerInfoBarDelegateAndroid > is also simplified and consolidated in this CL to make it easier to > understand. > > BUG= 739257 > > Change-Id: Iaaf5d23e8798e3d213a714cd3194bc83d674d919 > Reviewed-on: https://chromium-review.googlesource.com/566350 > Reviewed-by: Matthew Jones <mdjones@chromium.org> > Reviewed-by: Xi Han <hanxi@chromium.org> > Commit-Queue: Dominick Ng <dominickn@chromium.org> > Cr-Commit-Position: refs/heads/master@{#487674} TBR=mdjones@chromium.org Bug: 739257 Change-Id: Ia62a228f1e8122ce63be579e894f80d97017851f Reviewed-on: https://chromium-review.googlesource.com/577607 Reviewed-by: Dominick Ng <dominickn@chromium.org> Commit-Queue: Dominick Ng <dominickn@chromium.org> Cr-Commit-Position: refs/heads/master@{#487843} [modify] https://crrev.com/4b7e3eabc822afcccac2da0b935f276b59c9595b/chrome/android/java/src/org/chromium/chrome/browser/banners/InstallerDelegate.java [modify] https://crrev.com/4b7e3eabc822afcccac2da0b935f276b59c9595b/chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarAndroid.java [modify] https://crrev.com/4b7e3eabc822afcccac2da0b935f276b59c9595b/chrome/android/java/src/org/chromium/chrome/browser/infobar/AppBannerInfoBarDelegateAndroid.java [modify] https://crrev.com/4b7e3eabc822afcccac2da0b935f276b59c9595b/chrome/android/javatests/src/org/chromium/chrome/browser/banners/AppBannerManagerTest.java [modify] https://crrev.com/4b7e3eabc822afcccac2da0b935f276b59c9595b/chrome/android/javatests/src/org/chromium/chrome/browser/banners/InstallerDelegateTest.java [modify] https://crrev.com/4b7e3eabc822afcccac2da0b935f276b59c9595b/chrome/browser/android/banners/app_banner_infobar_delegate_android.cc [modify] https://crrev.com/4b7e3eabc822afcccac2da0b935f276b59c9595b/chrome/browser/android/banners/app_banner_infobar_delegate_android.h
,
Jul 19 2017
,
Jul 19 2017
The following revision refers to this bug: https://chrome-internal.googlesource.com/clank/internal/apps/+/d767a1d470e8bdc194a909b0e25a931e835f88a3 commit d767a1d470e8bdc194a909b0e25a931e835f88a3 Author: Dominick Ng <dominickn@google.com> Date: Wed Jul 19 14:17:35 2017
,
Jul 19 2017
The following revision refers to this bug: https://chrome-internal.googlesource.com/clank/internal/apps/+/36287f8775c905cb490cfbbf493bac94769cf161 commit 36287f8775c905cb490cfbbf493bac94769cf161 Author: Mounir Lamouri <mlamouri@google.com> Date: Wed Jul 19 14:19:30 2017
,
Jul 21 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by dominickn@chromium.org
, Jul 12 2017