Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1637613003:
We have problems with multiple listeners per tab. (Closed)
|
Created:
4 years, 11 months ago by Pete Williamson Modified:
4 years, 11 months ago Reviewers:
fgorski CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWe have problems with multiple listeners per tab.
Use a list of observers instead of creating a new one each time, so we
only have one per tab.
BUG=577735
Committed: https://crrev.com/62d9666b6323f676507c07c9588f024434247c49
Cr-Commit-Position: refs/heads/master@{#371693}
Patch Set 1 #
Total comments: 19
Patch Set 2 : CR fixes per FGorski #Patch Set 3 : Added notes for later. #Patch Set 4 : Disable listener instead of removing observer. #Patch Set 5 : Make tab observer list package private. #
Messages
Total messages: 17 (7 generated)
petewil@chromium.org changed reviewers: + fgorski@chromium.org
https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (right): https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:29: private static final Map<Integer, OfflinePageConnectivityListener> sConnectivityListeners = Given that you store the listener in the observer, you don't need this anymore I think. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:37: * @param activity The ChromeActivity we are associated with nit: When you start with sentence case and with a period. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:59: // In case we go offline again, ensure our connectivity listener is enabled. we went offline? https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:61: observer.mListener.enable(); nit: because you have setters above, I am thinking this should be observer.enableConnectivityListener... https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:113: // TODO(petewil): In case any snackbars are showing, dismiss them before we switch tabs. A couple of notes for later: a) you will need a handle on snackbar manager (probably through ChromeActivity) b) you will need a handle to the snackbar controller (could be returned from showReload/EditSnackbar https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:119: if (mListener != null) { In what circumstances can a listener be null? https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:122: sConnectivityListeners.remove(destroyedTab.getId()); I think you should be removing observer from the the sTabObservers here. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:130: if (mListener != null) { In what circumstances can a listener be null? https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:131: reloadingTab.removeObserver(this); would this be a good time to remove the observer altogether? If you don't, I think you will never learn that the tab is destroyed, and you will keep a reference to the observer which will never observe anything.
Fixes per FGorski https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (right): https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:29: private static final Map<Integer, OfflinePageConnectivityListener> sConnectivityListeners = On 2016/01/26 18:23:21, fgorski wrote: > Given that you store the listener in the observer, you don't need this anymore I > think. Done. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:37: * @param activity The ChromeActivity we are associated with On 2016/01/26 18:23:21, fgorski wrote: > nit: When you start with sentence case and with a period. Done. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:59: // In case we go offline again, ensure our connectivity listener is enabled. On 2016/01/26 18:23:21, fgorski wrote: > we went offline? Comment expanded a bit, hopefully that clarifies it for you. LMK if it still needs more clarification. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:61: observer.mListener.enable(); On 2016/01/26 18:23:21, fgorski wrote: > nit: because you have setters above, I am thinking this should be > observer.enableConnectivityListener... Done. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:113: // TODO(petewil): In case any snackbars are showing, dismiss them before we switch tabs. On 2016/01/26 18:23:21, fgorski wrote: > A couple of notes for later: > a) you will need a handle on snackbar manager (probably through ChromeActivity) > b) you will need a handle to the snackbar controller (could be returned from > showReload/EditSnackbar Notes added as comments. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:119: if (mListener != null) { On 2016/01/26 18:23:21, fgorski wrote: > In what circumstances can a listener be null? It can't. I was programming defensively by habit from C++. removed this check (and the one below) https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:122: sConnectivityListeners.remove(destroyedTab.getId()); On 2016/01/26 18:23:21, fgorski wrote: > I think you should be removing observer from the the sTabObservers here. Done. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:130: if (mListener != null) { On 2016/01/26 18:23:21, fgorski wrote: > In what circumstances can a listener be null? Done. https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:131: reloadingTab.removeObserver(this); On 2016/01/26 18:23:21, fgorski wrote: > would this be a good time to remove the observer altogether? > > If you don't, I think you will never learn that the tab is destroyed, and you > will keep a reference to the observer which will never observe anything. This is just navigating the existing tab to a new URL. We keep the observer on the tab. If the tab is later destroyed, we should learn about it in the onDestroyed() method (above) and remove the observer there.
lgtm https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java (right): https://codereview.chromium.org/1637613003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageTabObserver.java:131: reloadingTab.removeObserver(this); On 2016/01/26 22:35:00, Pete Williamson wrote: > On 2016/01/26 18:23:21, fgorski wrote: > > would this be a good time to remove the observer altogether? > > > > If you don't, I think you will never learn that the tab is destroyed, and you > > will keep a reference to the observer which will never observe anything. > > This is just navigating the existing tab to a new URL. We keep the observer on > the tab. If the tab is later destroyed, we should learn about it in the > onDestroyed() method (above) and remove the observer there. reloadingTab.removeObserver(this) will prevent the call to onDestroyed() as you are no longer observing the tab... that is why I suggest you get rid of the observer now. Also, there is no point in having observer, if you are no longer on the page with bookmarkId... When you get there, you will simply add a new one I think. Unless you mean to disable the listener here instead.
As we discussed in person, the intent of OnUrlUpdated was to disable the listener, not remove the observer, changed code to reflect intent.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1637613003/#ps60001 (title: "Disable listener instead of removing observer.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637613003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637613003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1637613003/#ps80001 (title: "Make tab observer list package private.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1637613003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1637613003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== We have problems with multiple listeners per tab. Use a list of observers instead of creating a new one each time, so we only have one per tab. BUG=577735 ========== to ========== We have problems with multiple listeners per tab. Use a list of observers instead of creating a new one each time, so we only have one per tab. BUG=577735 Committed: https://crrev.com/62d9666b6323f676507c07c9588f024434247c49 Cr-Commit-Position: refs/heads/master@{#371693} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/62d9666b6323f676507c07c9588f024434247c49 Cr-Commit-Position: refs/heads/master@{#371693} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
