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

Issue 471763008: Nicely handle OnBeforeUnloads with guest and lock mode. (Closed)

Created:
6 years, 4 months ago by Mike Lerman
Modified:
6 years, 3 months ago
CC:
chromium-reviews, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

The Lock and Guest functions currently try to close all browser windows for their profiles, and any OnBeforeUnload dialogs can cause that closing to fail. This CL takes inspiration from the BrowserCloseManager that is used when an entire browser window is closed. Methods are added to BrowserList that will trigger all OnBeforeUpload handlers, wait for the user's response, and then close the profile's browsers and perform an appropriate success callback. A abort callback is permitted and will be used when ProfileDeletion also uses this flow. Guest and Lock will now only open the User Manager (and lock the profile, for Lock) if no OnBeforeUnload event is canceled. BUG=368497, 289390 TEST=Open a guest profile. Navigate to http://www.4guysfromrolla.com/demos/OnBeforeUnloadDemo1.htm. Click "Exit Guest" in the User Menu. Select "Stay on this page". The window should stay open and the User Manager should not be shown. Open a signed-in and lockable profile. Navigate to the above URL. Lock the profile from the User Menu. Select "Stay on this page". The User Manager should not be shown. Manually opening the User Manager should show that the Profile has not been locked. Committed: https://crrev.com/17267f48ca31c053628657ec7731bfb72f8ea8dc Cr-Commit-Position: refs/heads/master@{#293535}

Patch Set 1 #

Patch Set 2 : working browser closing and lock included #

Total comments: 36

Patch Set 3 : Remove OnFailure callback. Nits and comments. #

Patch Set 4 : Cleanup browser close; callback reset #

Total comments: 34

Patch Set 5 : Better comments, change profile_index to profile_path #

Patch Set 6 : Rebase #

Patch Set 7 : A couple of comments and compiler issues #

Total comments: 8

Patch Set 8 : Change to local static bool with AutoReset #

Total comments: 10

Patch Set 9 : Rebase #

Patch Set 10 : Post-LGTM nits #

Patch Set 11 : Callback before close browsers #

Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -14 lines) Patch
M chrome/browser/profiles/profile_window.cc View 1 2 3 4 5 6 7 8 9 1 chunk +19 lines, -8 lines 0 comments Download
M chrome/browser/ui/browser_list.h View 1 2 3 4 5 6 7 8 9 10 3 chunks +36 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser_list.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +60 lines, -0 lines 0 comments Download
M chrome/browser/ui/unload_controller.cc View 1 2 3 4 5 6 1 chunk +7 lines, -1 line 0 comments Download
M chrome/browser/ui/views/profiles/profile_chooser_view.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -5 lines 0 comments Download

Messages

Total messages: 24 (1 generated)
Mike Lerman
mlerman@chromium.org changed reviewers: + noms@chromium.org, pkasting@chromium.org
6 years, 3 months ago (2014-08-26 19:26:23 UTC) #1
Mike Lerman
Hello, Can I please get a review on this CL? Thanks! noms - c/b/profiles pkasting ...
6 years, 3 months ago (2014-08-26 19:26:23 UTC) #2
noms (inactive)
Just a bunch of initial comments. I think Peter knows more about the BrowserList implementation, ...
6 years, 3 months ago (2014-08-26 20:17:33 UTC) #3
Peter Kasting
https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browser_list.cc File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browser_list.cc#newcode39 chrome/browser/ui/browser_list.cc:39: Browser* current_closing_browser_; Why do you actually need this? You ...
6 years, 3 months ago (2014-08-26 20:26:15 UTC) #4
Mike Lerman
Patchset #3 (id:40001) has been deleted
6 years, 3 months ago (2014-08-27 14:11:01 UTC) #5
Mike Lerman
Note: I'm investigating why browser->window()->Close() is invalidating my BrowserVector::iterator. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/profiles/profile_window.cc File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/profiles/profile_window.cc#newcode262 chrome/browser/profiles/profile_window.cc:262: ...
6 years, 3 months ago (2014-08-27 14:11:39 UTC) #6
Peter Kasting
https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browser_list.cc File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browser_list.cc#newcode39 chrome/browser/ui/browser_list.cc:39: Browser* current_closing_browser_; On 2014/08/27 14:11:38, Mike Lerman wrote: > ...
6 years, 3 months ago (2014-08-27 19:31:34 UTC) #7
Mike Lerman
mlerman@chromium.org changed reviewers: + sammc@chromium.org
6 years, 3 months ago (2014-08-28 15:06:53 UTC) #8
Mike Lerman
Adding @sammc since he knows the UnBeforeUnload and BrowserClose interactions very well. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browser_list.cc File chrome/browser/ui/browser_list.cc ...
6 years, 3 months ago (2014-08-28 15:06:53 UTC) #9
Peter Kasting
https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/profile_window.cc File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/profile_window.cc#newcode273 chrome/browser/profiles/profile_window.cc:273: base::Bind(&GuestBrowserCloseSuccess)); Nit: All lines of args should be aligned ...
6 years, 3 months ago (2014-08-28 18:46:13 UTC) #10
noms (inactive)
https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/profile_window.cc File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/profile_window.cc#newcode277 chrome/browser/profiles/profile_window.cc:277: void LockBrowserCloseSuccess(const size_t profile_index) { I think it would ...
6 years, 3 months ago (2014-08-28 19:22:29 UTC) #11
Mike Lerman
Thanks for the feedback Peter, Monica. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/profile_window.cc File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/profile_window.cc#newcode273 chrome/browser/profiles/profile_window.cc:273: base::Bind(&GuestBrowserCloseSuccess)); On 2014/08/28 ...
6 years, 3 months ago (2014-08-29 18:02:22 UTC) #12
Peter Kasting
https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/browser_list.cc File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/browser_list.cc#newcode187 chrome/browser/ui/browser_list.cc:187: // callback only executes once. It seems like we ...
6 years, 3 months ago (2014-08-29 21:30:26 UTC) #13
Mike Lerman
https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/browser_list.cc File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/browser_list.cc#newcode187 chrome/browser/ui/browser_list.cc:187: // callback only executes once. On 2014/08/29 21:30:25, Peter ...
6 years, 3 months ago (2014-09-02 15:15:41 UTC) #14
Peter Kasting
LGTM https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles/profile_window.cc File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles/profile_window.cc#newcode284 chrome/browser/profiles/profile_window.cc:284: true); Nit: Can place on end of previous ...
6 years, 3 months ago (2014-09-02 19:40:34 UTC) #15
noms (inactive)
profiles lgtm % nits https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles/profile_window.cc File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles/profile_window.cc#newcode274 chrome/browser/profiles/profile_window.cc:274: base::Bind(&GuestBrowserCloseSuccess)); nit: i think the ...
6 years, 3 months ago (2014-09-03 14:25:08 UTC) #16
Mike Lerman
Thank you both! https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles/profile_window.cc File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles/profile_window.cc#newcode274 chrome/browser/profiles/profile_window.cc:274: base::Bind(&GuestBrowserCloseSuccess)); On 2014/09/03 14:25:08, Monica Dinculescu ...
6 years, 3 months ago (2014-09-03 15:28:35 UTC) #17
Mike Lerman
Had to add a small patch for testing purposes. We need to execute the success ...
6 years, 3 months ago (2014-09-04 20:36:56 UTC) #18
noms (inactive)
slgtm
6 years, 3 months ago (2014-09-04 20:43:00 UTC) #19
noms (inactive)
Oh that's not a thing anymore. I see. Still lgtm. On 2014/09/04 20:43:00, Monica Dinculescu ...
6 years, 3 months ago (2014-09-04 20:43:20 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/471763008/240001
6 years, 3 months ago (2014-09-05 15:24:26 UTC) #22
commit-bot: I haz the power
Committed patchset #11 (id:240001) as 285d81f77ef0d1a60dc8c79a4e581b4cb1c6d013
6 years, 3 months ago (2014-09-05 16:43:08 UTC) #23
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 03:40:04 UTC) #24
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/17267f48ca31c053628657ec7731bfb72f8ea8dc
Cr-Commit-Position: refs/heads/master@{#293535}

Powered by Google App Engine
This is Rietveld 408576698