Changes to insure that when in app-mode, links open in the default browser.

Changes to insure that when in app-mode, links open in the default browser.  This change should have no affect on Chrome when not in app-mode.Tested against Gmail.  UITests added as app_mode_navigation_uitest.

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

[brg, 2009-01-15 21:22:00]

Some style changes and more restrictive scoping of when to open new links in the
default browser.

[darin (slow to review), 2009-01-16 05:08:48]

http://codereview.chromium.org/18093/diff/10/206
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/18093/diff/10/206#newcode822
Line 822: if (navigate_by_default_browser_ && validated_params.url.is_valid() &&
this seems wrong to me.  OnMsgNavigate is surely reached for navigations within
an application.

http://codereview.chromium.org/18093/diff/10/206#newcode1030
Line 1030: ShellExecute(NULL, L"open", new_url.c_str(), NULL, NULL,
SW_SHOWNORMAL);
it is a bad idea to call ShellExecute in the raw here.  please use
ExternalProtocolHandler::LaunchUrl instead.

[brg, 2009-01-16 13:08:23]

Thank you for the initial comments.  Please see my email for the design decision
of modifying RenderViewHost.  I will post that message and your replies in a
future publish comments.

http://codereview.chromium.org/18093/diff/10/206
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/18093/diff/10/206#newcode822
Line 822: if (navigate_by_default_browser_ && validated_params.url.is_valid() &&
On 2009/01/16 05:08:49, darin wrote:

Not for Gmails hacky window.open meta-refresh, this was the only place I could
find that URL being passed to an object which was aware of app-mode.

http://codereview.chromium.org/18093/diff/10/206#newcode1030
Line 1030: ShellExecute(NULL, L"open", new_url.c_str(), NULL, NULL,
SW_SHOWNORMAL);
On 2009/01/16 05:08:49, darin wrote:

Done.  Good catch.

[Ben Goodger (Google), 2009-01-16 20:13:44]

http://codereview.chromium.org/18093/diff/13/217
File chrome/browser/browser.cc (left):

http://codereview.chromium.org/18093/diff/13/217#oldcode1651
Line 1651: transition = PageTransition::START_PAGE;
why are you removing this?

http://codereview.chromium.org/18093/diff/13/217
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/18093/diff/13/217#newcode1653
Line 1653: if (type_ != TYPE_APP)
this seems like it could cause problems?

http://codereview.chromium.org/18093/diff/13/214
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/18093/diff/13/214#newcode833
Line 833: // in the default browser.
this should be broken out into a separate function

http://codereview.chromium.org/18093/diff/13/214#newcode1042
Line 1042: // dialog.  Note that Firefox is not white-listed as an external
this seems like it can be shared with the aforementioned function.

http://codereview.chromium.org/18093/diff/13/216
File chrome/browser/render_view_host.h (right):

http://codereview.chromium.org/18093/diff/13/216#newcode111
Line 111: void SetNavigateByDefaultBrowser(bool navigate) {
use set_foo style.

http://codereview.chromium.org/18093/diff/13/215
File chrome/browser/web_contents_view_win.cc (right):

http://codereview.chromium.org/18093/diff/13/215#newcode391
Line 391:
new_web_contents->render_view_host()->SetNavigateByDefaultBrowser(true);
It's kind of unfortunate to add all this high level logic in here (tabs, web
apps, etc). I think it'd be better to ask the delegate whether or not loads
should happen inside the frame.

[brg, 2009-01-16 20:51:24]

The changes in renderview now make it so the urls are not downloaded in the
hidden windows.

Made changes for all comments but one (initialization).

http://codereview.chromium.org/18093/diff/13/217
File chrome/browser/browser.cc (left):

http://codereview.chromium.org/18093/diff/13/217#oldcode1651
Line 1651: transition = PageTransition::START_PAGE;
On 2009/01/16 20:13:44, Ben Goodger wrote:

Done.

http://codereview.chromium.org/18093/diff/13/217
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/18093/diff/13/217#newcode1653
Line 1653: if (type_ != TYPE_APP)
On 2009/01/16 20:13:44, Ben Goodger wrote:

This is necessary since we use the renderview and renderviewhost to open, but we
hide their windows in application mode.

http://codereview.chromium.org/18093/diff/13/214
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/18093/diff/13/214#newcode833
Line 833: // in the default browser.
On 2009/01/16 20:13:44, Ben Goodger wrote:

Done.

http://codereview.chromium.org/18093/diff/13/214#newcode1042
Line 1042: // dialog.  Note that Firefox is not white-listed as an external
On 2009/01/16 20:13:44, Ben Goodger wrote:

Done.

http://codereview.chromium.org/18093/diff/13/216
File chrome/browser/render_view_host.h (right):

http://codereview.chromium.org/18093/diff/13/216#newcode111
Line 111: void SetNavigateByDefaultBrowser(bool navigate) {
On 2009/01/16 20:13:44, Ben Goodger wrote:

Done.  However, this style is not consistent with the file.

http://codereview.chromium.org/18093/diff/13/215
File chrome/browser/web_contents_view_win.cc (right):

http://codereview.chromium.org/18093/diff/13/215#newcode391
Line 391:
new_web_contents->render_view_host()->SetNavigateByDefaultBrowser(true);
On 2009/01/16 20:13:44, Ben Goodger wrote:

The new renderview will talk to the render_view_host to control opening.  This
is where the render_view_host is initialized, and I see this as a continuation
of the previous step.

[darin (slow to review), 2009-01-16 20:58:10]

http://codereview.chromium.org/18093/diff/220/225
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/18093/diff/220/225#newcode1581
Line 1581: bool is_application_fork =
what if the application needs to open a new window to show some element of the
application?  it is a very normal thing to open a new window to about:blank, and
then start dynamically adding content to it.

the only reason we can get away with is_fork is because of the magic nulling of
the window.opener property that gmail does.  and even that is fragile.  we are
basically assuming that because window.opener is null that the opened page will
not expect to be able to script back to the opener.  however, in practice there
are ways that a web page could expect to be able to do that even with the opener
being null.

[darin (slow to review), 2009-01-16 20:59:31]

This CL is also missing tests.  There are a number of interesting ways that a
web page can create a new window, and we should have tests to ensure that those
are all handled correctly.  At this stage of developing the patch, these tests
should help ensure that we are thinking about all cases.  Can you share your
tests so I can see if there are any cases that aren't being considered?

[brg, 2009-01-16 21:08:56]

http://codereview.chromium.org/18093/diff/220/225
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/18093/diff/220/225#newcode1581
Line 1581: bool is_application_fork =
On 2009/01/16 20:58:10, darin wrote:

I do not believe this is as fragile as your worries indicate.  The reason is
that we must be in application mode for this code branch to be hit, and we must
have create a new browser window from the main frame.

In app mode, there can only ever be one frame.  All links except those
navigating within the same frame must open in the default browser.  Hence once
we know that we are in app mode (by the command line), that we are in a not
visible window (Show for creating new windows is disabled in app-mode) which has
an opener (newly created window from the main frame) and is not about:blank (the
gmail hack) then we are ok.

The disposition and type checks are extraneous.

[darin (slow to review), 2009-01-16 21:22:25]

http://codereview.chromium.org/18093/diff/220/225
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/18093/diff/220/225#newcode1581
Line 1581: bool is_application_fork =
So, the problem with routing all new windows to the default browser is that it
prevents scripting between the app window and newly opened window.  For some
applications, that scripting may be very important.  For example, imagine the
case of popping out a chat window from Gmail.

[darin (slow to review), 2009-01-16 21:25:03]

http://codereview.chromium.org/18093/diff/220/221
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/18093/diff/220/221#newcode1312
Line 1312: &ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck,
this is wrong.  it would result in your compute getting owned by an attacker. 
please use ExternalProtocolHandler::LaunchUrl instead.  (if you use that it will
also take care of putting the work on the appropriate background thread for
you.)

[brettw, 2009-01-16 21:28:07]

http://codereview.chromium.org/18093/diff/220/221
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/18093/diff/220/221#newcode1009
Line 1009: if (navigate_by_default_browser_) {
I don't think the RenderViewHost layer is the appropriate layer for this logic.
RenderViewHost is our multiprocess embedding layer. It should be appropriate for
more general cases and not have application-specific logic in it like this when
possible (think of it as a higher-level WebKit glue).

I could probably be convinced that this should be in WebContents or Browser
(maybe Ben will have input here).

http://codereview.chromium.org/18093/diff/220/221#newcode1301
Line 1301: if (!url.spec().empty()) {
Use url.is_empty().

http://codereview.chromium.org/18093/diff/220/221#newcode1310
Line 1310: io_loop->PostTask(FROM_HERE,
It looks like ExternalProtocolHandler::LaunchURL does what you need, actually.
If you really have to do this kind of thing yourself, you should use the file
thread for this kind of thing (as LaunchURL does). The file thread is really the
"blocking I/O thread".

http://codereview.chromium.org/18093/diff/220/225
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/18093/diff/220/225#newcode1602
Line 1602: url != GURL("about:blank");  // TODO(brg) : Remove this with Gmail
fix.
This type of thing needs to be a function. 20 line if statements are not
readable, especially with embedded comments.

[brg, 2009-01-17 02:31:25]

Thank you for the discussions.

I've updated the change to be more similar to the initial check in.  Here the
logic to launch the external protocol handler is in WebContents and the logic to
determine when to launch externally is done by Browser and exposed through
TabContentsDelegate.

The feature now runs as follows.  When a user clicks on a link, Chrome launches
a new tabbed browser.  This is not shown to the user.  When the navigation URL
is finally sent from the rendered to this browser window, we launch the external
protocol handler for the url and close the browser window.  I believe this is
currently required since the Renderer does not provide the url to the browser
until after it has begun loading.

http://codereview.chromium.org/18093/diff/220/221
File chrome/browser/render_view_host.cc (right):

http://codereview.chromium.org/18093/diff/220/221#newcode1009
Line 1009: if (navigate_by_default_browser_) {
On 2009/01/16 21:28:07, brettw wrote:

Done.

http://codereview.chromium.org/18093/diff/220/221#newcode1301
Line 1301: if (!url.spec().empty()) {
On 2009/01/16 21:28:07, brettw wrote:

Done.

http://codereview.chromium.org/18093/diff/220/221#newcode1310
Line 1310: io_loop->PostTask(FROM_HERE,
On 2009/01/16 21:28:07, brettw wrote:

This is code directly from LaunchURL.  The reason to bypass the check is in my
response to darin below -- Firefox is not whitelisted.  Hence, every time a link
is clicked the user gets the dialog.  Since the url was verified before this was
called, it should be safe no?

http://codereview.chromium.org/18093/diff/220/221#newcode1312
Line 1312: &ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck,
On 2009/01/16 21:25:03, darin wrote:

As the comment to this call mentions, LaunchUrl does not white-list firefox, so
even checking to insure that "http" is allowed will not stop every url from
bothering the user.  So instead I insure manual validation is done by FilterUrl
before ever attempting to launch the external browser (I tried to mention this
in the comment and name of the method).

http://codereview.chromium.org/18093/diff/220/225
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/18093/diff/220/225#newcode1581
Line 1581: bool is_application_fork =
On 2009/01/16 21:22:25, darin wrote:

You are correct, I've removed this.

http://codereview.chromium.org/18093/diff/220/225#newcode1602
Line 1602: url != GURL("about:blank");  // TODO(brg) : Remove this with Gmail
fix.
On 2009/01/16 21:28:07, brettw wrote:

Done.

[brg, 2009-01-17 02:57:03]

small typo; publishing comments.

http://codereview.chromium.org/18093/diff/232/234
File chrome/browser/browser.h (right):

http://codereview.chromium.org/18093/diff/232/234#newcode1
Line 1: sta// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
typo will be corrected in next upload

[Ben Goodger (Google), 2009-01-20 22:07:49]

I want to hear from Darin about the implications of launching URLs the way you
are.

Were you going to write a UI test for this?

http://codereview.chromium.org/18093/diff/232/233
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/18093/diff/232/233#newcode1659
Line 1659: // url is inaccessbile and so we hide the window until the url is
Comment nit: you're not hiding the window, you're just not showing it.

http://codereview.chromium.org/18093/diff/232/234
File chrome/browser/browser.h (right):

http://codereview.chromium.org/18093/diff/232/234#newcode627
Line 627: bool new_tabs_in_default_browser_;
Comment please.

http://codereview.chromium.org/18093/diff/232/237
File chrome/browser/web_contents.cc (right):

http://codereview.chromium.org/18093/diff/232/237#newcode1
Line 1: star// Copyright (c) 2006-2008 The Chromium Authors. All rights
reserved.
Make sure you get this one too.

[brg, 2009-01-21 00:39:04]

http://codereview.chromium.org/18093/diff/232/233
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/18093/diff/232/233#newcode1659
Line 1659: // url is inaccessbile and so we hide the window until the url is
On 2009/01/20 22:07:49, Ben Goodger wrote:

Done.

http://codereview.chromium.org/18093/diff/232/234
File chrome/browser/browser.h (right):

http://codereview.chromium.org/18093/diff/232/234#newcode1
Line 1: sta// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/01/17 02:57:04, brg wrote:

Done.

http://codereview.chromium.org/18093/diff/232/234#newcode627
Line 627: bool new_tabs_in_default_browser_;
On 2009/01/20 22:07:49, Ben Goodger wrote:

Done.

http://codereview.chromium.org/18093/diff/232/237
File chrome/browser/web_contents.cc (right):

http://codereview.chromium.org/18093/diff/232/237#newcode1
Line 1: star// Copyright (c) 2006-2008 The Chromium Authors. All rights
reserved.
On 2009/01/20 22:07:49, Ben Goodger wrote:

Done.

[brettw, 2009-01-21 05:46:08]

http://codereview.chromium.org/18093/diff/250/255
File chrome/browser/web_contents.cc (right):

http://codereview.chromium.org/18093/diff/250/255#newcode628
Line 628: &ExternalProtocolHandler::LaunchUrlWithoutSecurityCheck,
It looks like you only changed one of the callers of this function rather than
all of them (and removing this function which has a security hold and uses the
wrong thread).

[brg, 2009-01-22 10:38:55]

Updated all comments.

Added UItests for app mode navigation.  Outside of app mode, it seems the cases
are covered.

http://codereview.chromium.org/18093/diff/49/270
File chrome/browser/appmodenavigation_uitest.cc (right):

http://codereview.chromium.org/18093/diff/49/270#newcode193
Line 193: }  // namespace
The empty line appears on my client, but is not uploading.  Will resolve after
discussion.

[darin (slow to review), 2009-01-22 17:32:36]

http://codereview.chromium.org/18093/diff/49/270
File chrome/browser/appmodenavigation_uitest.cc (right):

http://codereview.chromium.org/18093/diff/49/270#newcode29
Line 29: class AppmodeNavigationTest : public UITest {
style-nit: should be AppModeNavigationTest

also, this file should be app_mode_navigation_uitest.cc

http://codereview.chromium.org/18093/diff/49/266
File chrome/browser/browser.h (right):

http://codereview.chromium.org/18093/diff/49/266#newcode652
Line 652: // The default value is false, and is set to true in browser.cc.
nit: comments like this that refer to what another file does risk getting out of
date if the code in that file changes.  best not to write comments that
duplicate what code does.

also, i think this comment is probably better placed near the public setter. 
the name of the method "set_new_tabs_in_default_browser" is not entirely clear.

http://codereview.chromium.org/18093/diff/49/268
File chrome/browser/tab_contents/web_contents.cc (right):

http://codereview.chromium.org/18093/diff/49/268#newcode692
Line 692: if (!(params.url.is_empty()) && (NULL != rvh) && (NULL !=
rvh->process())) {
nit: instead of comparing to NULL, just write "&& rvh && rvh->process()"

that is easier to read imo :)

http://codereview.chromium.org/18093/diff/49/268#newcode978
Line 978: if (!url.is_empty() && (NULL != rvh) && (NULL != rvh->process())) {
ditto

however, there seems to be a bit of code duplication here.  please refactor into
a helper method.

http://codereview.chromium.org/18093/diff/49/271
File chrome/test/data/appmodenavigation_test.html (right):

http://codereview.chromium.org/18093/diff/49/271#newcode5
Line 5: w = window.open();
nit: please indent the body of this function

http://codereview.chromium.org/18093/diff/49/271#newcode13
Line 13: <a href="http://www.google.com" target=blank> Link Test </a>
target=_blank right?

[brg, 2009-01-22 19:03:01]

http://codereview.chromium.org/18093/diff/49/270
File chrome/browser/appmodenavigation_uitest.cc (right):

http://codereview.chromium.org/18093/diff/49/270#newcode29
Line 29: class AppmodeNavigationTest : public UITest {
On 2009/01/22 17:32:36, darin wrote:

Done.

http://codereview.chromium.org/18093/diff/49/266
File chrome/browser/browser.h (right):

http://codereview.chromium.org/18093/diff/49/266#newcode652
Line 652: // The default value is false, and is set to true in browser.cc.
On 2009/01/22 17:32:36, darin wrote:

Done.

http://codereview.chromium.org/18093/diff/49/268
File chrome/browser/tab_contents/web_contents.cc (right):

http://codereview.chromium.org/18093/diff/49/268#newcode692
Line 692: if (!(params.url.is_empty()) && (NULL != rvh) && (NULL !=
rvh->process())) {
On 2009/01/22 17:32:36, darin wrote:

Done.

http://codereview.chromium.org/18093/diff/49/268#newcode978
Line 978: if (!url.is_empty() && (NULL != rvh) && (NULL != rvh->process())) {
On 2009/01/22 17:32:36, darin wrote:

Done.

http://codereview.chromium.org/18093/diff/49/271
File chrome/test/data/appmodenavigation_test.html (right):

http://codereview.chromium.org/18093/diff/49/271#newcode5
Line 5: w = window.open();
On 2009/01/22 17:32:36, darin wrote:

Done.

http://codereview.chromium.org/18093/diff/49/271#newcode13
Line 13: <a href="http://www.google.com" target=blank> Link Test </a>
On 2009/01/22 17:32:36, darin wrote:

It works the same either way.

[brettw, 2009-01-22 19:19:21]

Looks like you forgot to add the uitest file to the CL.

[brg, 2009-01-22 19:38:59]

RE: UITest not in changelist.  That was odd, it was in the changelist.  I might
have set the type to M instead of A.

[brettw, 2009-01-22 20:27:03]

LGTM with these minor nits.

http://codereview.chromium.org/18093/diff/304/68
File chrome/browser/app_mode_navigation_uitest.cc (right):

http://codereview.chromium.org/18093/diff/304/68#newcode61
Line 61: std::wstring WindowCaptionFromPageTitle(std::wstring page_title) {
It doesn't matter since it's a tests, but it's good practice to pass strings as
const references.

http://codereview.chromium.org/18093/diff/304/68#newcode193
Line 193: }  // namespace
Put a blank line above this.

[brg, 2009-01-22 23:33:03]

http://codereview.chromium.org/18093/diff/304/68
File chrome/browser/app_mode_navigation_uitest.cc (right):

http://codereview.chromium.org/18093/diff/304/68#newcode61
Line 61: std::wstring WindowCaptionFromPageTitle(std::wstring page_title) {
On 2009/01/22 20:27:03, brettw wrote:

Done.

http://codereview.chromium.org/18093/diff/304/68#newcode193
Line 193: }  // namespace
On 2009/01/22 20:27:03, brettw wrote:

Done.

[Mark Larson, 2009-01-23 07:02:32]

So, I'm using Chromium r8526. I run Gmail as an app in Chromium trunk builds. It
keeps me insane, and keeps me in touch with what's going on with the latest
code.

However, I also have Google Chrome installed (1.0.154.43), and *that* is my
default browser.

So, now when I click on a link from Gmail, all hell breaks loose. 

First: I get a Chromium protocol handler prompt asking if I want to launch
Google Chrome to handle the request. Ick.

I say yes, and it opens the link just fine.

Now, go to a Chromium tabbed browser window and click a link . I see the
protocol handler dialog flash briefly and then disappear as the tab I clicked on
gets closed. Dang! 

OK, I'll just Ctrl+Shift+T to reopen that tab. Nope. It opens and closes
immediately. No new tabs can be opened, and any attempt to navigate existing
tabs closes them.

I'm guessing we fail to unset the 'open in default browser' bool, so now every
navigation in Chromium results in an (failed) attempt to launch my default
browser. 

I'll file a bug, but this change is driving me crazy right now.

http://codereview.chromium.org/18093/diff/76/77
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/18093/diff/76/77#newcode1660
Line 1660: b->set_open_new_windows_in_default_browser(true);
When does this get unset?