Fix DesktopMediaPickerViews to handle background parent_web_contents

chrome/browser/ui/views/desktop_media_picker_views.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/desktop_media_picker_views.h"
 
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/media/desktop_media_list.h"
 #include "chrome/browser/ui/ash/ash_util.h"
 #include "chrome/browser/ui/views/constrained_window_views.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/web_modal/popup_manager.h"
 #include "components/web_modal/web_contents_modal_dialog_manager.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents_delegate.h"
 #include "ui/aura/window_tree_host.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/events/event_constants.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 #include "ui/gfx/canvas.h"
 #include "ui/native_theme/native_theme.h"
 #include "ui/views/background.h"
 #include "ui/views/bubble/bubble_frame_view.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
@@ skipping 372 matching lines @@
   }
   label_->SetMultiLine(true);
   label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   AddChildView(label_);
 
   scroll_view_->SetContents(list_view_);
   scroll_view_->ClipHeightTo(
       GetMediaListViewHeightForRows(1), GetMediaListViewHeightForRows(2));
   AddChildView(scroll_view_);
 
-  // If |parent_web_contents| is set, the picker will be shown modal to the
-  // web contents. Otherwise, a new dialog widget inside |parent_window| will be
-  // created for the picker. Note that |parent_window| may also be NULL if
-  // parent web contents is not set. In this case the picker will be parented
-  // by a root window.
+  // If |parent_web_contents| is set and it's not a background page then the
+  // picker will be shown modal to the web contents. Otherwise the picker is
+  // shown in a separate window.
   views::Widget* widget = NULL;
-  if (parent_web_contents)
+  bool modal_dialog =

[Wez, 2014/08/23 21:51:21]

nit: Suggest has_visible_parent

[Sergey Ulanov, 2014/08/25 17:07:45]

IMO |modal_dialog| makes code below more readable.

+      parent_web_contents &&
+      !parent_web_contents->GetDelegate()->IsNeverVisible(parent_web_contents);
+  if (modal_dialog) {
     widget = CreateWebModalDialogViews(this, parent_web_contents);
-  else
-    widget = DialogDelegate::CreateDialogWidget(this, context, parent_window);
+  } else {
+    widget = DialogDelegate::CreateDialogWidget(this, context, NULL);

[Wez, 2014/08/23 21:51:20]

Why do we also want to change the no-parent_web_contents case? Or is it always
NULL if IsNeverVisible is true?

[Sergey Ulanov, 2014/08/25 17:07:46]

Currently this code is always called with parent_web_contents != NULL, so
currently this line is never executed.
When parent_web_contents is not visible we get parent_window that is an not
visible and has zero dimensions. If I pass that window it crashes here:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/view.cc&l...

Also, there is a cleanup CL for this code that I plan to land after this one:
https://codereview.chromium.org/502693003/ In that CL I'm removing parent_window
as it's not useful anymore.

+  }
 
   // DesktopMediaList needs to know the ID of the picker window which
   // matches the ID it gets from the OS. Depending on the OS and configuration
   // we get this ID differently.
   DesktopMediaID::Id dialog_window_id = 0;
 
-  // If there is |parent_window| or |parent_web_contents|, the picker window
-  // is embedded in the parent and does not have its own native window id, so we
-  // do not filter in that case.
-  if (!parent_window && !parent_web_contents) {
+  // If the picker is not modal then the picker window needs to be filtered.

[Wez, 2014/08/23 21:51:21]

nit: Clarify what it means to be filtered/why? What are we filtering?

[Sergey Ulanov, 2014/08/25 17:07:45]

Removed this comment and updated the comment above to avoid duplication.

+  if (!modal_dialog) {
 #if defined(USE_ASH)
     if (chrome::IsNativeWindowInAsh(widget->GetNativeWindow())) {
       dialog_window_id =
           DesktopMediaID::RegisterAuraWindow(widget->GetNativeWindow()).id;
       DCHECK_NE(dialog_window_id, 0);
     }
 #endif
 
     if (dialog_window_id == 0) {
       dialog_window_id = AcceleratedWidgetToDesktopMediaId(
           widget->GetNativeWindow()->GetHost()->GetAcceleratedWidget());
     }
   }
 
   list_view_->StartUpdating(dialog_window_id);
 
-  if (parent_web_contents) {
+  if (modal_dialog) {
     web_modal::PopupManager* popup_manager =
         web_modal::PopupManager::FromWebContents(parent_web_contents);
     popup_manager->ShowModalDialog(GetWidget()->GetNativeView(),
                                    parent_web_contents);
   } else {
     widget->Show();
   }
 }
 
 DesktopMediaPickerDialogView::~DesktopMediaPickerDialogView() {}
@@ skipping 137 matching lines @@
   content::BrowserThread::PostTask(
       content::BrowserThread::UI, FROM_HERE,
       base::Bind(callback_, source));
   callback_.Reset();
 }
 
 // static
 scoped_ptr<DesktopMediaPicker> DesktopMediaPicker::Create() {
   return scoped_ptr<DesktopMediaPicker>(new DesktopMediaPickerViews());
 }