Don't show FileManagerDialog if another FileManagerDialog is shown in the same tab.

Don't show FileManagerDialog if another FileManagerDialog is shown in the same tab.

This situation is unlikely to happen, unless we really try, but it is possible.
The result: dialog that was opened first cannot be closed, and the machine becomes unusable.

TEST=FileManagerDialogBrowserTest.*
BUG=chromium-os:22669
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109584

[tbarzic, 2011-11-10 20:51:30]

Hey James,
could you take a look at this

Toni

[achuithb, 2011-11-10 20:58:40]

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog.cc (right):

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog.cc:209: int32 tab_id = (tab ?
tab->restore_tab_helper()->session_id().id() : 0);
parens are not required.

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog.h (right):

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog.h:61:
FRIEND_TEST_ALL_PREFIXES(FileManagerDialogBrowserTest, OpenTwoDialogs);
You could add a protected method like OpenSecondDialog to
FileManagerDialogBrowserTest to avoid this friending if you like...

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog.h:77: bool PendingExists(int32
tab_id);
static?

[James Cook, 2011-11-10 21:04:54]

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog.h (right):

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog.h:77: bool PendingExists(int32
tab_id);
Maybe call this IsPending()?  const?

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog_browsertest.cc (right):

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:93: void
OpenDialog(SelectFileDialog::Type dialog_type,
Nice that you extracted this shared code!

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:131: void
CloseDialog(bool press_ok, const gfx::NativeWindow& owning_window) {
Maybe use an enum or a string for the button to press?  This would make the
calling code self-documenting, like: CloseDialog(window, "ok") or
CloseDialog(window, BTN_OK).

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:278:
OpenDialog(SelectFileDialog::SELECT_OPEN_FILE, FilePath(), owning_window, "");
Yeah, these are much easier to read with the common code extracted.  Good work.

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:309:
scoped_refptr<FileManagerDialog>  dialog2 =
Optional: dialog2 could become a member variable of this test. 
FileManagerDialog objects are cheap to construct, so I think it would be OK to
make two of them for every test run.  That would eliminate the extra friend
declaration.  Up to you; I don't feel strongly about adding friends for testing.

[tbarzic, 2011-11-10 22:00:47]

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog.cc (right):

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog.cc:209: int32 tab_id = (tab ?
tab->restore_tab_helper()->session_id().id() : 0);
On 2011/11/10 20:58:40, achuith.bhandarkar wrote:
> parens are not required.
removed

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog.h (right):

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog.h:61:
FRIEND_TEST_ALL_PREFIXES(FileManagerDialogBrowserTest, OpenTwoDialogs);
On 2011/11/10 20:58:40, achuith.bhandarkar wrote:
> You could add a protected method like OpenSecondDialog to
> FileManagerDialogBrowserTest to avoid this friending if you like...

Done.

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog.h:77: bool PendingExists(int32
tab_id);
On 2011/11/10 20:58:40, achuith.bhandarkar wrote:
> static?

Yep

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog.h:77: bool PendingExists(int32
tab_id);
On 2011/11/10 21:04:54, James Cook (Chromium) wrote:
> Maybe call this IsPending()?  const?

Hm, I don't feel IsPending would be the right name (this name would be more
suitable if the method checked if this dialog is pending)
Since we are checking if there already is pending dialog in the tab,
PendingExists seems better (at least that is my opinion)..

As for const, I'd rather make it static

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog_browsertest.cc (right):

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:131: void
CloseDialog(bool press_ok, const gfx::NativeWindow& owning_window) {
On 2011/11/10 21:04:54, James Cook (Chromium) wrote:
> Maybe use an enum or a string for the button to press?  This would make the
> calling code self-documenting, like: CloseDialog(window, "ok") or
> CloseDialog(window, BTN_OK).

Done.

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:278:
OpenDialog(SelectFileDialog::SELECT_OPEN_FILE, FilePath(), owning_window, "");
On 2011/11/10 21:04:54, James Cook (Chromium) wrote:
> Yeah, these are much easier to read with the common code extracted.  Good
work.

:)

http://codereview.chromium.org/8523006/diff/3001/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:309:
scoped_refptr<FileManagerDialog>  dialog2 =
On 2011/11/10 21:04:54, James Cook (Chromium) wrote:
> Optional: dialog2 could become a member variable of this test. 
> FileManagerDialog objects are cheap to construct, so I think it would be OK to
> make two of them for every test run.  That would eliminate the extra friend
> declaration.  Up to you; I don't feel strongly about adding friends for
testing.

Done.

[James Cook, 2011-11-10 23:17:18]

LGTM with one optional nit

http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog_browsertest.cc (right):

http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:145: //  some dialog
type.
nit: remove extra space at start of comment

[achuithb, 2011-11-10 23:30:03]

LGTM

http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog_browsertest.cc (right):

http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:118: string16() /*
title */,
Do you like this commenting style? Have you considered:
string16(),  // title
file_path,
NULL,  // file_types
0,  // file_type_index
etc

http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:127: LOG(INFO) <<
"Waiting for JavaScript ready message.";
Is this logging necessary?

[James Cook, 2011-11-10 23:36:42]

>
http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
> chrome/browser/ui/views/file_manager_dialog_browsertest.cc:127: LOG(INFO) <<
> "Waiting for JavaScript ready message.";
> Is this logging necessary?

I would keep this logging.  A common failure mode of the file manager JavaScript
is for it to fail to initialize properly.  Printing this line makes it clear
when debugging that the test failed because it didn't get a ready message. 
Otherwise you just see the test timeout and you don't know where it happened.

[tbarzic, 2011-11-11 03:26:24]

http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
File chrome/browser/ui/views/file_manager_dialog_browsertest.cc (right):

http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:118: string16() /*
title */,
On 2011/11/10 23:30:03, achuith.bhandarkar wrote:
> Do you like this commenting style? Have you considered:
> string16(),  // title
> file_path,
> NULL,  // file_types
> 0,  // file_type_index
> etc

Usually not, but when commenting function args I prefer this style (makes it
easier to move args between lines)

http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:127: LOG(INFO) <<
"Waiting for JavaScript ready message.";
On 2011/11/10 23:30:03, achuith.bhandarkar wrote:
> Is this logging necessary?

not necessary, but useful

http://codereview.chromium.org/8523006/diff/2011/chrome/browser/ui/views/file...
chrome/browser/ui/views/file_manager_dialog_browsertest.cc:145: //  some dialog
type.
On 2011/11/10 23:17:18, James Cook (Chromium) wrote:
> nit: remove extra space at start of comment

Done.

[commit-bot: I haz the power, 2011-11-11 03:26:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/8523006/8001

[commit-bot: I haz the power, 2011-11-11 04:25:32]

Change committed as 109584