Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2161753002:
MD Settings: Bluetooth dialog, remove unnecessary 'close-dialog' event. (Closed)
|
Created:
4 years, 5 months ago by dpapad Modified:
4 years, 5 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, Dan Beam, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Bluetooth dialog, remove unnecessary 'close-dialog' event.
The interaction of blueetooth-device-dialog with its parent (bluetooth-page)
was more complex than necessary. Specifically summarizing the complexity
below
- The dialog was firing a "close-dialog" event. In some of those occasions
the dialog had already been closed by the time this event was fired (see
BluetoothDeviceDialog#onIronOverlayClosed_).
- Parent was catching the "close-dialog" event and was closing the
dialog with the close() method (sometimes unnecessarily, since the dialog
was already closed).
- Parent was also additionally listening to the 'iron-overlay-closed'
event.
It seems unnecessary to have the parent listen to two different events, one
to trigger the dialog closing, and one that fires after the dialog is closed.
Instead, the dialog can always close itself directly, and the parent can only
listen to the 'iron-overlay-closed' event.
This simplification is part of the effort to migrate <cr-dialog> to use
native <dialog>. Native <dialog> throws an error if close() is called while
it is already closed.
BUG=625332
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/f4af3d1625c8a80eface78c51d3a2811124f56d8
Cr-Commit-Position: refs/heads/master@{#406339}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Nit #
Messages
Total messages: 27 (18 generated)
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. BUG= ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by dpapad@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...
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - In a few occasions the dialog was firing a "close-dialog" event. - Parent was catching the "close-dialog" event and closed the dialog using the close() method. - Parent was also additionally listening to the 'iron-overlay-closed' event. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - In a few occasions the dialog was firing a "close-dialog" event. - Parent was catching the "close-dialog" event and closed the dialog using the close() method. - Parent was also additionally listening to the 'iron-overlay-closed' event. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - In a few occasions the dialog was firing a "close-dialog" event. - Parent was catching the "close-dialog" event and was closing the dialog in the onCloseDialog_ handler using the close() method. - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate cr-dialog to use native <dialog>. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - In a few occasions the dialog was firing a "close-dialog" event. - Parent was catching the "close-dialog" event and was closing the dialog in the onCloseDialog_ handler using the close() method. - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate cr-dialog to use native <dialog>. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - In a few occasions the dialog was firing a "close-dialog" event. - Parent was catching the "close-dialog" event and was closing the dialog in the onCloseDialog_ handler using the close() method. - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate cr-dialog to use native <dialog>. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - In a few occasions the dialog was firing a "close-dialog" event. - Parent was catching the "close-dialog" event and was closing the dialog in the onCloseDialog_ handler using the close() method. - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate cr-dialog to use native <dialog>. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - In a few occasions the dialog was firing a "close-dialog" event. - Parent was catching the "close-dialog" event and was closing the dialog in the onCloseDialog_ handler using the close() method. - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate <cr-dialog> to use native <dialog>. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - In a few occasions the dialog was firing a "close-dialog" event. - Parent was catching the "close-dialog" event and was closing the dialog in the onCloseDialog_ handler using the close() method. - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate <cr-dialog> to use native <dialog>. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - The dialog was firing a "close-dialog" event. In some of those occasions the dialog had already been closed by the time this event was fired (see BluetoothDeviceDialog#onIronOverlayClosed_). - Parent was catching the "close-dialog" event and was closing the dialog with the close() method (sometimes unnecessarily, since the dialog was already closed). - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate <cr-dialog> to use native <dialog>. Native <dialog> throws an error if close() is called while it is already closed. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + stevenjb@chromium.org
@dbeam: Assigned @stevenjb as a reviewer, but feel free to review. I recall that you also worked on this dialog briefly.
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary close-dialog event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - The dialog was firing a "close-dialog" event. In some of those occasions the dialog had already been closed by the time this event was fired (see BluetoothDeviceDialog#onIronOverlayClosed_). - Parent was catching the "close-dialog" event and was closing the dialog with the close() method (sometimes unnecessarily, since the dialog was already closed). - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate <cr-dialog> to use native <dialog>. Native <dialog> throws an error if close() is called while it is already closed. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary 'close-dialog' event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - The dialog was firing a "close-dialog" event. In some of those occasions the dialog had already been closed by the time this event was fired (see BluetoothDeviceDialog#onIronOverlayClosed_). - Parent was catching the "close-dialog" event and was closing the dialog with the close() method (sometimes unnecessarily, since the dialog was already closed). - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate <cr-dialog> to use native <dialog>. Native <dialog> throws an error if close() is called while it is already closed. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary 'close-dialog' event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - The dialog was firing a "close-dialog" event. In some of those occasions the dialog had already been closed by the time this event was fired (see BluetoothDeviceDialog#onIronOverlayClosed_). - Parent was catching the "close-dialog" event and was closing the dialog with the close() method (sometimes unnecessarily, since the dialog was already closed). - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead the dialog can always close itself, and the parent can only listen to the iron-overlay-closed event. This simplification is part of the effort to migrate <cr-dialog> to use native <dialog>. Native <dialog> throws an error if close() is called while it is already closed. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary 'close-dialog' event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - The dialog was firing a "close-dialog" event. In some of those occasions the dialog had already been closed by the time this event was fired (see BluetoothDeviceDialog#onIronOverlayClosed_). - Parent was catching the "close-dialog" event and was closing the dialog with the close() method (sometimes unnecessarily, since the dialog was already closed). - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead, the dialog can always close itself directly, and the parent can only listen to the 'iron-overlay-closed' event. This simplification is part of the effort to migrate <cr-dialog> to use native <dialog>. Native <dialog> throws an error if close() is called while it is already closed. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/18 at 20:39:15, dpapad wrote: > @dbeam: Assigned @stevenjb as a reviewer, but feel free to review. I recall that you > also worked on this dialog briefly. Ping.
Sorry, I thought dan was reviewing this. I -think- that the complexity was related to the previous implementation where we had multiple dialogs, but now this -should- be fine. I'll test it once it lands. lgtm https://codereview.chromium.org/2161753002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right): https://codereview.chromium.org/2161753002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:535: return; no need for the early exit any more.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
lgtm
Thanks. https://codereview.chromium.org/2161753002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js (right): https://codereview.chromium.org/2161753002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/bluetooth_page/bluetooth_page.js:535: return; On 2016/07/19 at 17:21:54, stevenjb wrote: > no need for the early exit any more. Done.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2161753002/#ps20001 (title: "Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Bluetooth dialog, remove unnecessary 'close-dialog' event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - The dialog was firing a "close-dialog" event. In some of those occasions the dialog had already been closed by the time this event was fired (see BluetoothDeviceDialog#onIronOverlayClosed_). - Parent was catching the "close-dialog" event and was closing the dialog with the close() method (sometimes unnecessarily, since the dialog was already closed). - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead, the dialog can always close itself directly, and the parent can only listen to the 'iron-overlay-closed' event. This simplification is part of the effort to migrate <cr-dialog> to use native <dialog>. Native <dialog> throws an error if close() is called while it is already closed. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Bluetooth dialog, remove unnecessary 'close-dialog' event. The interaction of blueetooth-device-dialog with its parent (bluetooth-page) was more complex than necessary. Specifically summarizing the complexity below - The dialog was firing a "close-dialog" event. In some of those occasions the dialog had already been closed by the time this event was fired (see BluetoothDeviceDialog#onIronOverlayClosed_). - Parent was catching the "close-dialog" event and was closing the dialog with the close() method (sometimes unnecessarily, since the dialog was already closed). - Parent was also additionally listening to the 'iron-overlay-closed' event. It seems unnecessary to have the parent listen to two different events, one to trigger the dialog closing, and one that fires after the dialog is closed. Instead, the dialog can always close itself directly, and the parent can only listen to the 'iron-overlay-closed' event. This simplification is part of the effort to migrate <cr-dialog> to use native <dialog>. Native <dialog> throws an error if close() is called while it is already closed. BUG=625332 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/f4af3d1625c8a80eface78c51d3a2811124f56d8 Cr-Commit-Position: refs/heads/master@{#406339} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/f4af3d1625c8a80eface78c51d3a2811124f56d8 Cr-Commit-Position: refs/heads/master@{#406339} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
