|
|
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. |
DescriptionThe 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 #
Messages
Total messages: 24 (1 generated)
mlerman@chromium.org changed reviewers: + noms@chromium.org, pkasting@chromium.org
Hello, Can I please get a review on this CL? Thanks! noms - c/b/profiles pkasting - c/b/ui and c/b/ui/views/profiles Thanks!
Just a bunch of initial comments. I think Peter knows more about the BrowserList implementation, though, so I'd maybe hold off on my comments before you get his -- I might be wrong about a bunch of things. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_window.cc:262: void GuestBrowserCloseSuccess(const size_t profile_index) { I don't think you need the profile_index as an argument if you're not using it. Also, can you just use base::Bind(&chrome::ShowUserManager) instead? I don't think anyone else calls GuestBrowserCloseSuccess... If not, ShowUserManagerMaybeWithTutorial(null) is also an option. I don't think you need to define this function just to call ShowUserManager https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:140: void BrowserList::CloseAllBrowsersWithProfile(Profile* profile, I'm a little confused because CodeSearch still shows as this being an existing method: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Is CodeSearch just out of sync or Rietveld confused? https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:154: TryToCloseBrowserList(browsers_to_close, on_close_success, Hmm, can this and CloseAllBrowsers just get merged? It doesn't seem to be called anywhere else, and you could just do the if statement from line 165 up after line 147? https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:181: browsers_ready_to_close[i]->window()->Close(); The original function in CodeSearch shows that an iterator used to work (see https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). I think it's important to understand why this no longer works. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:77: // Closes all browsers for |profile| across all desktops. If an OnBeforeUnload nit: maybe add "event"? https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:93: static void DoNothing(size_t var) {} Hmm, I'm not super keen on this going in the public interface. Perhaps the function caller should be the one to define it locally? Though I'm looking at where you call CloseAllBrowsersWithProfile, and you always pass DoNothing for on_close_aborted: maybe then this parameter isn't needed at all? https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:102: // Attempts to close |browser_to_close| while respecting OnBeforeUnloads. nit: OnBeforeUnload events? https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:108: // Called after handling an OnBeforeUnload. Will either try to close more nit: an OnBeforeUnload event? https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:114: bool proceed); Please document all parameters
https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:39: Browser* current_closing_browser_; Why do you actually need this? You only ever use it in PostBeforeUnloadHandlers(), to check whether it's non-NULL, but the only method that queues a call to that always sets this. Is this supposed to cause some kind of automatic bail-out if we get a new call to CloseAllBrowsersWithProfile() while a previous one has not finished executing? https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:140: void BrowserList::CloseAllBrowsersWithProfile(Profile* profile, On 2014/08/26 20:17:32, Monica Dinculescu wrote: > I'm a little confused because CodeSearch still shows as this being an existing > method: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > Is CodeSearch just out of sync or Rietveld confused? I think you're confused; that method is defined just above. It is, however, confusing that we have these two identically-named methods which take different args. Is the top (pre-existing) one no longer needed? If not, please remove it. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:150: ProfileInfoCache& cache = Nit: Prefer pointers or const refs to non-const refs. In some cases, simply avoiding the temp by inlining may be better. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:155: on_close_aborted, profile_index); Nit: All lines of args must be aligned https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:181: browsers_ready_to_close[i]->window()->Close(); On 2014/08/26 20:17:32, Monica Dinculescu wrote: > The original function in CodeSearch shows that an iterator used to work (see > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). > I think it's important to understand why this no longer works. Yeah, the difference between this and the above scares me. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:80: static void CloseAllBrowsersWithProfile(Profile* profile, Nit: Align all args. (Several places) https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:93: static void DoNothing(size_t var) {} On 2014/08/26 20:17:32, Monica Dinculescu wrote: > Hmm, I'm not super keen on this going in the public interface. Perhaps the > function caller should be the one to define it locally? Though I'm looking at > where you call CloseAllBrowsersWithProfile, and you always pass DoNothing for > on_close_aborted: maybe then this parameter isn't needed at all? Yeah, don't define something like this in the class API. If you don't use the parameter, just remove it entirely. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:709: if (browser_->profile()->IsGuestSession()) { Nit: {} no longer necessary here
Patchset #3 (id:40001) has been deleted
Note: I'm investigating why browser->window()->Close() is invalidating my BrowserVector::iterator. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/profiles/... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/profiles/... chrome/browser/profiles/profile_window.cc:262: void GuestBrowserCloseSuccess(const size_t profile_index) { On 2014/08/26 20:17:32, Monica Dinculescu wrote: > I don't think you need the profile_index as an argument if you're not using it. > Also, can you just use base::Bind(&chrome::ShowUserManager) instead? I don't > think anyone else calls GuestBrowserCloseSuccess... > > If not, ShowUserManagerMaybeWithTutorial(null) is also an option. I don't think > you need to define this function just to call ShowUserManager The on_close_confirmed parameter of CloseAllBrowsersWithProfile needs to take a function of signature void(size_t), which is necessary for LockBrowserCloseSuccess. Neither of the method you've mentioned that will directly create the UserManager have that signature, so I think I need to make a method here that matches. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:39: Browser* current_closing_browser_; On 2014/08/26 20:26:15, Peter Kasting wrote: > Why do you actually need this? You only ever use it in > PostBeforeUnloadHandlers(), to check whether it's non-NULL, but the only method > that queues a call to that always sets this. Is this supposed to cause some > kind of automatic bail-out if we get a new call to CloseAllBrowsersWithProfile() > while a previous one has not finished executing? This is done in BrowserCloseManager where I got this code from. It does seem to create an automatic bail-out to make sure we're not trying to close browsers too many times. I don't know the case that would necessitate its creation (and the CL where the code was added didn't provide any insight), but I'd rather be safe than sorry and keep it in. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:140: void BrowserList::CloseAllBrowsersWithProfile(Profile* profile, On 2014/08/26 20:26:15, Peter Kasting wrote: > On 2014/08/26 20:17:32, Monica Dinculescu wrote: > > I'm a little confused because CodeSearch still shows as this being an existing > > method: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > Is CodeSearch just out of sync or Rietveld confused? > > I think you're confused; that method is defined just above. > > It is, however, confusing that we have these two identically-named methods which > take different args. Is the top (pre-existing) one no longer needed? If not, > please remove it. There is one place (Profile Deletion flow) that calls the above method. My plan is to land this CL and then change the last caller onto this method, deleting the method above. I can either mark is as deprecated in a TODO comment, or change one of the names. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:150: ProfileInfoCache& cache = On 2014/08/26 20:26:15, Peter Kasting wrote: > Nit: Prefer pointers or const refs to non-const refs. In some cases, simply > avoiding the temp by inlining may be better. Inlined, since the ProfileInfoCache is only used once here. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:154: TryToCloseBrowserList(browsers_to_close, on_close_success, On 2014/08/26 20:17:32, Monica Dinculescu wrote: > Hmm, can this and CloseAllBrowsers just get merged? It doesn't seem to be called > anywhere else, and you could just do the if statement from line 165 up after > line 147? TryToCloseBrowserList is called in PostBeforeUnloadHandlers, below. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:155: on_close_aborted, profile_index); On 2014/08/26 20:26:15, Peter Kasting wrote: > Nit: All lines of args must be aligned No longer an issue. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:181: browsers_ready_to_close[i]->window()->Close(); On 2014/08/26 20:26:15, Peter Kasting wrote: > On 2014/08/26 20:17:32, Monica Dinculescu wrote: > > The original function in CodeSearch shows that an iterator used to work (see > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). > > I think it's important to understand why this no longer works. > > Yeah, the difference between this and the above scares me. Okay, I shall dig further into why the iterator gets invalidated. I'll come back on another patch/comment. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:77: // Closes all browsers for |profile| across all desktops. If an OnBeforeUnload On 2014/08/26 20:17:32, Monica Dinculescu wrote: > nit: maybe add "event"? Done. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:80: static void CloseAllBrowsersWithProfile(Profile* profile, On 2014/08/26 20:26:15, Peter Kasting wrote: > Nit: Align all args. (Several places) Done (several places). https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:93: static void DoNothing(size_t var) {} On 2014/08/26 20:26:15, Peter Kasting wrote: > On 2014/08/26 20:17:32, Monica Dinculescu wrote: > > Hmm, I'm not super keen on this going in the public interface. Perhaps the > > function caller should be the one to define it locally? Though I'm looking at > > where you call CloseAllBrowsersWithProfile, and you always pass DoNothing for > > on_close_aborted: maybe then this parameter isn't needed at all? > > Yeah, don't define something like this in the class API. If you don't use the > parameter, just remove it entirely. I'll remove the on_close_aborted parameter of CloseAllBrowsersWithProfile until I'm sure I'll need it, when I use that new method for Profile Deletion. That way, I won't need this placeholder method at all. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:102: // Attempts to close |browser_to_close| while respecting OnBeforeUnloads. On 2014/08/26 20:17:32, Monica Dinculescu wrote: > nit: OnBeforeUnload events? Done. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:108: // Called after handling an OnBeforeUnload. Will either try to close more On 2014/08/26 20:17:32, Monica Dinculescu wrote: > nit: an OnBeforeUnload event? Done. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:114: bool proceed); On 2014/08/26 20:17:32, Monica Dinculescu wrote: > Please document all parameters Done. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:709: if (browser_->profile()->IsGuestSession()) { On 2014/08/26 20:26:15, Peter Kasting wrote: > Nit: {} no longer necessary here Bye bye braces! Thanks!
https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:39: Browser* current_closing_browser_; On 2014/08/27 14:11:38, Mike Lerman wrote: > On 2014/08/26 20:26:15, Peter Kasting wrote: > > Why do you actually need this? You only ever use it in > > PostBeforeUnloadHandlers(), to check whether it's non-NULL, but the only > method > > that queues a call to that always sets this. Is this supposed to cause some > > kind of automatic bail-out if we get a new call to > CloseAllBrowsersWithProfile() > > while a previous one has not finished executing? > > This is done in BrowserCloseManager where I got this code from. It does seem to > create an automatic bail-out to make sure we're not trying to close browsers too > many times. I don't know the case that would necessitate its creation (and the > CL where the code was added didn't provide any insight), but I'd rather be safe > than sorry and keep it in. I'm never OK with landing code the author and reviewer don't fully understand. Before you land this, you need to verify for sure that it is necessary and know why, or else remove it. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:140: void BrowserList::CloseAllBrowsersWithProfile(Profile* profile, On 2014/08/27 14:11:38, Mike Lerman wrote: > On 2014/08/26 20:26:15, Peter Kasting wrote: > > On 2014/08/26 20:17:32, Monica Dinculescu wrote: > > > I'm a little confused because CodeSearch still shows as this being an > existing > > > method: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > Is CodeSearch just out of sync or Rietveld confused? > > > > I think you're confused; that method is defined just above. > > > > It is, however, confusing that we have these two identically-named methods > which > > take different args. Is the top (pre-existing) one no longer needed? If not, > > please remove it. > > There is one place (Profile Deletion flow) that calls the above method. My plan > is to land this CL and then change the last caller onto this method, deleting > the method above. Can you do that in this CL? If not, don't worry about doing anything here.
mlerman@chromium.org changed reviewers: + sammc@chromium.org
Adding @sammc since he knows the UnBeforeUnload and BrowserClose interactions very well. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:39: Browser* current_closing_browser_; On 2014/08/27 19:31:34, Peter Kasting wrote: > On 2014/08/27 14:11:38, Mike Lerman wrote: > > On 2014/08/26 20:26:15, Peter Kasting wrote: > > > Why do you actually need this? You only ever use it in > > > PostBeforeUnloadHandlers(), to check whether it's non-NULL, but the only > > method > > > that queues a call to that always sets this. Is this supposed to cause some > > > kind of automatic bail-out if we get a new call to > > CloseAllBrowsersWithProfile() > > > while a previous one has not finished executing? > > > > This is done in BrowserCloseManager where I got this code from. It does seem > to > > create an automatic bail-out to make sure we're not trying to close browsers > too > > many times. I don't know the case that would necessitate its creation (and the > > CL where the code was added didn't provide any insight), but I'd rather be > safe > > than sorry and keep it in. > > I'm never OK with landing code the author and reviewer don't fully understand. > Before you land this, you need to verify for sure that it is necessary and know > why, or else remove it. Answer: If I have multiple browsers open, and accept all the OnBeforeUnloads of the first browser but reject on the second browser window, the callback (PostBeforeUnloadHandlers) will get called for both browsers. We only want to call PostBeforeUnloadHandlers once. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:140: void BrowserList::CloseAllBrowsersWithProfile(Profile* profile, On 2014/08/27 19:31:34, Peter Kasting wrote: > On 2014/08/27 14:11:38, Mike Lerman wrote: > > On 2014/08/26 20:26:15, Peter Kasting wrote: > > > On 2014/08/26 20:17:32, Monica Dinculescu wrote: > > > > I'm a little confused because CodeSearch still shows as this being an > > existing > > > > method: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > Is CodeSearch just out of sync or Rietveld confused? > > > > > > I think you're confused; that method is defined just above. > > > > > > It is, however, confusing that we have these two identically-named methods > > which > > > take different args. Is the top (pre-existing) one no longer needed? If > not, > > > please remove it. > > > > There is one place (Profile Deletion flow) that calls the above method. My > plan > > is to land this CL and then change the last caller onto this method, deleting > > the method above. > > Can you do that in this CL? If not, don't worry about doing anything here. I'm going to see if the TPM's are okay with merging this back to M38 (since guest lives there), so I'd rather keep this CL small. https://codereview.chromium.org/471763008/diff/20001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:181: browsers_ready_to_close[i]->window()->Close(); On 2014/08/27 14:11:38, Mike Lerman wrote: > On 2014/08/26 20:26:15, Peter Kasting wrote: > > On 2014/08/26 20:17:32, Monica Dinculescu wrote: > > > The original function in CodeSearch shows that an iterator used to work (see > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...). > > > I think it's important to understand why this no longer works. > > > > Yeah, the difference between this and the above scares me. > > Okay, I shall dig further into why the iterator gets invalidated. I'll come back > on another patch/comment. The reason there's a difference between here and above is because here I'm interacting directly with the OnBeforeUnload handlers, and above I'm not. Close() calls ShouldClose() which determines if the UnBeforeUnload handlers are processing (which they are), and thus prevents this from executing correctly. I've been able to change this block of code to look like the block above by changing the UnloadController slightly. Added it in for review. Also added @sammc, since he's very familiar with all this code :)
https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_window.cc:273: base::Bind(&GuestBrowserCloseSuccess)); Nit: All lines of args should be aligned (multiple places) https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_window.cc:278: ProfileInfoCache& cache = Nit: Use pointers or const refs rather than non-const refs https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:39: Browser* current_closing_browser_; You don't care about the actual pointer value here, so this could at least be changed to a bool. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:170: on_close_success.Run(profile_index); It surprises me that we run this before we've actually closed the browsers. Is this order important? If so, make the ordering clear in the header comments. Otherwise perhaps we should do this at the end of the function. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:173: it != browsers_to_close.end(); ++it) { Nit: {} not necessary https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:184: // Process only one call if the latter of multiple browsers clicks abort. It's still unclear to me why this code has this effect, probably because CallBeforeUnloadHandlers() itself is poorly documented and so I can't actually understand the control flow among all these different methods and callbacks. I suggest improving the documentation of that function (and its whole class) as well as adding some comments in TryToCloseBrowserList() about why we're setting a variable and the control flow that will lead to us making use of it. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:80: // event causes any browser to not be closed, |on_close_aborted| is called. This comment is now out-of-date. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:83: Profile* profile, const base::Callback<void(size_t)>& on_close_success); Nit: One arg per line https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:100: // Attempt to close |browser_to_close| while respecting OnBeforeUnload events. Nit: Attempt -> Attempts; browser -> browsers https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:105: const base::Callback<void(size_t)>& on_close_confirmed, Nit: Maybe rename this arg |on_close_completed| or |on_close_succeeded| or |on_close_success|? Make the args to CloseAllBrowsersWithProfile() and PostBeforeUnloadHandlers() match the name https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:106: const size_t profile_index); Nit: Pass by const value is unusual; probably should just pass by value (multiple places) https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:109: // user responded that closing the browser tab should proceed or not. If so, Nit: Remove "or not" (makes the sentence technically mean something other than what you intended) https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:110: // call |TryToCloseBrowserList|, passing the parameters |browsers_to_close|, Nit: call -> calls https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:111: // |on_close_confirmed|, and |profile_index|. Otherwise, reset all the Nit: reset -> resets (although perhaps this should be "restores"?) https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:112: // OnBeforeUnload events. Nit: events -> handlers? (You can't reset an event, can you?) https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/unload... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/unload... chrome/browser/ui/unload_controller.cc:305: on_close_confirmed_.Reset(); Clearly order is important here; add comments explaining the control flow and why we need to reset the member before running the callback.
https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_window.cc:277: void LockBrowserCloseSuccess(const size_t profile_index) { I think it would be better if this took the profile path, and not an index. It would make GuestBrowserCloseSuccess look less weird (actually use its parameter) and reduce any sketchiness about the indexes in the ProfileInfoCache (which change whenever the ProfileInfoCache gets resorted)
Thanks for the feedback Peter, Monica. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_window.cc:273: base::Bind(&GuestBrowserCloseSuccess)); On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: All lines of args should be aligned (multiple places) Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_window.cc:277: void LockBrowserCloseSuccess(const size_t profile_index) { On 2014/08/28 19:22:29, Monica Dinculescu wrote: > I think it would be better if this took the profile path, and not an index. It > would make GuestBrowserCloseSuccess look less weird (actually use its parameter) > and reduce any sketchiness about the indexes in the ProfileInfoCache (which > change whenever the ProfileInfoCache gets resorted) Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/profiles/... chrome/browser/profiles/profile_window.cc:278: ProfileInfoCache& cache = On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: Use pointers or const refs rather than non-const refs Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:39: Browser* current_closing_browser_; On 2014/08/28 18:46:12, Peter Kasting wrote: > You don't care about the actual pointer value here, so this could at least be > changed to a bool. Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:170: on_close_success.Run(profile_index); On 2014/08/28 18:46:12, Peter Kasting wrote: > It surprises me that we run this before we've actually closed the browsers. Is > this order important? If so, make the ordering clear in the header comments. > Otherwise perhaps we should do this at the end of the function. Not important. Moved. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:173: it != browsers_to_close.end(); ++it) { On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: {} not necessary Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.cc:184: // Process only one call if the latter of multiple browsers clicks abort. On 2014/08/28 18:46:12, Peter Kasting wrote: > It's still unclear to me why this code has this effect, probably because > CallBeforeUnloadHandlers() itself is poorly documented and so I can't actually > understand the control flow among all these different methods and callbacks. > > I suggest improving the documentation of that function (and its whole class) as > well as adding some comments in TryToCloseBrowserList() about why we're setting > a variable and the control flow that will lead to us making use of it. I've improved the documentation in browser_list.h that describes the control flow among the methods here. I think this adds clarity to the control flow. The UnloadController.h seems to have some good documentation on both the private and public methods. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:80: // event causes any browser to not be closed, |on_close_aborted| is called. On 2014/08/28 18:46:13, Peter Kasting wrote: > This comment is now out-of-date. Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:83: Profile* profile, const base::Callback<void(size_t)>& on_close_success); On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: One arg per line Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:100: // Attempt to close |browser_to_close| while respecting OnBeforeUnload events. On 2014/08/28 18:46:13, Peter Kasting wrote: > Nit: Attempt -> Attempts; browser -> browsers Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:105: const base::Callback<void(size_t)>& on_close_confirmed, On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: Maybe rename this arg |on_close_completed| or |on_close_succeeded| or > |on_close_success|? Make the args to CloseAllBrowsersWithProfile() and > PostBeforeUnloadHandlers() match the name Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:106: const size_t profile_index); On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: Pass by const value is unusual; probably should just pass by value > (multiple places) This parameter has changed from a size_t to a base::FilePath& (as par noms's comment), so I'll keep it as a const base::FilePath&. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:109: // user responded that closing the browser tab should proceed or not. If so, On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: Remove "or not" (makes the sentence technically mean something other than > what you intended) Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:110: // call |TryToCloseBrowserList|, passing the parameters |browsers_to_close|, On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: call -> calls Done. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:111: // |on_close_confirmed|, and |profile_index|. Otherwise, reset all the On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: reset -> resets (although perhaps this should be "restores"?) Done. (the method called is ResetBeforeUnloadHandlers(), so I'll keep reset). https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/browse... chrome/browser/ui/browser_list.h:112: // OnBeforeUnload events. On 2014/08/28 18:46:12, Peter Kasting wrote: > Nit: events -> handlers? (You can't reset an event, can you?) How about "event handlers"? It's really the event that's being handled, and the handlers that's being reset. https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/unload... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/471763008/diff/80001/chrome/browser/ui/unload... chrome/browser/ui/unload_controller.cc:305: on_close_confirmed_.Reset(); On 2014/08/28 18:46:13, Peter Kasting wrote: > Clearly order is important here; add comments explaining the control flow and > why we need to reset the member before running the callback. Done.
https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:187: // callback only executes once. It seems like we can simplify this to avoid the member variable by using a local static: void PostBeforeUnloadHandlers() { // We need this bool to avoid infinite recursion when resetting the // BeforeUnload handlers, since doing that will trigger calls back to this // method for each affected window. static bool resetting_handlers = false; if (tab_close_confirmed) { ... } else if (!resetting_handlers) { base::AutoReset<bool> resetting_handlers_scoper(&resetting_handlers, true); for () ResetBeforeUnloadHandlers(); } } https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:85: // OnBeforeUnload event. If all OnBeforeUnload events are confirmed, Nit: Second sentence might be clearer as: Uses TryToCloseBrowserList() to do the actual closing and trigger any OnBeforeUnload events. https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:118: // true, call |TryToCloseBrowserList|, passing the parameters Nit: call |TryToCloseBrowserList| -> calls TryToCloseBrowserList() https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:120: // reset all the OnBeforeUnload event handlers and processing ends. Nit: reset -> resets; remove "and processing ends".
https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:187: // callback only executes once. On 2014/08/29 21:30:25, Peter Kasting wrote: > It seems like we can simplify this to avoid the member variable by using a local > static: > > void PostBeforeUnloadHandlers() { > // We need this bool to avoid infinite recursion when resetting the > // BeforeUnload handlers, since doing that will trigger calls back to this > // method for each affected window. > static bool resetting_handlers = false; > if (tab_close_confirmed) { > ... > } else if (!resetting_handlers) { > base::AutoReset<bool> resetting_handlers_scoper(&resetting_handlers, true); > for () > ResetBeforeUnloadHandlers(); > } > } I like that AutoReset class! And done. https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:85: // OnBeforeUnload event. If all OnBeforeUnload events are confirmed, On 2014/08/29 21:30:25, Peter Kasting wrote: > Nit: Second sentence might be clearer as: > > Uses TryToCloseBrowserList() to do the actual closing and trigger any > OnBeforeUnload events. Done. https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:118: // true, call |TryToCloseBrowserList|, passing the parameters On 2014/08/29 21:30:26, Peter Kasting wrote: > Nit: call |TryToCloseBrowserList| -> calls TryToCloseBrowserList() Done. https://codereview.chromium.org/471763008/diff/160001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:120: // reset all the OnBeforeUnload event handlers and processing ends. On 2014/08/29 21:30:25, Peter Kasting wrote: > Nit: reset -> resets; remove "and processing ends". Done.
LGTM https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:284: true); Nit: Can place on end of previous line if you want https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:291: BrowserList::CloseAllBrowsersWithProfile(profile, Nit: Place |profile| on next line; you may leave the 2nd arg on that same line if you want (If you end up with multiple lines of args, they need to start at the same position.) https://codereview.chromium.org/471763008/diff/180001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:186: it != browsers_to_close.end(); ++it) { Nit: Indent one more
profiles lgtm % nits https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:274: base::Bind(&GuestBrowserCloseSuccess)); nit: i think the base::Bind can go on the same line as profile https://codereview.chromium.org/471763008/diff/180001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:23: class FilePath; nit: I these should be sorted alphabetically by namespace name (so base before chrome)
Thank you both! https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles... File chrome/browser/profiles/profile_window.cc (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:274: base::Bind(&GuestBrowserCloseSuccess)); On 2014/09/03 14:25:08, Monica Dinculescu wrote: > nit: i think the base::Bind can go on the same line as profile Done. https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:284: true); On 2014/09/02 19:40:34, Peter Kasting wrote: > Nit: Can place on end of previous line if you want Done. https://codereview.chromium.org/471763008/diff/180001/chrome/browser/profiles... chrome/browser/profiles/profile_window.cc:291: BrowserList::CloseAllBrowsersWithProfile(profile, On 2014/09/02 19:40:34, Peter Kasting wrote: > Nit: Place |profile| on next line; you may leave the 2nd arg on that same line > if you want > > (If you end up with multiple lines of args, they need to start at the same > position.) Done. https://codereview.chromium.org/471763008/diff/180001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.cc:186: it != browsers_to_close.end(); ++it) { On 2014/09/02 19:40:34, Peter Kasting wrote: > Nit: Indent one more Done. https://codereview.chromium.org/471763008/diff/180001/chrome/browser/ui/brows... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/471763008/diff/180001/chrome/browser/ui/brows... chrome/browser/ui/browser_list.h:23: class FilePath; On 2014/09/03 14:25:08, Monica Dinculescu wrote: > nit: I these should be sorted alphabetically by namespace name (so base before > chrome) Done.
Had to add a small patch for testing purposes. We need to execute the success callback before closing the browser windows. Some tests were failing. The previous flow within profile_window.cc was to always close the browser windows last, after doing "other things" that needed doing for Lock or Guest, so we preserve that now. Peter, Monica, if you have any comments let me know.
slgtm
Oh that's not a thing anymore. I see. Still lgtm. On 2014/09/04 20:43:00, Monica Dinculescu wrote: > slgtm
The CQ bit was checked by mlerman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mlerman@chromium.org/471763008/240001
Message was sent while issue was closed.
Committed patchset #11 (id:240001) as 285d81f77ef0d1a60dc8c79a4e581b4cb1c6d013
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/17267f48ca31c053628657ec7731bfb72f8ea8dc Cr-Commit-Position: refs/heads/master@{#293535} |