Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(41)

Issue 18093: Changes to insure that when in app-mode, links open in the default browser. (Closed)

Created:
10 years, 7 months ago by brg
Modified:
8 years, 3 months ago
CC:
levin, chromium-reviews_googlegroups.com
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 4

Patch Set 3 : '' #

Total comments: 12

Patch Set 4 : '' #

Total comments: 14

Patch Set 5 : '' #

Total comments: 8

Patch Set 6 : '' #

Total comments: 1

Patch Set 7 : '' #

Patch Set 8 : '' #

Total comments: 13

Patch Set 9 : '' #

Patch Set 10 : '' #

Patch Set 11 : '' #

Total comments: 4

Patch Set 12 : '' #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+300 lines, -26 lines) Patch
A chrome/browser/app_mode_navigation_uitest.cc View 11 1 chunk +194 lines, -0 lines 0 comments Download
M chrome/browser/browser.h View 5 6 7 8 4 chunks +10 lines, -2 lines 0 comments Download
M chrome/browser/browser.cc View 1 2 3 4 5 6 7 8 6 chunks +19 lines, -5 lines 1 comment Download
M chrome/browser/tab_contents/tab_contents_delegate.h View 3 chunks +8 lines, -6 lines 0 comments Download
M chrome/browser/tab_contents/web_contents.h View 9 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/tab_contents/web_contents.cc View 7 8 10 chunks +36 lines, -13 lines 0 comments Download
A chrome/test/data/appmodenavigation_test.html View 7 8 1 chunk +21 lines, -0 lines 0 comments Download
M chrome/test/ui/ui_tests.vcproj View 7 8 9 10 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 26 (0 generated)
brg
10 years, 7 months ago (2009-01-15 13:43:07 UTC) #1
brg
Some style changes and more restrictive scoping of when to open new links in the ...
10 years, 7 months ago (2009-01-15 21:22:00 UTC) #2
darin (slow to review)
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 ...
10 years, 7 months ago (2009-01-16 05:08:48 UTC) #3
brg
Thank you for the initial comments. Please see my email for the design decision of ...
10 years, 7 months ago (2009-01-16 13:08:23 UTC) #4
Ben Goodger (Google)
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? ...
10 years, 7 months ago (2009-01-16 20:13:44 UTC) #5
brg
The changes in renderview now make it so the urls are not downloaded in the ...
10 years, 7 months ago (2009-01-16 20:51:24 UTC) #6
darin (slow to review)
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 ...
10 years, 7 months ago (2009-01-16 20:58:10 UTC) #7
darin (slow to review)
This CL is also missing tests. There are a number of interesting ways that a ...
10 years, 7 months ago (2009-01-16 20:59:31 UTC) #8
brg
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: ...
10 years, 7 months ago (2009-01-16 21:08:56 UTC) #9
darin (slow to review)
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 ...
10 years, 7 months ago (2009-01-16 21:22:25 UTC) #10
darin (slow to review)
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 ...
10 years, 7 months ago (2009-01-16 21:25:03 UTC) #11
brettw
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 ...
10 years, 7 months ago (2009-01-16 21:28:07 UTC) #12
brg
Thank you for the discussions. I've updated the change to be more similar to the ...
10 years, 7 months ago (2009-01-17 02:31:25 UTC) #13
brg
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 ...
10 years, 7 months ago (2009-01-17 02:57:03 UTC) #14
Ben Goodger (Google)
I want to hear from Darin about the implications of launching URLs the way you ...
10 years, 7 months ago (2009-01-20 22:07:49 UTC) #15
brg
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 ...
10 years, 7 months ago (2009-01-21 00:39:04 UTC) #16
brettw
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 ...
10 years, 7 months ago (2009-01-21 05:46:08 UTC) #17
brg
Updated all comments. Added UItests for app mode navigation. Outside of app mode, it seems ...
10 years, 7 months ago (2009-01-22 10:38:55 UTC) #18
darin (slow to review)
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 ...
10 years, 6 months ago (2009-01-22 17:32:36 UTC) #19
irishceltic1.leo
10 years, 6 months ago (2009-01-22 17:38:22 UTC) #20
brg
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 ...
10 years, 6 months ago (2009-01-22 19:03:01 UTC) #21
brettw
Looks like you forgot to add the uitest file to the CL.
10 years, 6 months ago (2009-01-22 19:19:21 UTC) #22
brg
RE: UITest not in changelist. That was odd, it was in the changelist. I might ...
10 years, 6 months ago (2009-01-22 19:38:59 UTC) #23
brettw
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) ...
10 years, 6 months ago (2009-01-22 20:27:03 UTC) #24
brg
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 ...
10 years, 6 months ago (2009-01-22 23:33:03 UTC) #25
Mark Larson
10 years, 6 months ago (2009-01-23 07:02:32 UTC) #26
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?

Powered by Google App Engine
This is Rietveld 408576698