New issue
Advanced search Search tips

Issue 753290 link

Starred by 1 user
Status: Verified
Owner:
sdefresne@chromium.org
Closed: Oct 2017
Cc:
edchin@chromium.org
rohitrao@chromium.org
ios-bugs@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 2
Type: Bug



Sign in to add a comment

Design better API for cleaning up Tab's snapshot

Project Member Reported by sdefresne@chromium.org, Aug 8 2017 
https://chromium-review.googlesource.com/c/597407 introduced a IsShuttingDown() method to ApplicationContext to allow deciding whether the snapshot have to be destroyed when a Tab is closed.

This was introduced to fix a regression but the API feels wrong (non-local). A better API needs to be designed.

Possible options:

1. introduce an additional API to inform that a Tab is closed to an user action (like TabStripModel::CloseWebContents does)

2. only cleanup snapshots on startup

Currently, I would say that option 1. is probably the better alternative but will require some refactoring of TabModel and Tab.

Comment 1 by sdefresne@chromium.org, Aug 8 2017

Cc: edchin@chromium.org rohitrao@chromium.org
Components: UI>Browser>Sessions UI>Browser>Mobile>TabSwitcher
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 18 2017 

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

commit 6b776d70c3a1f8a6f39f5b92741c4f95d14033ca
Author: Sylvain Defresne <sdefresne@chromium.org>
Date: Wed Oct 18 21:14:06 2017

Informs observer whether a Tab is closed due to user action.

The Tab snapshot should only be deleted when the Tab is closed due
to user action (or when the Tab is replaced by another in reaction
to user action). In particular, the snapshot should not be deleted
when the application is terminated.

Change WebStateList CloseWebStateAt to take a flagset allowing to
inform the observers whether the Tab was closed due to an action
initiated by the user or not and use it to decide whether the Tab
snapshot should be deleted or not.

Remove global IsShuttingDown() from ApplicationContext.

Bug:  753290 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ie626042e0b16215fd44f9631fb4e14f68c015e86
Reviewed-on: https://chromium-review.googlesource.com/723826
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Rohit Rao (ping after 24h) <rohitrao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509876}
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/app/main_controller.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/application_context.h
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/application_context_impl.cc
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/application_context_impl.h
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/sessions/tab_restore_service_delegate_impl_ios.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/tabs/tab.h
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/tabs/tab.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/tabs/tab_model.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/tabs/tab_model_closing_web_state_observer.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/tabs/tab_unittest.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/ui/browser_list/browser_list_session_service_impl.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/web_state_list/web_state_list.h
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/web_state_list/web_state_list.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/web_state_list/web_state_list_observer.h
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/web_state_list/web_state_list_observer.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/web_state_list/web_state_list_observer_bridge.h
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/browser/web_state_list/web_state_list_observer_bridge.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/test/testing_application_context.h
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/chrome/test/testing_application_context.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/clean/chrome/browser/ui/overlays/overlay_queue_manager_unittest.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/clean/chrome/browser/ui/tab_collection/tab_collection_mediator_unittest.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/clean/chrome/browser/ui/tab_grid/tab_grid_mediator_unittest.mm
[modify] https://crrev.com/6b776d70c3a1f8a6f39f5b92741c4f95d14033ca/ios/clean/chrome/browser/ui/tab_strip/tab_strip_coordinator.mm

Comment 3 by sdefresne@chromium.org, Oct 18 2017

Status: Fixed (was: Assigned)

Comment 4 by subhashi...@chromium.org, Oct 24 2017

Status: Verified (was: Fixed)
Verified on :

App Version: 64.0.3248.0 canary
iOS Version: 10.3.3, 11.1
Devices: iPhone 7, iPhone 8 Plus

1. Tab snapshots are visible in tab switcher mode When app is terminated and launched.
2. Tab snapshots are visible in tab switcher mode when app is upgraded from older to new version of app.
►

Sign in to add a comment