Allow ChromeOS file selection dialog to be shown from shell windows.

Allow ChromeOS file selection dialog to be shown from shell windows.

This enables the fileSystem APIs to work on ChromeOS. With this change
the dialogs are shown full screen when invoked from shell windows.

BUG=133594
TEST=Test that file selection dialog can be used successfully on
ChromeOS from a range of uses, including loading an extension and
saving a file you have downloaded. Check that the dialog looks the
same as it always has.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=144021

[benwells, 2012-06-20 05:03:09]

This changes fixes the problem but doesn't feel great.

A better solution would be to have something like a TopLevelWindow interface,
implemented by both ShellWindow and BrowserWindow (or Browser), and a
TopLevelWindowList like BrowserList. Something like this would also be useful
for things, e.g. shutdown.

I'm hoping to get this in in the meantime as it lets the fileSystem APIs work on
ChromeOS, and isn't too bad.

[Mihai Parparita -not on Chrome, 2012-06-20 05:21:06]

http://codereview.chromium.org/10584026/diff/2001/chrome/browser/ui/views/sel...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

http://codereview.chromium.org/10584026/diff/2001/chrome/browser/ui/views/sel...
chrome/browser/ui/views/select_file_dialog_extension.cc:259: // Extension
background pages may not supply an owner_window, in which
In cases where the the file picker is triggered from a platform app's background
page and there are no browser windows open, will you end up hitting the
NOTREACHED on line 297?

http://codereview.chromium.org/10584026/diff/2001/chrome/browser/ui/views/sel...
chrome/browser/ui/views/select_file_dialog_extension.cc:264: if (owner_browser)
{
In the scenario where you have a shell window and a browser open, it seems like
the dialog will end up being attached to the browser window, even if the shell
window is the one that triggered this. It seems like you should always do the
GetShellWindowForNativeWindow(owner_window) check.

[benwells, 2012-06-20 05:41:32]

http://codereview.chromium.org/10584026/diff/2001/chrome/browser/ui/views/sel...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

http://codereview.chromium.org/10584026/diff/2001/chrome/browser/ui/views/sel...
chrome/browser/ui/views/select_file_dialog_extension.cc:259: // Extension
background pages may not supply an owner_window, in which
On 2012/06/20 05:21:06, Mihai Parparita wrote:
> In cases where the the file picker is triggered from a platform app's
background
> page and there are no browser windows open, will you end up hitting the
> NOTREACHED on line 297?

No, as that will fail in the file system API code which already checks that
there is a ShellWindow associated with the calling RenderViewHost. (See
FileSystemPickerFunction::ShowPicker in file_system_api.cc, conventiently also
modified in this change).

http://codereview.chromium.org/10584026/diff/2001/chrome/browser/ui/views/sel...
chrome/browser/ui/views/select_file_dialog_extension.cc:264: if (owner_browser)
{
On 2012/06/20 05:21:06, Mihai Parparita wrote:
> In the scenario where you have a shell window and a browser open, it seems
like
> the dialog will end up being attached to the browser window, even if the shell
> window is the one that triggered this. It seems like you should always do the
> GetShellWindowForNativeWindow(owner_window) check.

As owner_window will be set it will call FindBrowserWithWindow, which will
return NULL, so I think it will work.

The logic was pretty convoluted though. Hopefully it's clearer now.

[sky, 2012-06-20 14:36:09]

I agree on this being ick. I would rather see us do the right long term thing.

[jeremya, 2012-06-20 15:32:34]

It looks to me like browser/ui/views/extension_dialog.cc only needs Browser to
get a Profile and a window to use as a reference for position. Perhaps it could
be refactored to take references to those directly rather than pulling them out
of a Browser?

ExtensionDialog could theoretically take a views::Widget* reference, which we
can extract equally well from a browser or a shell window using
views::Widget::GetWidgetForNativeWindow().

Not sure if that's the best way to do things, but it would (I think) eliminate
the need to search through all the shell windows. ExtensionDialog seems to be
agnostic about what kind of window it actually gets put in.

[benwells, 2012-06-22 16:42:46]

Updated to be less icky. It still has the horrible loop through profiles. I'm
still thinking about the best way to get rid of that loop - either by making the
registry not a profile keyed service, or by introducing a new list of all base
windows, or something else...

[stevenjb (google-dont-use), 2012-06-22 17:29:00]

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:64: TabContents* tab_contents()
const { return contents_.get(); }
The UI team prefers a pattern where this should either return a const
TabContents* (if possible), or not be a const member.

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/views/se...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/views/se...
chrome/browser/ui/views/select_file_dialog_extension.cc:279: iter <
profiles.end(); iter++) {
nit: alignment

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/views/se...
chrome/browser/ui/views/select_file_dialog_extension.cc:291: }
I think what you really want to use here is ExtensionWindowList
(chrome/browser/extensions/extension_window_list.h). That contains a list of
ExtensionWindowController instances, which should include all Browser and
ShellWindow instances (and eventually Panels also).

ExtensionWindowController contains both a Profile* and a BaseWindow*. BaseWindow
already has a GetNativeWindow() method, so you could replace
ShellWindowRegistry::GetShellWindowForNativeWindow with
ExtensionWindowList::GetWindowForNativeWindow(owner_window) and get BaseWindow
and Profile from there. You might need to add GetActiveTabContents to
BaseWindow, which seems fine.

[benwells, 2012-06-22 19:51:47]

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/views/se...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/views/se...
chrome/browser/ui/views/select_file_dialog_extension.cc:291: }
On 2012/06/22 17:29:00, stevenjb1 wrote:
> I think what you really want to use here is ExtensionWindowList
> (chrome/browser/extensions/extension_window_list.h). That contains a list of
> ExtensionWindowController instances, which should include all Browser and
> ShellWindow instances (and eventually Panels also).
> 
> ExtensionWindowController contains both a Profile* and a BaseWindow*.
BaseWindow
> already has a GetNativeWindow() method, so you could replace
> ShellWindowRegistry::GetShellWindowForNativeWindow with
> ExtensionWindowList::GetWindowForNativeWindow(owner_window) and get BaseWindow
> and Profile from there. You might need to add GetActiveTabContents to
> BaseWindow, which seems fine.

This looks like a great plan, and I had most of the code written when I realised
there is no ExtensionWindowController for ShellWindows. This was removed as
shell window's aren't controllable via the chrome.windows API.

Given that ExtensionWindowList is really a list of windows which are
controllable via chrome.windows this no longer feels like a good way forward.

[stevenjb (google-dont-use), 2012-06-22 21:17:10]

Interesting. I knew that we decided not to use the chrome.windows API, but
didn't realize they weren't in ExtensionWindowController.

Anyway, this is probably fine as-is.
On Jun 22, 2012 12:51 PM, <benwells@chromium.org> wrote:

>
> http://codereview.chromium.**org/10584026/diff/12001/**
>
chrome/browser/ui/views/**select_file_dialog_extension.**cc<http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/views/select_file_dialog_extension.cc>
> File chrome/browser/ui/views/**select_file_dialog_extension.**cc (right):
>
> http://codereview.chromium.**org/10584026/diff/12001/**
>
chrome/browser/ui/views/**select_file_dialog_extension.**cc#newcode291<http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/views/select_file_dialog_extension.cc#newcode291>
> chrome/browser/ui/views/**select_file_dialog_extension.**cc:291: }
> On 2012/06/22 17:29:00, stevenjb1 wrote:
>
>> I think what you really want to use here is ExtensionWindowList
>> (chrome/browser/extensions/**extension_window_list.h). That contains a
>>
> list of
>
>> ExtensionWindowController instances, which should include all Browser
>>
> and
>
>> ShellWindow instances (and eventually Panels also).
>>
>
>  ExtensionWindowController contains both a Profile* and a BaseWindow*.
>>
> BaseWindow
>
>> already has a GetNativeWindow() method, so you could replace
>> ShellWindowRegistry::**GetShellWindowForNativeWindow with
>> ExtensionWindowList::**GetWindowForNativeWindow(**owner_window) and get
>>
> BaseWindow
>
>> and Profile from there. You might need to add GetActiveTabContents to
>> BaseWindow, which seems fine.
>>
>
> This looks like a great plan, and I had most of the code written when I
> realised there is no ExtensionWindowController for ShellWindows. This
> was removed as shell window's aren't controllable via the chrome.windows
> API.
>
> Given that ExtensionWindowList is really a list of windows which are
> controllable via chrome.windows this no longer feels like a good way
> forward.
>
>
http://codereview.chromium.**org/10584026/<http://codereview.chromium.org/105...
>

[benwells, 2012-06-22 21:54:28]

PTAL

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.h (right):

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.h:64: TabContents* tab_contents()
const { return contents_.get(); }
On 2012/06/22 17:29:00, stevenjb1 wrote:
> The UI team prefers a pattern where this should either return a const
> TabContents* (if possible), or not be a const member.

Done.

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/views/se...
File chrome/browser/ui/views/select_file_dialog_extension.cc (right):

http://codereview.chromium.org/10584026/diff/12001/chrome/browser/ui/views/se...
chrome/browser/ui/views/select_file_dialog_extension.cc:279: iter <
profiles.end(); iter++) {
On 2012/06/22 17:29:00, stevenjb1 wrote:
> nit: alignment

Done.

[sky, 2012-06-22 22:28:12]

http://codereview.chromium.org/10584026/diff/20001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10584026/diff/20001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:164: if (y < 0)
some screens have negative coordinates.

[benwells, 2012-06-22 23:05:03]

http://codereview.chromium.org/10584026/diff/20001/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10584026/diff/20001/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:164: if (y < 0)
On 2012/06/22 22:28:12, sky wrote:
> some screens have negative coordinates.

Done.

[sky, 2012-06-25 14:23:14]

LGTM

http://codereview.chromium.org/10584026/diff/24002/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10584026/diff/24002/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:166: if (y <
screen_rect.y())
Replace all this with AdjustToFit

[benwells, 2012-06-25 17:52:59]

http://codereview.chromium.org/10584026/diff/24002/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/extension_dialog.cc (right):

http://codereview.chromium.org/10584026/diff/24002/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/extension_dialog.cc:166: if (y <
screen_rect.y())
On 2012/06/25 14:23:15, sky wrote:
> Replace all this with AdjustToFit

Done.

[commit-bot: I haz the power, 2012-06-25 18:00:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10584026/24003

[commit-bot: I haz the power, 2012-06-25 18:22:19]

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

[commit-bot: I haz the power, 2012-06-25 18:59:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/10584026/24003

[commit-bot: I haz the power, 2012-06-25 20:58:29]

Failed to apply patch for chrome/browser/ui/views/extensions/extension_dialog.h:
While running patch -p1 --forward --force;
patching file chrome/browser/ui/views/extensions/extension_dialog.h
Hunk #3 FAILED at 31.
Hunk #4 succeeded at 100 (offset 3 lines).
Hunk #5 succeeded at 109 (offset 3 lines).
1 out of 5 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/extensions/extension_dialog.h.rej