[DevTools] Fix Dialog's close button to properly close dialog.

[DevTools] Fix Dialog's close button to properly close dialog.

Recently regressed in crrev.com/3e3774f369e1e642d06e895276b08814b279c20c.

BUG=694130
Review-Url: https://codereview.chromium.org/2706293007
Cr-Commit-Position: refs/heads/master@{#452319}
Committed: https://chromium.googlesource.com/chromium/src/+/169beb0367bc0ca03c4cb0baa18d183740f157d0

[dgozman, 2017-02-21 22:29:26]

dgozman@chromium.org changed reviewers:
+ caseq@chromium.org

[dgozman, 2017-02-21 22:29:27]

Take a look please.

[caseq, 2017-02-21 22:56:29]

https://codereview.chromium.org/2706293007/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/Dialog.js (right):

https://codereview.chromium.org/2706293007/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:80:
closeButton.addEventListener('click', () => this.hideDialog(), false);
So it looks like calling detach() on this one is a bad idea, right? Should we
move the logic of hideDialog into willHide() perhaps?

[dgozman, 2017-02-21 23:01:48]

https://codereview.chromium.org/2706293007/diff/1/third_party/WebKit/Source/d...
File third_party/WebKit/Source/devtools/front_end/ui/Dialog.js (right):

https://codereview.chromium.org/2706293007/diff/1/third_party/WebKit/Source/d...
third_party/WebKit/Source/devtools/front_end/ui/Dialog.js:80:
closeButton.addEventListener('click', () => this.hideDialog(), false);
On 2017/02/21 22:56:29, caseq wrote:
> So it looks like calling detach() on this one is a bad idea, right? Should we
> move the logic of hideDialog into willHide() perhaps?

I can try to do that in a follow up.

[caseq, 2017-02-21 23:13:09]

Another option for you is just to override detach(), this should be fine, too.

[dgozman, 2017-02-22 00:03:53]

On 2017/02/21 23:13:09, caseq wrote:
> Another option for you is just to override detach(), this should be fine, too.

I don't think so, because that implies overriding show() as well, which takes
different parameters.

[dgozman, 2017-02-22 02:50:41]

PTAL, new approach.

[caseq, 2017-02-22 03:15:18]

lgtm

[dgozman, 2017-02-22 04:21:00]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2017-02-22 04:22:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 06:04:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 06:04:01]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dgozman, 2017-02-22 18:59:07]

The CQ bit was checked by dgozman@chromium.org

[commit-bot: I haz the power, 2017-02-22 19:00:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 20:54:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-22 20:54:17]

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_...)

[dgozman, 2017-02-22 21:57:38]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2017-02-22 21:57:38]

The patchset sent to the CQ was uploaded after l-g-t-m from caseq@chromium.org
Link to the patchset: https://codereview.chromium.org/2706293007/#ps60001
(title: "test")

[commit-bot: I haz the power, 2017-02-22 21:58:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 00:56:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487800658343300,
"parent_rev": "6f057b93cf230adadd55af0490c836d91ba6e1fd", "commit_rev":
"169beb0367bc0ca03c4cb0baa18d183740f157d0"}

[commit-bot: I haz the power, 2017-02-23 00:57:14]

Description was changed from

==========
[DevTools] Fix Dialog's close button to properly close dialog.

Recently regressed in crrev.com/3e3774f369e1e642d06e895276b08814b279c20c.

BUG=694130
==========

to

==========
[DevTools] Fix Dialog's close button to properly close dialog.

Recently regressed in crrev.com/3e3774f369e1e642d06e895276b08814b279c20c.

BUG=694130

Review-Url: https://codereview.chromium.org/2706293007
Cr-Commit-Position: refs/heads/master@{#452319}
Committed:
https://chromium.googlesource.com/chromium/src/+/169beb0367bc0ca03c4cb0baa18d...
==========

[commit-bot: I haz the power, 2017-02-23 00:57:15]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/169beb0367bc0ca03c4cb0baa18d...