Don't use browser windows for platform app shell windows

Don't use browser windows for platform app shell windows.

This frees platform apps from having to disable browser behaviors they don't
want (e.g. keyboard shortcuts).

Reverts part of r112378, which add a "shell" browser window type, and redoes
part of r114162 to not depend on browser windows when checking if the context
menu is being displayed for a platform app.

(reland of r116803, which got reverted with r116810 -- now builds on Chrome OS)

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

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

[Mihai Parparita -not on Chrome, 2011-12-20 05:13:09]

(--send-mail didn't seem to actually send the mail, so I'm resending this)

Don't use browser windows for platform app shell windows

This frees platform apps from having to disable browser behaviors they don't 
want (e.g. keyboard shortcuts). 

Reverts part of r112378, which add a "shell" browser window type, and redoes 
part of r114162 to not depend on browser windows when checking if the context 
menu is being displayed for a platform app.

[miket_OOO, 2011-12-20 19:12:34]

LGTM. LVGTM.

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_process_manager.cc:123:
host->CreateView(NULL);
I wish ExtensionHost had something like CreateViewWithoutBrowser() instead of
accepting NULL as a valid argument. It forces people to go to the .h and read
comments. Hey, wait a minute, maybe you can do that now!

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:531: return;
This early return scares me in an Init method, particularly one as long as this
one. If there's a way to refactor this method to make it more obvious that it's
following either of two paths, I could see fewer weird bugs cropping up where
someone expected code to run that didn't. Like this:

InitMenu() {
  ...stuff...
  if (one_test) {
    InitOneWay();
  } else {
    InitAnotherWay();
  }
  DoStuffForAllWays();
}

Just a suggestion. I see another return already there, so maybe it's OK as-is.

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/ui/gtk/extens...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/ui/gtk/extens...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:17:
gtk_widget_set_size_request(GTK_WIDGET(window_), 512, 384);
Original Mac resolution?

http://codereview.chromium.org/8985008/diff/2001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8985008/diff/2001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:2957: 'browser/ui/extensions/shell_window.cc',
alphabetize, please.

[Mihai Parparita -not on Chrome, 2011-12-20 22:18:20]

Thanks for the prompt review. Unfortunately there's some tryserver failures
and I'm leaving for the airport soon, so this will have to wait until I get
back Friday afternoon.

Mihai

On Tue, Dec 20, 2011 at 11:12 AM, <miket@chromium.org> wrote:

> LGTM. LVGTM.
>
>
> http://codereview.chromium.**org/8985008/diff/2001/chrome/**
>
browser/extensions/extension_**process_manager.cc<http://codereview.chromium.org/8985008/diff/2001/chrome/browser/extensions/extension_process_manager.cc>
> File chrome/browser/extensions/**extension_process_manager.cc (right):
>
> http://codereview.chromium.**org/8985008/diff/2001/chrome/**
>
browser/extensions/extension_**process_manager.cc#newcode123<http://codereview.chromium.org/8985008/diff/2001/chrome/browser/extensions/extension_process_manager.cc#newcode123>
> chrome/browser/extensions/**extension_process_manager.cc:**123:
> host->CreateView(NULL);
> I wish ExtensionHost had something like CreateViewWithoutBrowser()
> instead of accepting NULL as a valid argument. It forces people to go to
> the .h and read comments. Hey, wait a minute, maybe you can do that now!
>
> http://codereview.chromium.**org/8985008/diff/2001/chrome/**
>
browser/tab_contents/render_**view_context_menu.cc<http://codereview.chromium.org/8985008/diff/2001/chrome/browser/tab_contents/render_view_context_menu.cc>
> File chrome/browser/tab_contents/**render_view_context_menu.cc (right):
>
> http://codereview.chromium.**org/8985008/diff/2001/chrome/**
>
browser/tab_contents/render_**view_context_menu.cc#**newcode531<http://codereview.chromium.org/8985008/diff/2001/chrome/browser/tab_contents/render_view_context_menu.cc#newcode531>
> chrome/browser/tab_contents/**render_view_context_menu.cc:**531: return;
> This early return scares me in an Init method, particularly one as long
> as this one. If there's a way to refactor this method to make it more
> obvious that it's following either of two paths, I could see fewer weird
> bugs cropping up where someone expected code to run that didn't. Like
> this:
>
> InitMenu() {
>  ...stuff...
>  if (one_test) {
>    InitOneWay();
>  } else {
>    InitAnotherWay();
>  }
>  DoStuffForAllWays();
> }
>
> Just a suggestion. I see another return already there, so maybe it's OK
> as-is.
>
> http://codereview.chromium.**org/8985008/diff/2001/chrome/**
>
browser/ui/gtk/extensions/**shell_window_gtk.cc<http://codereview.chromium.org/8985008/diff/2001/chrome/browser/ui/gtk/extensions/shell_window_gtk.cc>
> File chrome/browser/ui/gtk/**extensions/shell_window_gtk.cc (right):
>
> http://codereview.chromium.**org/8985008/diff/2001/chrome/**
>
browser/ui/gtk/extensions/**shell_window_gtk.cc#newcode17<http://codereview.chromium.org/8985008/diff/2001/chrome/browser/ui/gtk/extensions/shell_window_gtk.cc#newcode17>
> chrome/browser/ui/gtk/**extensions/shell_window_gtk.**cc:17:
> gtk_widget_set_size_request(**GTK_WIDGET(window_), 512, 384);
> Original Mac resolution?
>
> http://codereview.chromium.**org/8985008/diff/2001/chrome/**
>
chrome_browser.gypi<http://codereview.chromium.org/8985008/diff/2001/chrome/chrome_browser.gypi>
> File chrome/chrome_browser.gypi (right):
>
> http://codereview.chromium.**org/8985008/diff/2001/chrome/**
>
chrome_browser.gypi#**newcode2957<http://codereview.chromium.org/8985008/diff/2001/chrome/chrome_browser.gypi#newcode2957>
> chrome/chrome_browser.gypi:**2957:
> 'browser/ui/extensions/shell_**window.cc',
> alphabetize, please.
>
>
http://codereview.chromium.**org/8985008/<http://codereview.chromium.org/8985...
>

[jeremya, 2012-01-04 03:41:26]

I have a quick and dirty implementation of a ShellWindow for Views on top of
this: http://codereview.chromium.org/9030017

[Mihai Parparita -not on Chrome, 2012-01-04 22:46:45]

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_process_manager.cc:123:
host->CreateView(NULL);
On 2011/12/20 19:12:34, miket wrote:
> I wish ExtensionHost had something like CreateViewWithoutBrowser() instead of
> accepting NULL as a valid argument. It forces people to go to the .h and read
> comments. Hey, wait a minute, maybe you can do that now!

Done.

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/render_view_context_menu.cc:531: return;
On 2011/12/20 19:12:34, miket wrote:
> This early return scares me in an Init method, particularly one as long as
this
> one.

I've cleaned things up a bit, so that platform app menu initialization is
contained in a couple  GetPlatformApp/AppendPlatformAppItems methods, so that
the early return is more obvious.

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/ui/gtk/extens...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.cc (right):

http://codereview.chromium.org/8985008/diff/2001/chrome/browser/ui/gtk/extens...
chrome/browser/ui/gtk/extensions/shell_window_gtk.cc:17:
gtk_widget_set_size_request(GTK_WIDGET(window_), 512, 384);
On 2011/12/20 19:12:34, miket wrote:
> Original Mac resolution?

If it was good enough in 1984, it's good enough now. Added a TODO for allowing
window sizes to be specified.

http://codereview.chromium.org/8985008/diff/2001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/8985008/diff/2001/chrome/chrome_browser.gypi#n...
chrome/chrome_browser.gypi:2957: 'browser/ui/extensions/shell_window.cc',
On 2011/12/20 19:12:34, miket wrote:
> alphabetize, please.

Done.

[commit-bot: I haz the power, 2012-01-06 06:43:15]

No LGTM from valid reviewers yet.

[Mihai Parparita -not on Chrome, 2012-01-06 06:46:39]

+John or Avi for the content/common changes.

[jam, 2012-01-06 16:48:11]

On 2012/01/06 06:46:39, Mihai Parparita wrote:
> +John or Avi for the content/common changes.

i don't see any changes to content?

[Mihai Parparita -not on Chrome, 2012-01-06 19:17:30]

On Fri, Jan 6, 2012 at 8:48 AM, <jam@chromium.org> wrote:

> On 2012/01/06 06:46:39, Mihai Parparita wrote:
>
>> +John or Avi for the content/common changes.
>>
>
> i don't see any changes to content?
>

Oops, my bad, I misread chrome/common as content/common.

Mihai

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

Scott, could you look at the chrome/browser/ui changes (the OWNERS file got a
set noparent since I started this CL)?

Thanks,
Mihai

[sky, 2012-01-06 20:10:25]

http://codereview.chromium.org/8985008/diff/32001/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/8985008/diff/32001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:14: #include
"chrome/browser/ui/gtk/extensions/shell_window_gtk.h"
General pattern for these sort of things is to define a static function to do
the creation so that you don't have to include all the headers here. See
BrowserWindow::Create for an example of this.

http://codereview.chromium.org/8985008/diff/32001/chrome/browser/ui/gtk/exten...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.h (right):

http://codereview.chromium.org/8985008/diff/32001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.h:32: };
DISALLOW_COPY_AND_ASSIGN

[sky, 2012-01-06 20:10:56]

Also, I'm not the most knowledgeable gtk person. One of the Evans or Elliot is a
better reviewer for that

[Mihai Parparita -not on Chrome, 2012-01-06 21:33:19]

Elliot, do you mind looking at the GTK bits? Generally, the intent is to have a
bare-bones window that can display extension content, so no fancy functionality
is needed (for now).

Thanks,
Mihai

http://codereview.chromium.org/8985008/diff/32001/chrome/browser/ui/extension...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/8985008/diff/32001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:14: #include
"chrome/browser/ui/gtk/extensions/shell_window_gtk.h"
On 2012/01/06 20:10:25, sky wrote:
> General pattern for these sort of things is to define a static function to do
> the creation so that you don't have to include all the headers here. See
> BrowserWindow::Create for an example of this.

Done (I was following the pattern of ExtensionHost::CreateView, but that's a bit
different, since the platform-specific ExtensionView classes don't share a base
class).

http://codereview.chromium.org/8985008/diff/32001/chrome/browser/ui/gtk/exten...
File chrome/browser/ui/gtk/extensions/shell_window_gtk.h (right):

http://codereview.chromium.org/8985008/diff/32001/chrome/browser/ui/gtk/exten...
chrome/browser/ui/gtk/extensions/shell_window_gtk.h:32: };
On 2012/01/06 20:10:25, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[Elliot Glaysher, 2012-01-06 22:06:43]

I don't see anything immediately wrong with your new shell window but I get
nervous whenever people make windows.

shaky lgtm

[sky, 2012-01-06 22:52:14]

LGTM

[commit-bot: I haz the power, 2012-01-06 23:06:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/8985008/41002

[commit-bot: I haz the power, 2012-01-07 00:06:00]

Try job failure for 8985008-41002 (retry) on linux_rel for steps "browser_tests,
ui_tests" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-01-07 00:17:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/8985008/41002

[commit-bot: I haz the power, 2012-01-07 03:09:06]

Change committed as 116803

[Mihai Parparita -not on Chrome, 2012-01-07 20:18:10]

This got reverted with r116810 due to build failures on Chrome OS:
TOOLKIT_USES_GTK is true for Chrome OS too, the check instead should be for
TOOLKIT_GTK. I've updated shell_window.cc with that, and have started a bunch of
ChromeOS tryjobs. Will re-land once they finish.

[commit-bot: I haz the power, 2012-01-07 21:45:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/8985008/53002

[commit-bot: I haz the power, 2012-01-07 22:52:58]

Change committed as 116834

[Mihai Parparita -not on Chrome, 2012-01-09 02:12:23]

This got reverted (again) with r116842 since the platform app tests were failing
on the Mac debug bots. In retrospect, I'm surprised they worked at all (even in
release), since platform apps are effectively only implemented on GTK with this
patch. I'm doing to disable the tests on non-GTK platforms. I've sent off debug
tryjobs, will re-re-land once they finish.

[commit-bot: I haz the power, 2012-01-09 04:28:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mihaip@chromium.org/8985008/48023

[commit-bot: I haz the power, 2012-01-09 05:48:34]

Change committed as 116855