Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1784093002:
Ensure tab is valid when distiller callback is set (Closed)
DescriptionEnsure tab is valid when distiller callback is set
BUG=592032
Committed: https://crrev.com/400ad195011eeea5f10196f517e25e838203f078
Cr-Commit-Position: refs/heads/master@{#381231}
Patch Set 1 #
Total comments: 7
Patch Set 2 : get tab by id #Messages
Total messages: 12 (3 generated)
mdjones@chromium.org changed reviewers: + wychen@chromium.org
PTAL
https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java (right): https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:519: if (tabId == Tab.INVALID_TAB_ID || mTabStatusMap.get(tabId).isCallbackSet()) { Do we need to check mTabStatusMap.containsKey(...) here? https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:519: if (tabId == Tab.INVALID_TAB_ID || mTabStatusMap.get(tabId).isCallbackSet()) { I'm a bit confused about what would happen if tabId and currentTab.getId() is different. Does it make sense to keep using readerTabId, but check readerTabId == Tab.INVALID_TAB_ID?
https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java (right): https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:519: if (tabId == Tab.INVALID_TAB_ID || mTabStatusMap.get(tabId).isCallbackSet()) { On 2016/03/11 07:28:01, wychen wrote: > Do we need to check mTabStatusMap.containsKey(...) here? mTabStatusMap has an entry set for that ID immediately before each call to this function so it should absolutely never be null. There should always be a one-to-one relationship between those objects. https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:519: if (tabId == Tab.INVALID_TAB_ID || mTabStatusMap.get(tabId).isCallbackSet()) { On 2016/03/11 07:28:01, wychen wrote: > I'm a bit confused about what would happen if tabId and currentTab.getId() is > different. > Does it make sense to keep using readerTabId, but check readerTabId == > Tab.INVALID_TAB_ID? tabId will now be the tab that had some event called on in; we always want that tab. I think this is more clear but can change it back if you disagree.
https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java (right): https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:519: if (tabId == Tab.INVALID_TAB_ID || mTabStatusMap.get(tabId).isCallbackSet()) { On 2016/03/11 16:22:27, mdjones wrote: > On 2016/03/11 07:28:01, wychen wrote: > > Do we need to check mTabStatusMap.containsKey(...) here? > > mTabStatusMap has an entry set for that ID immediately before each call to this > function so it should absolutely never be null. There should always be a > one-to-one relationship between those objects. I see. Thanks for the explaination. https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:519: if (tabId == Tab.INVALID_TAB_ID || mTabStatusMap.get(tabId).isCallbackSet()) { On 2016/03/11 16:22:27, mdjones wrote: > On 2016/03/11 07:28:01, wychen wrote: > > I'm a bit confused about what would happen if tabId and currentTab.getId() is > > different. > > Does it make sense to keep using readerTabId, but check readerTabId == > > Tab.INVALID_TAB_ID? > > tabId will now be the tab that had some event called on in; we always want that > tab. I think this is more clear but can change it back if you disagree. I'm still confused. In setDelegate(), currentTab is still used. If tabId is what we want, do we replace currentTab with readerTab?
https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java (right): https://codereview.chromium.org/1784093002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/dom_distiller/ReaderModeManager.java:519: if (tabId == Tab.INVALID_TAB_ID || mTabStatusMap.get(tabId).isCallbackSet()) { On 2016/03/11 21:47:03, wychen wrote: > On 2016/03/11 16:22:27, mdjones wrote: > > On 2016/03/11 07:28:01, wychen wrote: > > > I'm a bit confused about what would happen if tabId and currentTab.getId() > is > > > different. > > > Does it make sense to keep using readerTabId, but check readerTabId == > > > Tab.INVALID_TAB_ID? > > > > tabId will now be the tab that had some event called on in; we always want > that > > tab. I think this is more clear but can change it back if you disagree. > > I'm still confused. In setDelegate(), currentTab is still used. If tabId is what > we want, do we replace currentTab with readerTab? I see what you mean now, good catch! I updated the code to get the tab based on the ID passed in.
lgtm
The CQ bit was checked by mdjones@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1784093002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1784093002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Ensure tab is valid when distiller callback is set BUG=592032 ========== to ========== Ensure tab is valid when distiller callback is set BUG=592032 Committed: https://crrev.com/400ad195011eeea5f10196f517e25e838203f078 Cr-Commit-Position: refs/heads/master@{#381231} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/400ad195011eeea5f10196f517e25e838203f078 Cr-Commit-Position: refs/heads/master@{#381231} |
