Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(28)

Issue 25414005: Clear session-only data when all browser windows are closed. (Closed)

Created:
7 years, 2 months ago by Sam McNally
Modified:
7 years, 2 months ago
CC:
chromium-reviews, marja+watch_chromium.org, chrome-apps-syd-reviews_chromium.org
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Clear session-only data when all browser windows are closed. This clears session-only cookies and cookies and local storage for origins that are set to clear on exit. With this change, closing all browser windows clears session data regardless of whether an app is keeping the browser process alive. BUG=268224 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229281

Patch Set 1 : #

Total comments: 2

Patch Set 2 : rebase #

Patch Set 3 : #

Patch Set 4 : #

Total comments: 10

Patch Set 5 : #

Total comments: 6

Patch Set 6 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+392 lines, -8 lines) Patch
M chrome/browser/background/background_mode_manager.h View 1 2 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/background/background_mode_manager.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/app_background_page_apitest.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/sessions/better_session_restore_browsertest.cc View 1 2 3 4 5 11 chunks +185 lines, -2 lines 0 comments Download
A chrome/browser/sessions/session_data_deleter.h View 1 chunk +15 lines, -0 lines 0 comments Download
A chrome/browser/sessions/session_data_deleter.cc View 1 2 3 4 5 1 chunk +169 lines, -0 lines 0 comments Download
M chrome/browser/sessions/session_service.cc View 1 2 3 4 5 2 chunks +12 lines, -0 lines 0 comments Download
M chrome/browser/sessions/session_service_factory.cc View 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
Sam McNally
7 years, 2 months ago (2013-10-08 00:23:52 UTC) #1
benwells
https://codereview.chromium.org/25414005/diff/120001/chrome/browser/sessions/session_service.cc File chrome/browser/sessions/session_service.cc (right): https://codereview.chromium.org/25414005/diff/120001/chrome/browser/sessions/session_service.cc#newcode342 chrome/browser/sessions/session_service.cc:342: if (!has_open_trackable_browsers_) How does this interact with background mode? ...
7 years, 2 months ago (2013-10-09 00:38:06 UTC) #2
Sam McNally
https://codereview.chromium.org/25414005/diff/120001/chrome/browser/sessions/session_service.cc File chrome/browser/sessions/session_service.cc (right): https://codereview.chromium.org/25414005/diff/120001/chrome/browser/sessions/session_service.cc#newcode342 chrome/browser/sessions/session_service.cc:342: if (!has_open_trackable_browsers_) On 2013/10/09 00:38:06, benwells wrote: > How ...
7 years, 2 months ago (2013-10-14 01:44:23 UTC) #3
benwells
Just nits and questions... https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/better_session_restore_browsertest.cc File chrome/browser/sessions/better_session_restore_browsertest.cc (right): https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/better_session_restore_browsertest.cc#newcode322 chrome/browser/sessions/better_session_restore_browsertest.cc:322: g_browser_process->AddRefModule(); Is there a reason ...
7 years, 2 months ago (2013-10-15 08:28:51 UTC) #4
marja
Hmm, doens't this break the "Continue where I left off" feature, where we keep session-only ...
7 years, 2 months ago (2013-10-15 10:07:45 UTC) #5
marja
On 2013/10/15 10:07:45, marja wrote: > Hmm, doens't this break the "Continue where I left ...
7 years, 2 months ago (2013-10-15 10:08:21 UTC) #6
Sam McNally
On 2013/10/15 10:08:21, marja wrote: > On 2013/10/15 10:07:45, marja wrote: > > Hmm, doens't ...
7 years, 2 months ago (2013-10-15 22:53:05 UTC) #7
Sam McNally
+atwilson for chrome/browser/background
7 years, 2 months ago (2013-10-15 22:57:52 UTC) #8
benwells
lgtm https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/session_service_factory.cc File chrome/browser/sessions/session_service_factory.cc (right): https://codereview.chromium.org/25414005/diff/216001/chrome/browser/sessions/session_service_factory.cc#newcode37 chrome/browser/sessions/session_service_factory.cc:37: DeleteSessionOnlyData(profile); On 2013/10/15 22:53:05, Sam McNally wrote: > ...
7 years, 2 months ago (2013-10-15 22:58:40 UTC) #9
Andrew T Wilson (Slow)
browser/background lgtm.
7 years, 2 months ago (2013-10-16 07:33:57 UTC) #10
marja
(K. Removing myself from reviewer list, my drive-by commenting got me added...)
7 years, 2 months ago (2013-10-16 08:01:28 UTC) #11
Sam McNally
On 2013/10/16 08:01:28, marja wrote: > (K. Removing myself from reviewer list, my drive-by commenting ...
7 years, 2 months ago (2013-10-16 08:26:50 UTC) #12
marja
Alright :) It has been a while since I've worked on this code the last ...
7 years, 2 months ago (2013-10-16 14:18:08 UTC) #13
Sam McNally
https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/session_data_deleter.cc File chrome/browser/sessions/session_data_deleter.cc (right): https://codereview.chromium.org/25414005/diff/235001/chrome/browser/sessions/session_data_deleter.cc#newcode32 chrome/browser/sessions/session_data_deleter.cc:32: bool delete_all_session_cookies); On 2013/10/16 14:18:08, marja wrote: > Naming ...
7 years, 2 months ago (2013-10-16 23:30:59 UTC) #14
marja
lgtm
7 years, 2 months ago (2013-10-17 08:09:17 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sammc@chromium.org/25414005/142008
7 years, 2 months ago (2013-10-17 09:00:55 UTC) #16
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=178365
7 years, 2 months ago (2013-10-17 12:40:46 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sammc@chromium.org/25414005/142008
7 years, 2 months ago (2013-10-17 23:17:53 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sammc@chromium.org/25414005/142008
7 years, 2 months ago (2013-10-18 01:20:16 UTC) #19
commit-bot: I haz the power
7 years, 2 months ago (2013-10-18 02:21:03 UTC) #20
Message was sent while issue was closed.
Change committed as 229281

Powered by Google App Engine
This is Rietveld 408576698