Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2936643003:
Allow cancelling the currently-presented dialog. (Closed)
|
Created:
3 years, 6 months ago by kkhorimoto Modified:
3 years, 6 months ago CC:
chromium-reviews, marq+watch_chromium.org, ios-reviews+chrome_chromium.org, noyau+watch_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow cancelling the currently-presented dialog.
DialogPresenter's |-cancelDialogForWebState:| had a DCHECK ensuring that
the WebState whose dialogs are being cancelled is not the one currently
presenting a dialog. This CL removes that DCHECK and correctly handles
cancelling the presented dialog. Closing the Tab presenting the current
dialog is impossible through the UI, so this DCHECK was never hit in
practice, but it's technically possible for the current Tab to be closed
by window.close or asynchronous JavaScript execution while the dialog is
presented.
BUG=none
Review-Url: https://codereview.chromium.org/2936643003
Cr-Commit-Position: refs/heads/master@{#481259}
Committed: https://chromium.googlesource.com/chromium/src/+/709a3cbb9e85dd0c4572ee30a32bda68140fbb52
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added DCHECK, renamed selector #Patch Set 3 : fix test #Patch Set 4 : declare selector #Patch Set 5 : fix compilation #
Messages
Total messages: 29 (13 generated)
kkhorimoto@chromium.org changed reviewers: + rohitrao@chromium.org, sdefresne@chromium.org
https://codereview.chromium.org/2936643003/diff/1/ios/chrome/browser/ui/dialo... File ios/chrome/browser/ui/dialogs/dialog_presenter.mm (right): https://codereview.chromium.org/2936643003/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.mm:323: if (cancelingPresentedDialog) { Should we DCHECK(_dialogCoordinatorsForWebStates[webState] == NULL)? https://codereview.chromium.org/2936643003/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.mm:324: // Simulate a button tap to trigger showing the next dialog. Can a single WebState have multiple dialogs queued up? Is it possible for this line to show *another* dialog for the same |webState|? https://codereview.chromium.org/2936643003/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.mm:325: [self buttonWasTappedForCoordinator:dialogToCancel]; It feels a little weird to call a function with this name from here. Maybe we should pull what we need out into a helper function?
https://codereview.chromium.org/2936643003/diff/1/ios/chrome/browser/ui/dialo... File ios/chrome/browser/ui/dialogs/dialog_presenter.mm (right): https://codereview.chromium.org/2936643003/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.mm:323: if (cancelingPresentedDialog) { On 2017/06/12 23:43:50, rohitrao (ping after 24h) wrote: > Should we DCHECK(_dialogCoordinatorsForWebStates[webState] == NULL)? Done. https://codereview.chromium.org/2936643003/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.mm:324: // Simulate a button tap to trigger showing the next dialog. On 2017/06/12 23:43:50, rohitrao (ping after 24h) wrote: > Can a single WebState have multiple dialogs queued up? Is it possible for this > line to show *another* dialog for the same |webState|? This class was built on the assumption that it was not possible. JavaScript execution is halted until the dialog is dismissed and the WebKit completion handler is executed, so only one would be displayed at a time. https://codereview.chromium.org/2936643003/diff/1/ios/chrome/browser/ui/dialo... ios/chrome/browser/ui/dialogs/dialog_presenter.mm:325: [self buttonWasTappedForCoordinator:dialogToCancel]; On 2017/06/12 23:43:50, rohitrao (ping after 24h) wrote: > It feels a little weird to call a function with this name from here. Maybe we > should pull what we need out into a helper function? I renamed it to |-dialogCoordinatorWasStopped:|, which is more accurate.
lgtm
lgtm
The CQ bit was checked by kkhorimoto@chromium.org
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
fix test
The CQ bit was checked by kkhorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2936643003/#ps40001 (title: "fix test")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
declare selector
The CQ bit was checked by kkhorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2936643003/#ps60001 (title: "declare selector")
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
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
fix compilation
The CQ bit was checked by kkhorimoto@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/2936643003/#ps80001 (title: "fix compilation")
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": 80001, "attempt_start_ts": 1498069868066850,
"parent_rev": "07708943258531dcacbd85b3fc54459989ca5bbd", "commit_rev":
"709a3cbb9e85dd0c4572ee30a32bda68140fbb52"}
Message was sent while issue was closed.
Description was changed from ========== Allow cancelling the currently-presented dialog. DialogPresenter's |-cancelDialogForWebState:| had a DCHECK ensuring that the WebState whose dialogs are being cancelled is not the one currently presenting a dialog. This CL removes that DCHECK and correctly handles cancelling the presented dialog. Closing the Tab presenting the current dialog is impossible through the UI, so this DCHECK was never hit in practice, but it's technically possible for the current Tab to be closed by window.close or asynchronous JavaScript execution while the dialog is presented. BUG=none ========== to ========== Allow cancelling the currently-presented dialog. DialogPresenter's |-cancelDialogForWebState:| had a DCHECK ensuring that the WebState whose dialogs are being cancelled is not the one currently presenting a dialog. This CL removes that DCHECK and correctly handles cancelling the presented dialog. Closing the Tab presenting the current dialog is impossible through the UI, so this DCHECK was never hit in practice, but it's technically possible for the current Tab to be closed by window.close or asynchronous JavaScript execution while the dialog is presented. BUG=none Review-Url: https://codereview.chromium.org/2936643003 Cr-Commit-Position: refs/heads/master@{#481259} Committed: https://chromium.googlesource.com/chromium/src/+/709a3cbb9e85dd0c4572ee30a32b... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/709a3cbb9e85dd0c4572ee30a32b... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
