Remove TabContents from media galleries.

Remove TabContents from media galleries.

BUG=107201
TEST=no visible change

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

[Avi (use Gerrit), 2012-10-14 18:26:53]

Evan: the media gallery code
Jenn: can you answer my question about shell windows?

BTW, this depends on removal of TabContents from constrained windows, which is
being reviewed at https://codereview.chromium.org/11111022/ .

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:176: }
This code made no sense. Shell windows have TabContents, so if you call
TabContents::FromWebContents on them, you will get non-NULL back. From what I
see, this if() block was NEVER true...

[jennb, 2012-10-15 16:42:32]

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:176: }
On 2012/10/14 18:26:53, Avi wrote:
> This code made no sense. Shell windows have TabContents, so if you call
> TabContents::FromWebContents on them, you will get non-NULL back. From what I
> see, this if() block was NEVER true...

ShellWindows aren't supposed to have TabContents. Ben Wells has a TODO to undo
that change.  http://crbug.com/151493

[Avi (use Gerrit), 2012-10-15 16:45:06]

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:176: }
Oh, so this code predates that.

Does this look reasonable to you, then, to drop this entire section?

[jennb, 2012-10-15 17:08:17]

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:176: }
On 2012/10/15 16:45:06, Avi wrote:
> Oh, so this code predates that.
> 
> Does this look reasonable to you, then, to drop this entire section?

Looks reasonable to me as no TabContents are needed here after this change, so
it won't matter whether ShellWindow has tabcontents or not.

[Avi (use Gerrit), 2012-10-15 17:39:48]

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:176: }
That's exactly the point ;)

[Evan Stade, 2012-10-15 21:34:06]

http://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

http://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:166: if
(!tab_contents) {
this code was here in case the API was called from the background page. You
still need to do something similar, but I don't see that happening anywhere.

[Avi (use Gerrit), 2012-10-15 22:10:00]

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:166: if
(!tab_contents) {
Question: Suppose that the background page doesn't have a TabContents. Why does
the source page need a TabContents for this to work? What about a TabContents is
needed?

[Evan Stade, 2012-10-15 22:51:33]

http://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

http://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/a...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:166: if
(!tab_contents) {
On 2012/10/15 22:10:01, Avi wrote:
> Question: Suppose that the background page doesn't have a TabContents. Why
does
> the source page need a TabContents for this to work? What about a TabContents
is
> needed?

the background page does not have a tab contents. That is why we have to find a
different one (i.e. the tab contents for the last-used shell window for the
app). This tab contents is necessary as the contents the constrained window sits
on top of/within.

[Avi (use Gerrit), 2012-10-15 23:11:13]

On 2012/10/15 22:51:33, Evan Stade wrote:
> This tab contents is necessary as the contents the constrained window sits
> on top of/within.

I removed TabContents from constrained windows (as noted in my first comment on
this CL), so now only a WebContents is needed. Please re-consider this CL in
that light.

[Evan Stade, 2012-10-16 02:57:05]

On 2012/10/15 23:11:13, Avi wrote:
> On 2012/10/15 22:51:33, Evan Stade wrote:
> > This tab contents is necessary as the contents the constrained window sits
> > on top of/within.
> 
> I removed TabContents from constrained windows (as noted in my first comment
on
> this CL), so now only a WebContents is needed. Please re-consider this CL in
> that light.

there is a web contents for the background page but it is not visible in any tab
or any shell window. So what happens when you try to use that for a constrained
window?

[Avi (use Gerrit), 2012-10-16 03:00:32]

On 2012/10/16 02:57:05, Evan Stade wrote:
> there is a web contents for the background page but it is not visible in any
tab
> or any shell window. So what happens when you try to use that for a
constrained
> window?

Very good question. Let me take a look and I'll get back to you.

I can test with a background page; I'm not sure of a good way to test with
background contents. Curses, that thing.

[Avi (use Gerrit), 2012-10-16 14:57:20]

The docs that I see (http://developer.chrome.com/trunk/apps/mediaGalleries.html)
don't have calls that show UI. Can you send me a snippet to kick up a dialog?

[Evan Stade, 2012-10-16 21:29:53]

On 2012/10/16 14:57:20, Avi wrote:
> The docs that I see
(http://developer.chrome.com/trunk/apps/mediaGalleries.html)
> don't have calls that show UI. Can you send me a snippet to kick up a dialog?

try this:

chrome.mediaGalleries.getMediaFileSystems({'interactive': 'yes'}, function()
{});

[Evan Stade, 2012-10-16 21:31:31]

On 2012/10/16 21:29:53, Evan Stade wrote:
> On 2012/10/16 14:57:20, Avi wrote:
> > The docs that I see
> (http://developer.chrome.com/trunk/apps/mediaGalleries.html)
> > don't have calls that show UI. Can you send me a snippet to kick up a
dialog?
> 
> try this:
> 
> chrome.mediaGalleries.getMediaFileSystems({'interactive': 'yes'}, function()
> {});

there's also a demo extension in my home dir under media-gallery/

[Avi (use Gerrit), 2012-10-17 22:15:42]

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:166: if
(!tab_contents) {
I have a fix, but found a related crash at the same time. If the background page
loads a constrained window on the shell window (as this code below does), and
then kicks off navigation on that window, the navigation completing cancels the
constrained window but doesn't hide it. When the user clicks a button it
crashes.

I can't commit a fix yet because I've got a dependency to fix. Working on this,
though.

[Evan Stade, 2012-10-17 22:18:59]

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:166: if
(!tab_contents) {
On 2012/10/17 22:15:42, Avi wrote:
> I have a fix, but found a related crash at the same time. If the background
page
> loads a constrained window on the shell window (as this code below does), and
> then kicks off navigation on that window, the navigation completing cancels
the
> constrained window but doesn't hide it. When the user clicks a button it
> crashes.
> 
> I can't commit a fix yet because I've got a dependency to fix. Working on
this,
> though.

might that be fixed by r162402?

[Avi (use Gerrit), 2012-10-17 22:28:48]

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:166: if
(!tab_contents) {
This appears to be different. I'm verifying with a trunk build that doesn't have
this patch :) and I'll send you a bug.

[Evan Stade, 2012-10-17 22:35:11]

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
File chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
(left):

https://codereview.chromium.org/11138010/diff/4001/chrome/browser/extensions/...
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:166: if
(!tab_contents) {
On 2012/10/17 22:28:48, Avi wrote:
> This appears to be different. I'm verifying with a trunk build that doesn't
have
> this patch :) and I'll send you a bug.

thanks.

[Avi (use Gerrit), 2012-10-18 03:00:41]

Evan, PTAL. If the background page spawns a media gallery, this new code crashes
in exactly the same way the old code does. That's
http://code.google.com/p/chromium/issues/detail?id=156537 which exists
independent of this CL.

[Evan Stade, 2012-10-18 18:52:47]

lgtm

[commit-bot: I haz the power, 2012-10-18 18:57:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11138010/18001

[commit-bot: I haz the power, 2012-10-18 18:57:45]

Presubmit check for 11138010-18001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui/views
    chrome/browser/extensions

Presubmit checks took 4.6s to calculate.

[Avi (use Gerrit), 2012-10-18 19:20:46]

Ben, can you stamp?

[Ben Goodger (Google), 2012-10-19 16:37:58]

lgtm

[commit-bot: I haz the power, 2012-10-19 16:44:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11138010/18001

[commit-bot: I haz the power, 2012-10-19 16:44:48]

Failed to apply patch for
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
  Hunk #1 FAILED at 18.
  Hunk #2 succeeded at 167 (offset 1 line).
  Hunk #3 succeeded at 187 (offset 1 line).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc.rej

Patch:      
chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
Index: chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
diff --git
a/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
b/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
index
98f89cce98f198aa247ed01abfe876b9ad21e579..9e0e43a4e18366dba2b6d4353f07013f3eb7cc33
100644
--- a/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
+++ b/chrome/browser/extensions/api/media_galleries/media_galleries_api.cc
@@ -18,8 +18,8 @@
 #include "chrome/browser/media_gallery/media_file_system_registry.h"
 #include "chrome/browser/media_gallery/media_galleries_dialog_controller.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
+#include "chrome/browser/ui/constrained_window_tab_helper.h"
 #include "chrome/browser/ui/extensions/shell_window.h"
-#include "chrome/browser/ui/tab_contents/tab_contents.h"
 #include "chrome/common/extensions/api/experimental_media_galleries.h"
 #include "chrome/common/extensions/api/media_galleries.h"
 #include "chrome/common/extensions/permissions/media_galleries_permission.h"
@@ -166,13 +166,16 @@ void
MediaGalleriesGetMediaFileSystemsFunction::ReturnGalleries(
 
 void MediaGalleriesGetMediaFileSystemsFunction::ShowDialog() {
   WebContents* contents = WebContents::FromRenderViewHost(render_view_host());
-  TabContents* tab_contents =
-      contents ? TabContents::FromWebContents(contents) : NULL;
-  if (!tab_contents) {
+  ConstrainedWindowTabHelper* constrained_window_tab_helper =
+      ConstrainedWindowTabHelper::FromWebContents(contents);
+  if (!constrained_window_tab_helper) {
+    // If there is no ConstrainedWindowTabHelper, then this contents is
probably
+    // the background page for an app. Try to find a shell window to host the
+    // dialog.
     ShellWindow* window = ShellWindowRegistry::Get(profile())->
         GetCurrentShellWindowForApp(GetExtension()->id());
     if (window) {
-      tab_contents = window->tab_contents();
+      contents = window->web_contents();
     } else {
       // Abort showing the dialog. TODO(estade) Perhaps return an error
instead.
       GetAndReturnGalleries();
@@ -183,7 +186,7 @@ void MediaGalleriesGetMediaFileSystemsFunction::ShowDialog()
{
   // Controller will delete itself.
   base::Closure cb = base::Bind(
       &MediaGalleriesGetMediaFileSystemsFunction::GetAndReturnGalleries, this);
-  new chrome::MediaGalleriesDialogController(tab_contents, *GetExtension(),
cb);
+  new chrome::MediaGalleriesDialogController(contents, *GetExtension(), cb);
 }
 
 // MediaGalleriesAssembleMediaFileFunction
-------------------------------------

[commit-bot: I haz the power, 2012-10-19 17:01:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11138010/24001

[commit-bot: I haz the power, 2012-10-19 17:33:43]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-19 17:47:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11138010/24001

[commit-bot: I haz the power, 2012-10-19 18:17:21]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-19 19:54:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11138010/24001

[commit-bot: I haz the power, 2012-10-19 20:45:24]

Sorry for I got bad news for ya.
Compile failed with a clobber build.
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-19 20:57:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/avi@chromium.org/11138010/24001