Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681203002:
Add Browser::SkipCallBeforeUnload so that the browser windows can be closed regardless of beforeunl… (Closed)
DescriptionAdd Browser::SkipCallBeforeUnload so that the browser windows can be closed regardless of beforeunload events.
This function will be used when force-sign-in policy is enabled. When the auth token become invalid, this policy
will close all browser windows, sign out user and display UserManager. Implement this function so that the window
closing process won't be blocked by the webpage contains onbeforeunload event.
BUG=642059
Review-Url: https://codereview.chromium.org/2681203002
Cr-Commit-Position: refs/heads/master@{#457206}
Committed: https://chromium.googlesource.com/chromium/src/+/9ea988f681de3e681c720e6046ceea96a0659783
Patch Set 1 #Patch Set 2 : refactor #
Total comments: 3
Patch Set 3 : sky's comments #
Total comments: 1
Patch Set 4 : comments #
Total comments: 18
Patch Set 5 : Charlie's comments #Patch Set 6 : rebase from master #
Total comments: 18
Patch Set 7 : cr #Patch Set 8 : cr - remove delegate in UnloadController #Messages
Total messages: 68 (43 generated)
The CQ bit was checked by zmin@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: This issue passed the CQ dry run.
zmin@chromium.org changed reviewers: + creis@chromium.org, sky@chromium.org
Hi Scott, Can you review this CL please? Thanks, Owen
Why don't we skip the unload handlers entirely in this case? I'm specifically suggesting the functions in Browser that call to the unload controllers don't call to the unload controllers. Also, what if the unload handlers are already processing an unload? Will they be left in a weird state? Lastly, I don't want another CloseAllBrowsers() variant, we already have two. Please add an enum indicating behavior to the existing ones.
On 2017/02/09 20:37:51, sky wrote: > Why don't we skip the unload handlers entirely in this case? I'm specifically > suggesting the functions in Browser that call to the unload controllers don't > call to the unload controllers. Skip the handler here will ends up with calling the |on_close_success| while the window closing will still be blocked as the beforeunload will be triggered somewhere else. Also, I can rename CallBeforeUnloadHandlers to TryWindowClosing() and ResetBeforeUnloadHandlers() to ResetWindowClosing(). Because these two functions are not only cancel/execute beforehandler events anyway. They are also (cancel)closing pages. > Also, what if the unload handlers are already > processing an unload? Will they be left in a weird state? Good question! FastUnloadHandler will be blocked if there's any ongoing prompt. I'll just skip the blocking and move the page to the next phase directly. UnloadHandler won't check whether there is ongoing prompt or not. However, the closing will still be blocked by the prompt. So i just delete the WebContents after web_contents->ClosePage() which is exactly FastUnloadHandler will do. I'll also add two test cases for this situation. > Lastly, I don't want another CloseAllBrowsers() variant, we already have two. > Please add an enum indicating behavior to the existing ones. Ok, I'll do that.
The CQ bit was checked by zmin@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...
Code updated. On 2017/02/10 00:58:59, zmin wrote: > On 2017/02/09 20:37:51, sky wrote: > > Why don't we skip the unload handlers entirely in this case? I'm specifically > > suggesting the functions in Browser that call to the unload controllers don't > > call to the unload controllers. > Skip the handler here will ends up with calling the |on_close_success| while the > window closing will still be blocked as the beforeunload will be triggered > somewhere else. > Also, I can rename CallBeforeUnloadHandlers to TryWindowClosing() and > ResetBeforeUnloadHandlers() to ResetWindowClosing(). > Because these two functions are not only cancel/execute beforehandler events > anyway. They are also (cancel)closing pages. > > > Also, what if the unload handlers are already > > processing an unload? Will they be left in a weird state? > Good question! FastUnloadHandler will be blocked if there's any ongoing prompt. > I'll just skip the blocking and move the page to the next phase directly. > UnloadHandler won't check whether there is ongoing prompt or not. However, the > closing will still be blocked by the prompt. So i just delete the WebContents > after web_contents->ClosePage() which is exactly FastUnloadHandler will do. > > I'll also add two test cases for this situation. > > > > Lastly, I don't want another CloseAllBrowsers() variant, we already have two. > > Please add an enum indicating behavior to the existing ones. > Ok, I'll do that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please add test coverage of this too. https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:328: // ResetBeforeUnloadHandlers() to reset all beforeunload handlers; calling Update comment. https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:332: // If |skip_before_unload_event| is set to true, all prompting will be I'm confused by the behavior of skip_before_unload_event. Can't this function return false if skip_before_unload_event is true? https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:340: void ResetCloseWindow(); It isn't obvious from this name it ties with TryToCloseWindow. Could you name it ResetAttemptToCloseWindow? Or ResetTryToCloseWindow?
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
Patchset #3 (id:40001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by zmin@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...
On 2017/02/13 16:55:51, sky wrote: > Please add test coverage of this too. Which part you want to test? I have already tested the force close and ongoing normal close + force close. > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > File chrome/browser/ui/browser.h (right): > > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > chrome/browser/ui/browser.h:328: // ResetBeforeUnloadHandlers() to reset all > beforeunload handlers; calling > Update comment. Done > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > chrome/browser/ui/browser.h:332: // If |skip_before_unload_event| is set to > true, all prompting will be > I'm confused by the behavior of skip_before_unload_event. Can't this function > return false if skip_before_unload_event is true? The comments I wrote is not 100% proper, so i rewrite them. The return value indicates whether there is beforeunload handlers and the on_close_confirmed will be called or not . skip_before_unload_event won't affect the return value. > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > chrome/browser/ui/browser.h:340: void ResetCloseWindow(); > It isn't obvious from this name it ties with TryToCloseWindow. Could you name it > ResetAttemptToCloseWindow? Or ResetTryToCloseWindow? Done
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/13 20:57:29, zmin wrote: > On 2017/02/13 16:55:51, sky wrote: > > Please add test coverage of this too. > Which part you want to test? I have already tested the force close and ongoing > normal close + force close. My mistake, I missed that. > > > > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > > File chrome/browser/ui/browser.h (right): > > > > > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > > chrome/browser/ui/browser.h:328: // ResetBeforeUnloadHandlers() to reset all > > beforeunload handlers; calling > > Update comment. > Done > > > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > > chrome/browser/ui/browser.h:332: // If |skip_before_unload_event| is set to > > true, all prompting will be > > I'm confused by the behavior of skip_before_unload_event. Can't this function > > return false if skip_before_unload_event is true? > > The comments I wrote is not 100% proper, so i rewrite them. > The return value indicates whether there is beforeunload handlers and the > on_close_confirmed will be called or not . > skip_before_unload_event won't affect the return value. I'm still confused by why you don't return false vs using the callback. Why do you need to use the callback for the case of skip_before_unload_event. > > > > > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > > chrome/browser/ui/browser.h:340: void ResetCloseWindow(); > > It isn't obvious from this name it ties with TryToCloseWindow. Could you name > it > > ResetAttemptToCloseWindow? Or ResetTryToCloseWindow? > Done
Minor comment on new wording. https://codereview.chromium.org/2681203002/diff/80001/chrome/browser/ui/brows... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2681203002/diff/80001/chrome/browser/ui/brows... chrome/browser/ui/browser.h:328: // and window closing will be confirmed without showing the prompting. prompting -> prompt
creis@chromium.org changed required reviewers: + creis@chromium.org
Sorry I haven't had time to review this yet-- I'm just returning from vacation. The last time we discussed this I wasn't happy with this approach, but maybe that conversation reached a different conclusion. I'll try to take a look tomorrow.
The CQ bit was checked by zmin@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...
On 2017/02/13 23:12:35, sky wrote: > On 2017/02/13 20:57:29, zmin wrote: > > On 2017/02/13 16:55:51, sky wrote: > > > Please add test coverage of this too. > > Which part you want to test? I have already tested the force close and ongoing > > normal close + force close. > > My mistake, I missed that. > > > > > > > > > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > > > File chrome/browser/ui/browser.h (right): > > > > > > > > > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > > > chrome/browser/ui/browser.h:328: // ResetBeforeUnloadHandlers() to reset all > > > beforeunload handlers; calling > > > Update comment. > > Done > > > > > > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > > > chrome/browser/ui/browser.h:332: // If |skip_before_unload_event| is set to > > > true, all prompting will be > > > I'm confused by the behavior of skip_before_unload_event. Can't this > function > > > return false if skip_before_unload_event is true? > > > > The comments I wrote is not 100% proper, so i rewrite them. > > The return value indicates whether there is beforeunload handlers and the > > on_close_confirmed will be called or not . > > skip_before_unload_event won't affect the return value. > > I'm still confused by why you don't return false vs using the callback. Why do > you need to use the callback for the case of skip_before_unload_event. I was thinking that "beforeunload handlers always means callback" might be easy for people to understand the codw. So I just changed it to the "return false and don't use callback if |skip_before_unload_event| is true" Also, update the comments. > > > > > > > > > > https://codereview.chromium.org/2681203002/diff/20001/chrome/browser/ui/brows... > > > chrome/browser/ui/browser.h:340: void ResetCloseWindow(); > > > It isn't obvious from this name it ties with TryToCloseWindow. Could you > name > > it > > > ResetAttemptToCloseWindow? Or ResetTryToCloseWindow? > > Done
On 2017/02/14 00:52:51, Charlie Reis wrote: > Sorry I haven't had time to review this yet-- I'm just returning from vacation. > The last time we discussed this I wasn't happy with this approach, but maybe > that conversation reached a different conclusion. I'll try to take a look > tomorrow. Hi Charlie, Thanks a lot. There are no more response in that thread so I thought you're ok. Also, actually (which I totally forget when I sent that email few weeks ago) the closing all browser window section was in the design doc when it's sent to the design review (it was hiding all browser window at the very beginning, and then change to close later) However, I remove that section later as I thought I have found a way to not do that. Unfortunately it turns out not true and that's why I sent the email to looking for the details. I have put that section back with more details.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for updating the code! Given Charlie's concerns I'm going to hold off on this until he responds. -Scott On Mon, Feb 13, 2017 at 7:10 PM, <zmin@chromium.org> wrote: > On 2017/02/14 00:52:51, Charlie Reis wrote: >> Sorry I haven't had time to review this yet-- I'm just returning from > vacation. >> The last time we discussed this I wasn't happy with this approach, but >> maybe >> that conversation reached a different conclusion. I'll try to take a look >> tomorrow. > > Hi Charlie, > > Thanks a lot. There are no more response in that thread so I thought you're > ok. > Also, actually (which I totally forget when I sent that email few weeks ago) > the > closing all browser window section was in the design doc when it's sent to > the > design review > (it was hiding all browser window at the very beginning, and then change to > close later) > However, I remove that section later as I thought I have found a way to not > do > that. Unfortunately it turns out not true and that's why I sent the email to > looking for the details. > I have put that section back with more details. > > https://codereview.chromium.org/2681203002/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/14 04:21:26, sky wrote: > Thanks for updating the code! > > Given Charlie's concerns I'm going to hold off on this until he responds. > > -Scott > > On Mon, Feb 13, 2017 at 7:10 PM, <mailto:zmin@chromium.org> wrote: > > On 2017/02/14 00:52:51, Charlie Reis wrote: > >> Sorry I haven't had time to review this yet-- I'm just returning from > > vacation. > >> The last time we discussed this I wasn't happy with this approach, but > >> maybe > >> that conversation reached a different conclusion. I'll try to take a look > >> tomorrow. > > > > Hi Charlie, > > > > Thanks a lot. There are no more response in that thread so I thought you're > > ok. > > Also, actually (which I totally forget when I sent that email few weeks ago) > > the > > closing all browser window section was in the design doc when it's sent to > > the > > design review > > (it was hiding all browser window at the very beginning, and then change to > > close later) > > However, I remove that section later as I thought I have found a way to not > > do > > that. Unfortunately it turns out not true and that's why I sent the email to > > looking for the details. > > I have put that section back with more details. > > > > https://codereview.chromium.org/2681203002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > I put a few comments in the doc and revived the email thread, so let's resolve the concerns there to find a good way to proceed. Thanks for your patience!
creis@chromium.org changed required reviewers: + sky@chromium.org - creis@chromium.org
Thanks for addressing my concerns on the design doc. The general approach here LGTM for the remaining cases that need this behavior, but please wait for sky@ to finish reviewing the details. https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:325: // return false. If |skip_before_unload_event| is true, all beforeunload Please add a very clear disclaimer here that beforeunload should never be skipped without first giving the user a chance to save their work. (Same if there are other entry points you think developers might use.) https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:335: bool TryToCloseWindow(bool skip_before_unload_event, You can probably simplify all of these to skip_beforeunload. (beforeunload is the name of the event, without spaces or underscores. The "event" part is fairly clear from the name, at least from my perspective.) https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.cc:28: BrowserList::BrowserVector GetBrowserToClose(Profile* profile) { GetBrowsersToClose (plural) https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:97: bool is_force); nit: skip_beforeunload https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:127: const bool is_force); skip_beforeunload https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:139: const bool is_force, skip_beforeunload https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/fast... File chrome/browser/ui/fast_unload_controller.h (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/fast... chrome/browser/ui/fast_unload_controller.h:145: // Processes the next tab that needs it's beforeunload/unload event fired. Unrelated nit, while we're here: its (no apostrophe) :) https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:318: if (contents->GetRenderViewHost()) What's the purpose of this GetRenderViewHost() check here? (Maybe include a comment?) https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller_web_contents_delegate.h (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller_web_contents_delegate.h:10: class UnloadControllerWebContentsDelegate Please include a class comment saying what the purpose of this delegate is.
The CQ bit was checked by zmin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:325: // return false. If |skip_before_unload_event| is true, all beforeunload On 2017/02/27 21:47:51, Charlie Reis (OOO til Mar 6) wrote: > Please add a very clear disclaimer here that beforeunload should never be > skipped without first giving the user a chance to save their work. (Same if > there are other entry points you think developers might use.) Done. https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:335: bool TryToCloseWindow(bool skip_before_unload_event, On 2017/02/27 21:47:51, Charlie Reis (OOO til Mar 6) wrote: > You can probably simplify all of these to skip_beforeunload. (beforeunload is > the name of the event, without spaces or underscores. The "event" part is > fairly clear from the name, at least from my perspective.) Done. https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.cc (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.cc:28: BrowserList::BrowserVector GetBrowserToClose(Profile* profile) { On 2017/02/27 21:47:51, Charlie Reis (OOO til Mar 6) wrote: > GetBrowsersToClose (plural) Done. https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... File chrome/browser/ui/browser_list.h (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:97: bool is_force); On 2017/02/27 21:47:51, Charlie Reis (OOO til Mar 6) wrote: > nit: skip_beforeunload Done. https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:127: const bool is_force); On 2017/02/27 21:47:51, Charlie Reis (OOO til Mar 6) wrote: > skip_beforeunload Done. https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/brow... chrome/browser/ui/browser_list.h:139: const bool is_force, On 2017/02/27 21:47:51, Charlie Reis (OOO til Mar 6) wrote: > skip_beforeunload Done. https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/fast... File chrome/browser/ui/fast_unload_controller.h (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/fast... chrome/browser/ui/fast_unload_controller.h:145: // Processes the next tab that needs it's beforeunload/unload event fired. On 2017/02/27 21:47:51, Charlie Reis (OOO til Mar 6) wrote: > Unrelated nit, while we're here: its (no apostrophe) > :) Done. https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:318: if (contents->GetRenderViewHost()) On 2017/02/27 21:47:51, Charlie Reis (OOO til Mar 6) wrote: > What's the purpose of this GetRenderViewHost() check here? (Maybe include a > comment?) I add the check here because FastUnloadController does that before setting the delegate. However, the comments in FastUnloadController says "Null check render_View_host here as this gets called on a PostTask and the tab's render_view_host may have been nulled out." And the close process has been changed to sync mode if the |skip_beforeunload| is true. So i removed the check here. https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller_web_contents_delegate.h (right): https://codereview.chromium.org/2681203002/diff/100001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller_web_contents_delegate.h:10: class UnloadControllerWebContentsDelegate On 2017/02/27 21:47:51, Charlie Reis (OOO til Mar 6) wrote: > Please include a class comment saying what the purpose of this delegate is. Done.
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by zmin@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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Hi sky, Can you take a look this CL again? Thanks, Owen
https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/lifetim... File chrome/browser/lifetime/browser_close_manager.cc (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/lifetim... chrome/browser/lifetime/browser_close_manager.cc:64: for (auto* browser : *BrowserList::GetInstance()) { no {} https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/fast... File chrome/browser/ui/fast_unload_controller.cc (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/fast... chrome/browser/ui/fast_unload_controller.cc:41: detached_delegate_(new UnloadControllerWebContentsDelegate()), MakeUnique https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/fast... chrome/browser/ui/fast_unload_controller.cc:194: return true && !skip_beforeunload; return !skip_beforeunload? https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:185: return true && !skip_beforeunload; return !skip_beforeunload https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:317: web_contents_delegate_.reset(new UnloadControllerWebContentsDelegate()); MakeUnique https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:320: (*it)->SetDelegate(web_contents_delegate_.get()); How do you know it's safe to reset the delegate? Might there already be a delegate that wants to track the WebContents? https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller_web_contents_delegate.h (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller_web_contents_delegate.h:12: // that the existed dialogs will be closed as well. It's used by exited -> existing
(Thanks-- just a few other minor grammar nits to clarify it.) https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:343: // has a chance to save his or her work before closing window without trigger nit: s/has/have/ nit: before the window is closed without triggering
The CQ bit was checked by zmin@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...
https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/lifetim... File chrome/browser/lifetime/browser_close_manager.cc (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/lifetim... chrome/browser/lifetime/browser_close_manager.cc:64: for (auto* browser : *BrowserList::GetInstance()) { On 2017/03/14 00:04:05, sky wrote: > no {} Done. https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/brow... File chrome/browser/ui/browser.h (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/brow... chrome/browser/ui/browser.h:343: // has a chance to save his or her work before closing window without trigger On 2017/03/14 00:06:30, Charlie Reis (slow) wrote: > nit: s/has/have/ > nit: before the window is closed without triggering Done. https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/fast... File chrome/browser/ui/fast_unload_controller.cc (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/fast... chrome/browser/ui/fast_unload_controller.cc:41: detached_delegate_(new UnloadControllerWebContentsDelegate()), On 2017/03/14 00:04:05, sky wrote: > MakeUnique Done. https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/fast... chrome/browser/ui/fast_unload_controller.cc:194: return true && !skip_beforeunload; On 2017/03/14 00:04:05, sky wrote: > return !skip_beforeunload? Done. https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:185: return true && !skip_beforeunload; On 2017/03/14 00:04:05, sky wrote: > return !skip_beforeunload Done. https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:317: web_contents_delegate_.reset(new UnloadControllerWebContentsDelegate()); On 2017/03/14 00:04:05, sky wrote: > MakeUnique Done. https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:320: (*it)->SetDelegate(web_contents_delegate_.get()); On 2017/03/14 00:04:05, sky wrote: > How do you know it's safe to reset the delegate? Might there already be a > delegate that wants to track the WebContents? This delegate makes sure that after windows are closed, there is no opened dialog left behind. I think it's safe because FastUnloadController has been using it. https://cs.chromium.org/chromium/src/chrome/browser/ui/fast_unload_controller... https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller_web_contents_delegate.h (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller_web_contents_delegate.h:12: // that the existed dialogs will be closed as well. It's used by On 2017/03/14 00:04:05, sky wrote: > exited -> existing Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:320: (*it)->SetDelegate(web_contents_delegate_.get()); On 2017/03/14 17:46:47, zmin wrote: > On 2017/03/14 00:04:05, sky wrote: > > How do you know it's safe to reset the delegate? Might there already be a > > delegate that wants to track the WebContents? > > This delegate makes sure that after windows are closed, there is no opened > dialog left behind. I think it's safe because FastUnloadController has been > using it. > > https://cs.chromium.org/chromium/src/chrome/browser/ui/fast_unload_controller... AFAIK FastUnloadController isn't shipping yet, so I'm concerned. Charlie, does this look problematic to you too?
https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... File chrome/browser/ui/unload_controller.cc (right): https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... chrome/browser/ui/unload_controller.cc:320: (*it)->SetDelegate(web_contents_delegate_.get()); On 2017/03/14 20:26:34, sky wrote: > On 2017/03/14 17:46:47, zmin wrote: > > On 2017/03/14 00:04:05, sky wrote: > > > How do you know it's safe to reset the delegate? Might there already be a > > > delegate that wants to track the WebContents? > > > > This delegate makes sure that after windows are closed, there is no opened > > dialog left behind. I think it's safe because FastUnloadController has been > > using it. > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/fast_unload_controller... > > AFAIK FastUnloadController isn't shipping yet, so I'm concerned. Charlie, does > this look problematic to you too? That's a great question. It looks like it depends on what tabs_needing_unload_fired_ can contain, and whether it overlaps with any of the (many) users of WebContents::SetDelegate. It might be worth checking with whomever works on FastUnloadController to see if they've reasoned about why it's safe. At first glance, there's a ton of features that create a WebContents and call SetDelegate on it, many of which are under our control and might be unlikely to have unload handlers (e.g., ChromeOS first run, login, etc). Some seem less obvious, like ChromeAppDelegate-- app windows could presumably have unload handlers, but I don't know if they end up on this list. If there is overlap, we'd be concerned if the existing delegate wanted to do something on cleanup which is now skipped. To be honest, it seems hard to guarantee this will remain safe as future code adds more uses of delegates, even if we happen to thread the needle and it is safe today. I wonder if it's better to to handle it in a different way? For example, if all we care about is that dialogs are suppressed and the tab gets deleted at a particular stage, maybe that's state we could set on the WebContents itself, rather than changing the delegate?
The CQ bit was checked by zmin@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...
On 2017/03/14 23:21:44, Charlie Reis (slow) wrote: > https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... > File chrome/browser/ui/unload_controller.cc (right): > > https://codereview.chromium.org/2681203002/diff/140001/chrome/browser/ui/unlo... > chrome/browser/ui/unload_controller.cc:320: > (*it)->SetDelegate(web_contents_delegate_.get()); > On 2017/03/14 20:26:34, sky wrote: > > On 2017/03/14 17:46:47, zmin wrote: > > > On 2017/03/14 00:04:05, sky wrote: > > > > How do you know it's safe to reset the delegate? Might there already be a > > > > delegate that wants to track the WebContents? > > > > > > This delegate makes sure that after windows are closed, there is no opened > > > dialog left behind. I think it's safe because FastUnloadController has been > > > using it. > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/fast_unload_controller... > > > > AFAIK FastUnloadController isn't shipping yet, so I'm concerned. Charlie, does > > this look problematic to you too? > > That's a great question. It looks like it depends on what > tabs_needing_unload_fired_ can contain, and whether it overlaps with any of the > (many) users of WebContents::SetDelegate. It might be worth checking with > whomever works on FastUnloadController to see if they've reasoned about why it's > safe. > > At first glance, there's a ton of features that create a WebContents and call > SetDelegate on it, many of which are under our control and might be unlikely to > have unload handlers (e.g., ChromeOS first run, login, etc). Some seem less > obvious, like ChromeAppDelegate-- app windows could presumably have unload > handlers, but I don't know if they end up on this list. > > If there is overlap, we'd be concerned if the existing delegate wanted to do > something on cleanup which is now skipped. > > To be honest, it seems hard to guarantee this will remain safe as future code > adds more uses of delegates, even if we happen to thread the needle and it is > safe today. I wonder if it's better to to handle it in a different way? > > For example, if all we care about is that dialogs are suppressed and the tab > gets deleted at a particular stage, maybe that's state we could set on the > WebContents itself, rather than changing the delegate? The SetDelegate was introduced in FastUnloadController since its first version https://chromium.googlesource.com/chromium/src/+/88c9201abfce799772a6d22ad283... Based on the CL's description, it might come from here in UnloadController first: https://src.chromium.org/viewvc/chrome?revision=195108&view=revision Unfortunately, the authors of both CLs are not in Google anymore. But, I agree that even the SetDelegate is safe now, it could still break something else in the future. On the other hand, I added this delegate into UnloadController in order to avoid hanging ongoing beforeunload dialog after force window closing. However, I found that this is not a problem anymore. There is no hanging dialog after closing even the delegate is not set. It passed both manually testing and the browser test I wrote for it. I don't know what fix this, but I guess it's safe to remove that delegate from UnloadController now. I didn't touch the delegate in FastUnloadController in case there is any other purpose. I can create an issue to track this. Is there anyone who still owns the FastUnloadController since it's not launched?
LGTM - not sure if anyone owns fastunloadcontroller
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by zmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org Link to the patchset: https://codereview.chromium.org/2681203002/#ps180001 (title: "cr - remove delegate in UnloadController")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1489611484293520,
"parent_rev": "e1cb39932458b42a69aa5a3c9b353ad383b78a62", "commit_rev":
"9ea988f681de3e681c720e6046ceea96a0659783"}
Message was sent while issue was closed.
Description was changed from ========== Add Browser::SkipCallBeforeUnload so that the browser windows can be closed regardless of beforeunload events. This function will be used when force-sign-in policy is enabled. When the auth token become invalid, this policy will close all browser windows, sign out user and display UserManager. Implement this function so that the window closing process won't be blocked by the webpage contains onbeforeunload event. BUG=642059 ========== to ========== Add Browser::SkipCallBeforeUnload so that the browser windows can be closed regardless of beforeunload events. This function will be used when force-sign-in policy is enabled. When the auth token become invalid, this policy will close all browser windows, sign out user and display UserManager. Implement this function so that the window closing process won't be blocked by the webpage contains onbeforeunload event. BUG=642059 Review-Url: https://codereview.chromium.org/2681203002 Cr-Commit-Position: refs/heads/master@{#457206} Committed: https://chromium.googlesource.com/chromium/src/+/9ea988f681de3e681c720e6046ce... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:180001) as https://chromium.googlesource.com/chromium/src/+/9ea988f681de3e681c720e6046ce... |
