Address bar is shown on pop-out extension windows.

Address bar is shown on pop-out extension windows.

The code path for window.open() call from extensions is not setting the
app_name_ on the browser instance which is causing the location bar to
be displayed.

This is a regression from "Cleanup popup related browser navigation
code." - svn://svn.chromium.org/chrome/trunk/src@83399

BUG=83692
TEST=Invoke window.open() from an extension background page.  There
should be no location bar displayed.

modified:   chrome/browser/extensions/extension_host.cc
modified:   chrome/browser/ui/browser_navigator.cc

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

[prasadt, 2011-05-24 04:18:20]

Is there some test that I can extend to add coverage for this?  The test should
do the following:

- Have an extension background page do window.open()
- Verify that the Browser instance that gets created would return true for
browser->is_app().

[stevenjb, 2011-05-24 20:39:29]

LGTM

We should at least add tests to browser_navigator_browsertest.cc to test that
setting params.extension_app_id results in browser.is_app() is true. Testing
window.open() behavior is probably more complicated, I am not certain where
those tests might be.

[Mihai Parparita -not on Chrome, 2011-05-24 21:45:47]

We have some tests for extension window.open behavior:

http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/exten...

Mihai

On Tue, May 24, 2011 at 1:39 PM, <stevenjb@chromium.org> wrote:

> LGTM
>
> We should at least add tests to browser_navigator_browsertest.cc to test
> that
> setting params.extension_app_id results in browser.is_app() is true.
> Testing
> window.open() behavior is probably more complicated, I am not certain where
> those tests might be.
>
>
>
>
>
> http://codereview.chromium.org/6992018/
>

[prasadt, 2011-05-25 03:02:39]

On 2011/05/24 20:39:29, Steven Bennetts wrote:
> LGTM
> 
> We should at least add tests to browser_navigator_browsertest.cc to test that
> setting params.extension_app_id results in browser.is_app() is true. Testing
> window.open() behavior is probably more complicated, I am not certain where
> those tests might be.

Added a test that'll test for this.  Please take another look.

[prasadt, 2011-05-25 03:06:15]

On 2011/05/24 21:45:47, Mihai Parparita wrote:
> We have some tests for extension window.open behavior:
> 
>
http://www.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/chrome/browser/exten...

Here is a comment from the bottom of that file :-)
//==============================================================================
// STOP! Please do not add any more random-ass tests here. Create new files for
// your tests grouped by functionality. Also, you should strongly consider using
// ExtensionAPITest if possible.
//==============================================================================

I added a new test to window_open_apitest.cc.  Please take a look.  Thanks.

> 
> Mihai
> 
> On Tue, May 24, 2011 at 1:39 PM, <mailto:stevenjb@chromium.org> wrote:
> 
> > LGTM
> >
> > We should at least add tests to browser_navigator_browsertest.cc to test
> > that
> > setting params.extension_app_id results in browser.is_app() is true.
> > Testing
> > window.open() behavior is probably more complicated, I am not certain where
> > those tests might be.
> >
> >
> >
> >
> >
> > http://codereview.chromium.org/6992018/
> >

[Erik does not do reviews, 2011-05-25 15:13:03]

http://codereview.chromium.org/6992018/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/6992018/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_host.cc:589: params.extension_app_id =
extension_id_;
I forget the semantics of ShowCreatedWindow in this context.  Will it only be
called for URLs that are part of the extension?  Or is it called for any URL
opened from within the extension?  Do you know if this change means that we've
changed behavior in this case?

In any event, if this param is the bit that causes the popup to open up without
an address bar, please add a comment to that effect so that others don't have to
go digging to track down the source of this behavior.

http://codereview.chromium.org/6992018/diff/3001/chrome/browser/extensions/wi...
File chrome/browser/extensions/window_open_apitest.cc (right):

http://codereview.chromium.org/6992018/diff/3001/chrome/browser/extensions/wi...
chrome/browser/extensions/window_open_apitest.cc:52:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BrowserIsApp) {
Could you add a test where the app opens a popup to a random external URL as
well?  I don't have a strong opinion about what the behavior should be other
than it match the behavior prior to the regression you're fixing with this CL.

[prasadt, 2011-05-25 17:53:00]

http://codereview.chromium.org/6992018/diff/3001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_host.cc (right):

http://codereview.chromium.org/6992018/diff/3001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_host.cc:589: params.extension_app_id =
extension_id_;
On 2011/05/25 15:13:03, Erik Kay wrote:
> I forget the semantics of ShowCreatedWindow in this context.  Will it only be
> called for URLs that are part of the extension?  Or is it called for any URL
> opened from within the extension?

This is called for any URL opened with window.open() within the extension.

> Do you know if this change means that we've
> changed behavior in this case?

We didn't change behavior.  This is the same behavior as it was before the
regression happened.

> In any event, if this param is the bit that causes the popup to open up
without
> an address bar, please add a comment to that effect so that others don't have
to
> go digging to track down the source of this behavior.

Not sure of the value of the comment here, but added on anyway.

http://codereview.chromium.org/6992018/diff/3001/chrome/browser/extensions/wi...
File chrome/browser/extensions/window_open_apitest.cc (right):

http://codereview.chromium.org/6992018/diff/3001/chrome/browser/extensions/wi...
chrome/browser/extensions/window_open_apitest.cc:52:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, BrowserIsApp) {
On 2011/05/25 15:13:03, Erik Kay wrote:
> Could you add a test where the app opens a popup to a random external URL as
> well?  I don't have a strong opinion about what the behavior should be other
> than it match the behavior prior to the regression you're fixing with this CL.

Done.

[Erik does not do reviews, 2011-05-25 19:23:38]

LGTM

[stevenjb, 2011-05-25 21:16:34]

LGTM

[Mihai Parparita -not on Chrome, 2011-05-26 00:11:29]

LGTM