Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2639403003:
Remove Last 1 pages when the tab holding them is destroyed. (Closed)
|
Created:
3 years, 11 months ago by dewittj Modified:
3 years, 10 months ago CC:
chromium-reviews, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, agrieve+watch_chromium.org, dimich+watch_chromium.org, Marc Treib Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove Last 1 pages when the tab holding them is destroyed.
Today, we remove last 1 pages when we navigate a tab, but if it is closed the
page remains until it expires, despite being ineligible for display due to
privacy constraints.
This patch solves much of this issue by simply deleting the pages for a tab when
it is destroyed (aka closed and undo is no longer possible). A future patch
will ensure no bugs remain by monitoring the tab list from startup and filtering
UI surfaces that show Last 1 pages.
BUG=675661
Patch Set 1 #Patch Set 2 : Update comments. #
Total comments: 7
Patch Set 3 : Comments addressed #
Total comments: 1
Patch Set 4 : Adds a test. #
Messages
Total messages: 28 (19 generated)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Remove Last 1 pages when the tab holding them is destroyed. Today, we remove last 1 pages when we navigate a tab, but if it is closed the page remains until it expires, despite being ineligible for display due to privacy constraints. This patch solves much of this issue by simply deleting the pages for a tab when it is destroyed (aka closed and undo is no longer possible). A future patch will ensure no bugs remain by monitoring the tab list from startup and filtering UI surfaces that show Last 1 pages. BUG=675661 ========== to ========== Remove Last 1 pages when the tab holding them is destroyed. Today, we remove last 1 pages when we navigate a tab, but if it is closed the page remains until it expires, despite being ineligible for display due to privacy constraints. This patch solves much of this issue by simply deleting the pages for a tab when it is destroyed (aka closed and undo is no longer possible). A future patch will ensure no bugs remain by monitoring the tab list from startup and filtering UI surfaces that show Last 1 pages. BUG=675661 ==========
dewittj@chromium.org changed reviewers: + dimich@chromium.org, vitaliii@google.com
dewittj@chromium.org changed reviewers: + tedchoc@chromium.org - vitaliii@google.com
tedchoc: ChromeActivity dimich: all treib: FYI
https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:484: if (mTabModelSelectorTabModelObserver != null) mTabModelSelectorTabModelObserver.destroy(); do you plan to add implementation to didAddTab in this CL or later? If later, I think it'd be better to not create an empty hookup now...
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
drive by https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:490: OfflinePageBridge.onTabAdded(tab); Why do we need to track added tabs? Can't we just issue a delete and never worry if it was successful? After all if you cannot find a thing to delete you are pretty much done. Result should be the same and there is less changes to do. Also if you go down that route, I'd put the onDestroyed implementation here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... ... And skip changing ChromeActivity. do you think that would work? https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:455: new ClientId(/* kLastNNamespace */ "last_n", Integer.toString(tab.getId())); look at 28-30
https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:490: OfflinePageBridge.onTabAdded(tab); On 2017/01/19 17:13:31, fgorski wrote: > Why do we need to track added tabs? > Can't we just issue a delete and never worry if it was successful? After all if > you cannot find a thing to delete you are pretty much done. Result should be the > same and there is less changes to do. > > Also if you go down that route, I'd put the onDestroyed implementation here: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > ... And skip changing ChromeActivity. > > do you think that would work? First off, I do think we should find a way to isolate this to something OfflinePage-y, whether that is the tab observer or not. I don't know if you want to hook into onDestroyed as that is likely called when Chrome is shut down but the user did not explicitly close the tab. I think you'll want TabModelObserver#didCloseTab -- potentially tabClosureCommitted, but I think didCloseTab looks right. You'll want to make sure that isn't called when Chrome is killed though (set "Don't keep activities" at the android system level and leave chrome and it should go through the shutdown path).
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dewittj@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL, thanks. https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:484: if (mTabModelSelectorTabModelObserver != null) mTabModelSelectorTabModelObserver.destroy(); On 2017/01/19 02:19:41, Dmitry Titov wrote: > do you plan to add implementation to didAddTab in this CL or later? If later, I > think it'd be better to not create an empty hookup now... Later. Removed. https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:490: OfflinePageBridge.onTabAdded(tab); Thanks for the comments! On 2017/01/19 17:13:31, fgorski wrote: > Why do we need to track added tabs? For future CL, only wanted to make one change to ChromeActivity. > Can't we just issue a delete and never worry if it was successful? After all if > you cannot find a thing to delete you are pretty much done. Result should be the > same and there is less changes to do. Yes, that is what I'm doing. > > Also if you go down that route, I'd put the onDestroyed implementation here: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > ... And skip changing ChromeActivity. > > > > do you think that would work? OfflinePageTabObserver doesn't track every tab, so it doesn't really work for this case. On 2017/01/19 17:49:35, Ted C wrote: > First off, I do think we should find a way to isolate this to something > OfflinePage-y, whether that is the tab observer or not. I need to track all tab closures, so ChromeActivity made some sense as an entry point here. Now that I've reduced it to one line, does that make more sense? Or would you like us to have a set of TabObservers somehwere in offline pages land? > > I don't know if you want to hook into onDestroyed as that is likely called when > Chrome is shut down but the user did not explicitly close the tab. In my testing (create a tab, then swipe away Chrome from the tab switcher) onDestroyed was not called. Are you thinking of a different shut down method? > I think you'll want TabModelObserver#didCloseTab -- potentially > tabClosureCommitted, but I think didCloseTab looks right. Profiles were the reason I didn't just do a pure TabModelObserver. I need one to choose the right OffinePageModel. didCloseTab has no reference to a Tab (just tab ID), so getting a profile was problematic. tabClosureCommitted occurred after a profile was no longer available on the Tab (tab.destroy() was called in finalizeTabClosure) Any advice here? > > You'll want to make sure that isn't called when Chrome is killed though (set > "Don't keep activities" at the android system level and leave chrome and it > should go through the shutdown path). OK, tried that, pages were not deleted. https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java (right): https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:455: new ClientId(/* kLastNNamespace */ "last_n", Integer.toString(tab.getId())); On 2017/01/19 17:13:31, fgorski wrote: > look at 28-30 Done.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/01/19 21:30:27, dewittj wrote: > PTAL, thanks. > > https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... > File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java > (right): > > https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:484: if > (mTabModelSelectorTabModelObserver != null) > mTabModelSelectorTabModelObserver.destroy(); > On 2017/01/19 02:19:41, Dmitry Titov wrote: > > do you plan to add implementation to didAddTab in this CL or later? If later, > I > > think it'd be better to not create an empty hookup now... > > Later. Removed. > > https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:490: > OfflinePageBridge.onTabAdded(tab); > Thanks for the comments! > > On 2017/01/19 17:13:31, fgorski wrote: > > Why do we need to track added tabs? > For future CL, only wanted to make one change to ChromeActivity. > > > Can't we just issue a delete and never worry if it was successful? After all > if > > you cannot find a thing to delete you are pretty much done. Result should be > the > > same and there is less changes to do. > > Yes, that is what I'm doing. > > > > > Also if you go down that route, I'd put the onDestroyed implementation here: > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > ... And skip changing ChromeActivity. > > > > > > do you think that would work? > > OfflinePageTabObserver doesn't track every tab, so it doesn't really work for > this case. > > On 2017/01/19 17:49:35, Ted C wrote: > > First off, I do think we should find a way to isolate this to something > > OfflinePage-y, whether that is the tab observer or not. > > I need to track all tab closures, so ChromeActivity made some sense as an entry > point here. Now that I've reduced it to one line, does that make more sense? Or > would you like us to have a set of TabObservers somehwere in offline pages land? > > > > > I don't know if you want to hook into onDestroyed as that is likely called > when > > Chrome is shut down but the user did not explicitly close the tab. > > In my testing (create a tab, then swipe away Chrome from the tab switcher) > onDestroyed was not called. Are you thinking of a different shut down method? Swipe away is a process kill. "Don't keep activities" is the way to force the full onDestroy path for an Activity. > > > I think you'll want TabModelObserver#didCloseTab -- potentially > > tabClosureCommitted, but I think didCloseTab looks right. > Profiles were the reason I didn't just do a pure TabModelObserver. I need one > to choose the right OffinePageModel. > > didCloseTab has no reference to a Tab (just tab ID), so getting a profile was > problematic. > tabClosureCommitted occurred after a profile was no longer available on the Tab > (tab.destroy() was called in finalizeTabClosure) > > Any advice here? > > > > > You'll want to make sure that isn't called when Chrome is killed though (set > > "Don't keep activities" at the android system level and leave chrome and it > > should go through the shutdown path). > > OK, tried that, pages were not deleted. > > https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... > File > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java > (right): > > https://codereview.chromium.org/2639403003/diff/20001/chrome/android/java/src... > chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:455: > new ClientId(/* kLastNNamespace */ "last_n", Integer.toString(tab.getId())); > On 2017/01/19 17:13:31, fgorski wrote: > > look at 28-30 > > Done.
On 2017/01/20 01:19:08, Ted C wrote: > Swipe away is a process kill. "Don't keep activities" is the way to force > the full onDestroy path for an Activity. Thanks for clarifying. I have tested both "don't keep activities" and swipe away.
lgtm
https://codereview.chromium.org/2639403003/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2639403003/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:522: public void onDestroyed(Tab tab) { By the way, this only works because of the organization of ChromeActivity. onDestroy on Tab is called when the activity is shutdown regardless of whether the tab is closed. It works purely because we unregister this observer before that happens in ChromeActivity#onDestroy. onTabClosed is the correct thing to listen to Just warning you that this might break if anyone adjust this class or moves it around slightly. And there is no way the offline page stuff could get access to the model without ChromeActivity poking it? We want to avoid adding more stuff to this class to make it live closer to the end feature. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
