Don't display the file select dialog from invisible windows....

Don't display the file select dialog from invisible windows.
See more details from http://code.google.com/p/chromium/issues/detail?id=58319.

BUG=58319
TEST=none


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

[brettw, 2010-10-15 22:15:10]

http://codereview.chromium.org/3832005/diff/5001/6002
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/3832005/diff/5001/6002#newcode1420
chrome/browser/renderer_host/render_view_host.cc:1420: if
(!platform_util::IsVisible(view()->GetNativeView())) {
We shouldn't having this view-related code here. At the very least, there should
be an IsVisible function on the view. But I wonder, is this really necessary? It
sounds like a bug if blocked popups aren't marked hidden, and does it really
matter if there's a race if the dialog was just hidden?

[abarth-chromium, 2010-10-17 10:46:57]

I've looked at this CL, but I don't have anything intelligent to say about it.
As we discussed, I think we should do this, but I'm not familiar enough with
this code to know whether or not this is the right implementation strategy.

[Johnny(Jianning) Ding, 2010-10-18 22:16:22]

http://codereview.chromium.org/3832005/diff/5001/6002
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/3832005/diff/5001/6002#newcode1420
chrome/browser/renderer_host/render_view_host.cc:1420: if
(!platform_util::IsVisible(view()->GetNativeView())) {
Now it is OK to ignore this race condition since we explicitly require a user
gesture to open the file select dialog.
But other features whose logics need to check the visibility status of render
views maybe need to be aware this race.
 
On 2010/10/15 22:15:11, brettw wrote:
> We shouldn't having this view-related code here. At the very least, there
should
> be an IsVisible function on the view. But I wonder, is this really necessary?
It
> sounds like a bug if blocked popups aren't marked hidden, and does it really
> matter if there's a race if the dialog was just hidden?
>

[brettw, 2010-10-20 05:09:21]

LGTM, thanks for the smaller patch.