Fix for cant reopen file dialog after close it by x button or alt+f4

The root of problem is x button or alt+f4 didn't close the window completely. There are still something in the pendingdialog list need to be cleaned. 

BUG=105311
TEST=settings->under the hood->Manage certificates->import; close the dialog
by x button or alt+f4; click import button again; verify if the dialog shows
up again.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111953

[bshe, 2011-11-25 01:15:10]

This is a quick work around for the issue. The most correct way to do it is to
override the x button&alt+f4 function.

But as I mentioned in the comments in issue 105311. This change is probably what
we want anyway(delete the pendingdialog list if it exists instead just return).
So I uploaded this patch. I will look into the override way as well.

[Rick Byers, 2011-11-25 15:06:06]

http://codereview.chromium.org/8692006/diff/1001/chrome/browser/ui/views/sele...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

http://codereview.chromium.org/8692006/diff/1001/chrome/browser/ui/views/sele...
chrome/browser/ui/views/select_file_dialog_extension.cc:215:
PendingDialog::GetInstance()->Remove(tab_id);
I haven't looked at the code in detail, but doesn't this just circumvent what
PendingDialog was trying to do (make sure you never have two dialogs up at
once)?

[flackr, 2011-11-25 15:18:19]

On 2011/11/25 15:06:06, Rick Byers wrote:
>
http://codereview.chromium.org/8692006/diff/1001/chrome/browser/ui/views/sele...
> File chrome/browser/ui/views/select_file_dialog_extension.cc (right):
> 
>
http://codereview.chromium.org/8692006/diff/1001/chrome/browser/ui/views/sele...
> chrome/browser/ui/views/select_file_dialog_extension.cc:215:
> PendingDialog::GetInstance()->Remove(tab_id);
> I haven't looked at the code in detail, but doesn't this just circumvent what
> PendingDialog was trying to do (make sure you never have two dialogs up at
> once)?

Yes, the problem would seem to be that the cleanup on closing the dialog using
the X isn't done. Try to find out how to handle the dialog closing. My guess is
that cancelling and closing the dialog should have the same behaviour.

[bshe, 2011-11-25 17:58:49]

http://codereview.chromium.org/8692006/diff/1001/chrome/browser/ui/views/sele...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

http://codereview.chromium.org/8692006/diff/1001/chrome/browser/ui/views/sele...
chrome/browser/ui/views/select_file_dialog_extension.cc:215:
PendingDialog::GetInstance()->Remove(tab_id);
I am not sure when and how a user attempts to open two dialogs in the same tab.
I was think maybe we can give user some sort of feedback (bring the previous one
up or delete previous one and create a new one or a warning dialog) when they
try to do so instead of just ignore. But I guess it depends on the specific
situation. Maybe ignore its exactly what we want. 

Also giving some sort of feedback may require some works, if the situation is
rare, it might not worth to spend time on it at least for now.

Anyway, I am working on making the close button behaves the same as cancel
button.
 

On 2011/11/25 15:06:06, Rick Byers wrote:
> I haven't looked at the code in detail, but doesn't this just circumvent what
> PendingDialog was trying to do (make sure you never have two dialogs up at
> once)?

[bshe, 2011-11-28 16:29:49]

This should fix the problem here. I have tested it on Aura and it works. But I
am not sure if its the best way. The WindowClosing calls Close function in
ExtensionDialog, which calls observer_->ExtensionDialogIsClosing(this) to clean
up the pending list.

Do you have any suggestion?

Thanks!

[flackr, 2011-11-28 16:46:54]

http://codereview.chromium.org/8692006/diff/8001/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/8692006/diff/8001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_dialog.cc:143: Close();
This doesn't seem correct to me. The window is already closing, and this might
even be called in response to a a call to Close. Instead you should probably
move the following code from ExtensionDialog::Close to here:
  if (observer_)
    observer_->ExtensionDialogIsClosing(this);

Just make sure that this method is called when the dialog is closed using
ExtensionDialog::Close.

[bshe, 2011-11-28 18:40:25]

Done.

I was thinking not change the code in Close function. But you are right. The
windowclosing function will also invoked by ExtensionDialog::Close function.
Your way is better.

http://codereview.chromium.org/8692006/diff/8001/chrome/browser/ui/views/exte...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/8692006/diff/8001/chrome/browser/ui/views/exte...
chrome/browser/ui/views/extensions/extension_dialog.cc:143: Close();
Done

On 2011/11/28 16:46:55, flackr wrote:
> This doesn't seem correct to me. The window is already closing, and this might
> even be called in response to a a call to Close. Instead you should probably
> move the following code from ExtensionDialog::Close to here:
>   if (observer_)
>     observer_->ExtensionDialogIsClosing(this);
> 
> Just make sure that this method is called when the dialog is closed using
> ExtensionDialog::Close.

[flackr, 2011-11-28 19:47:06]

http://codereview.chromium.org/8692006/diff/13001/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/8692006/diff/13001/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_dialog.cc:140: // Cleanup the
pending dialog list.
Correct me if I'm wrong, but your comment is specific to the file selector
dialog, right? I think the code is fairly self documenting, in that anything you
write wouldn't be any more specific than just reading that the observer is
notified of the window closing. So in this case remove the comment.

[bshe, 2011-11-28 22:26:50]

Done. Thanks!

http://codereview.chromium.org/8692006/diff/13001/chrome/browser/ui/views/ext...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/8692006/diff/13001/chrome/browser/ui/views/ext...
chrome/browser/ui/views/extensions/extension_dialog.cc:140: // Cleanup the
pending dialog list.
Done.
It is quite self documenting, so I will just remove it.

On 2011/11/28 19:47:06, flackr wrote:
> Correct me if I'm wrong, but your comment is specific to the file selector
> dialog, right? I think the code is fairly self documenting, in that anything
you
> write wouldn't be any more specific than just reading that the observer is
> notified of the window closing. So in this case remove the comment.

[flackr, 2011-11-29 01:55:59]

LGTM

[commit-bot: I haz the power, 2011-11-29 03:00:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/8692006/14004

[commit-bot: I haz the power, 2011-11-29 03:33:03]

Try job failure for 8692006-14004 (retry) on linux_clang for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

[commit-bot: I haz the power, 2011-11-29 14:51:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bshe@chromium.org/8692006/14004

[commit-bot: I haz the power, 2011-11-29 16:29:16]

Change committed as 111953