Links in platform apps should open in the system default browser.

Links in platform apps should open in the system default browser.

BUG=145646
TEST=manually


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

[Junmin Zhu, 2012-09-01 11:48:26]

Please review the patch. Thanks.

[jeremya, 2012-09-03 02:42:28]

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

http://codereview.chromium.org/10915047/diff/3001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:232: CFRelease(url);
It's kind of icky to have all these #ifdefs here. Perhaps move them into a
method on ShellIntegration? Something like
ShellIntegration::OpenURLInSystemBrowser()

http://codereview.chromium.org/10915047/diff/3001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:276: source->SetDelegate(NULL);
Rather than setting the delegate to the ShellWindow (which might get deleted
between AddNewContents and OpenURLFromTab!), perhaps it would be better to make
a new kind of WebContentsDelegate object that just listens for the
OpenURLFromTab message and then deletes itself (and the related WebContents).

It would be nice if we never had to create a new WebContents for this
navigation. From a little reading, I think that would require modifying the
dispatchDecidePolicyForNewWindowAction code in
third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp, so
perhaps that refactor could be done in a separate patch.

darin@ is probably a good person to comment on this, since he probably wrote
that code (though it's hard to tell through the mists of time).

[Marijn Kruisselbrink, 2012-09-04 15:40:53]

Maybe you should assign the bug to yourself, since you're working on it? (and
next time maybe do so before you start working on it to avoid duplicate work)

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

http://codereview.chromium.org/10915047/diff/3001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:19: #endif // OS_MACOSX
I think the chromium coding style says platform-specific #includes should come
after "regular" #includes
(http://www.chromium.org/developers/coding-style#TOC-Platform-defines)

[Mihai Parparita -not on Chrome, 2012-09-04 21:39:53]

http://codereview.chromium.org/10915047/diff/3001/chrome/browser/shell_integr...
File chrome/browser/shell_integration_linux.cc (right):

http://codereview.chromium.org/10915047/diff/3001/chrome/browser/shell_integr...
chrome/browser/shell_integration_linux.cc:691: void LaunchDefaultBrowser(const
GURL& url) {
This is already implemented (for all platforms) by platform_util::OpenExternal

[Junmin Zhu, 2012-09-05 07:38:26]

On 2012/09/04 15:40:53, Marijn Kruisselbrink wrote:
> Maybe you should assign the bug to yourself, since you're working on it? (and
> next time maybe do so before you start working on it to avoid duplicate work)

Sorry for this, and change the bug's owner to myself.
Thanks for you remind.

[Junmin Zhu, 2012-09-05 07:47:18]

> chrome/browser/ui/extensions/shell_window.cc:232: CFRelease(url);
> It's kind of icky to have all these #ifdefs here. Perhaps move them into a
> method on ShellIntegration? Something like
> ShellIntegration::OpenURLInSystemBrowser()

Done. According Mihaip's advice, leverage platform_util::OpenExternal here.
Thanks Mihaip.:)

> 
http://codereview.chromium.org/10915047/diff/3001/chrome/browser/ui/extension...
> chrome/browser/ui/extensions/shell_window.cc:276: source->SetDelegate(NULL);
> Rather than setting the delegate to the ShellWindow (which might get deleted
> between AddNewContents and OpenURLFromTab!), perhaps it would be better to
make
> a new kind of WebContentsDelegate object that just listens for the
> OpenURLFromTab message and then deletes itself (and the related WebContents).

Done, I found web_content is shared when navigate the same link. So here NOT
directly delete WebContentsDelegate object after the usage, but delete when
Shell Window was destructed in case user want to navigate link afterward.

> 
> It would be nice if we never had to create a new WebContents for this
> navigation. From a little reading, I think that would require modifying the
> dispatchDecidePolicyForNewWindowAction code in
> third_party/WebKit/Source/WebKit/chromium/src/FrameLoaderClientImpl.cpp, so
> perhaps that refactor could be done in a separate patch.

I would dig into this. Thanks for your comment.:)

[Mihai Parparita -not on Chrome, 2012-09-05 18:15:10]

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

http://codereview.chromium.org/10915047/diff/8001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:298: #if defined(OS_WIN) ||
defined(OS_MACOSX) || defined(OS_POSIX)
What platform are you looking to avoid here? Chrome OS? I believe OS_POSIX is
true there. I think OS_LINUX is more appropriate.

http://codereview.chromium.org/10915047/diff/8001/chrome/browser/ui/extension...
chrome/browser/ui/extensions/shell_window.cc:300:
external_content_delegate_.reset(new ExternalWebContentImpl());
Seems like ExternalWebContentImpl could be a singleton.

Also, it's not actually a WebContents (or an implementation of one).
ExternalWebContentsDelegate or ExternalUrlWebContentsDelegate or
ExternalUrlController may be better names.

[Junmin Zhu, 2012-09-06 03:11:19]

Please review again, thanks.

> chrome/browser/ui/extensions/shell_window.cc:298: #if defined(OS_WIN) ||
> defined(OS_MACOSX) || defined(OS_POSIX)
> What platform are you looking to avoid here? Chrome OS? I believe OS_POSIX is
> true there. I think OS_LINUX is more appropriate.
> 

I want to avoid the platform that haven't implemented OpenExternal, such as
Android. You are right, OS_LINUX is more appropriate.

> chrome/browser/ui/extensions/shell_window.cc:300:
> external_content_delegate_.reset(new ExternalWebContentImpl());
> Seems like ExternalWebContentImpl could be a singleton.
> 
> Also, it's not actually a WebContents (or an implementation of one).
> ExternalWebContentsDelegate or ExternalUrlWebContentsDelegate or
> ExternalUrlController may be better names.

Done. change to ExternalUrlController as a singleton.

[Mihai Parparita -not on Chrome, 2012-09-06 04:17:02]

LGTM

[commit-bot: I haz the power, 2012-09-06 04:20:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junmin.zhu@intel.com/10915047/5

[commit-bot: I haz the power, 2012-09-06 05:49:23]

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

[commit-bot: I haz the power, 2012-09-06 06:08:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junmin.zhu@intel.com/10915047/5

[commit-bot: I haz the power, 2012-09-06 07:49:25]

Try job failure for 10915047-5 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-06 09:58:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junmin.zhu@intel.com/10915047/5

[commit-bot: I haz the power, 2012-09-06 11:40:35]

Try job failure for 10915047-5 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Mihai Parparita -not on Chrome, 2012-09-06 14:47:27]

This isn't test flakiness, PlatformAppBrowserTest.OpenLink needs to be
updated:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

Since you're potentially opening the links in a wholly different browser, I
don't think you can write a reliable test for that. You may want to modify
ExternalUrlController to allow platform_util::OpenExternal to be swapped
with a different function, and in the test replace it with a stub that you
can assert gets called.

Mihai

On Thu, Sep 6, 2012 at 4:40 AM, <commit-bot@chromium.org> wrote:

> Try job failure for 10915047-5 (retry) on mac_rel for step "browser_tests".
> It's a second try, previously, step "browser_tests" failed.
> http://build.chromium.org/p/**tryserver.chromium/**
>
buildstatus?builder=mac_rel&**number=53502<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=53502>
>
>
>
https://chromiumcodereview.**appspot.com/10915047/<https://chromiumcodereview...
>

[Junmin Zhu, 2012-09-21 02:14:48]

Hi Mihaip,

  I add a different function for swapping OpenExternal in test scenario, and add
potential leak issue fix. 

  Please help to review it again, thanks.

[Mihai Parparita -not on Chrome, 2012-09-29 00:28:44]

http://codereview.chromium.org/10915047/diff/14001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/10915047/diff/14001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:107: if
(command_line->HasSwitch("OpenLinkTest")) {
Test behavior is normally not overridden via command line flags (the test run in
the same process, so there's no need to pass in flags).

The usual behavior is to have a static function that lets you override the
behavior, along the lines of:

http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions/...

In this case, you could have a ShellWindow::SetExternalUrlControllerForTesting
that lets you override the default delegate that you use. Then in your test, you
can pass in an implementation that opens a new tab (or not even that, it could
just store the URL, and then the test can check for it).

This also avoids having too much test-specific code in the implementation .cc
file.

[Junmin Zhu, 2012-10-06 11:54:29]

Thanks for your comment.
Refine the patch, please kindly review it again.

[Mihai Parparita -not on Chrome, 2012-10-12 23:00:17]

LGTM

http://codereview.chromium.org/10915047/diff/22001/chrome/browser/extensions/...
File chrome/browser/extensions/platform_app_browsertest_util.h (right):

http://codereview.chromium.org/10915047/diff/22001/chrome/browser/extensions/...
chrome/browser/extensions/platform_app_browsertest_util.h:25: class
MockExternalUrlController : public content::WebContentsDelegate {
This doesn't need to be defined in the header, it can just be in the .cc file
(you can just have a forward declaration here).

http://codereview.chromium.org/10915047/diff/22001/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/shell_window.cc (right):

http://codereview.chromium.org/10915047/diff/22001/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/shell_window.cc:59: static
content::WebContentsDelegate* controller_for_test_ = NULL;
Call this url_controller_for_test_.

[commit-bot: I haz the power, 2012-10-13 09:13:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junmin.zhu@intel.com/10915047/29001

[commit-bot: I haz the power, 2012-10-13 09:57:36]

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-13 14:21:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junmin.zhu@intel.com/10915047/26002

[commit-bot: I haz the power, 2012-10-13 15:01:10]

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-14 15:52:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/junmin.zhu@intel.com/10915047/38007

[commit-bot: I haz the power, 2012-10-14 20:19:33]

Change committed as 161812

[kinuko, 2012-10-15 06:27:10]

This one seems to be causing ASAN use-after-free errors in
PlatformAppBrowserTest.OpenLink test.  It might be a test only issue (since it's
happening in a mock class) but may I revert this change?

[kinuko, 2012-10-15 06:32:09]

The bot failure link is: 
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASAN%20Tests%20%...

==27274== ERROR: AddressSanitizer heap-use-after-free on address 0x7f6f7e227a68
at pc 0x47ca442 bp 0x7fff825be770 sp 0x7fff825be768
READ of size 8 at 0x7f6f7e227a68 thread T0
#0 0x47ca441 in WebContentsImpl::OpenURL(content::OpenURLParams const&) ???:0
#1 0x47d6a88 in WebContentsImpl::RequestTransferURL(GURL const&,
content::Referrer const&, WindowOpenDisposition, long, content::GlobalRequestID
const&) ???:0
#2 0x47d674b in non-virtual thunk to
WebContentsImpl::RequestOpenURL(content::RenderViewHost*, GURL const&,
content::Referrer const&, WindowOpenDisposition, long) ???:0
#3 0x46dd909 in content::RenderViewHostImpl::OnMsgOpenURL(GURL const&,
content::Referrer const&, WindowOpenDisposition, long) ???:0
#4 0x46dd71e in bool ViewHostMsg_OpenURL::Dispatch<content::RenderViewHostImpl,
content::RenderViewHostImpl, void (content::RenderViewHostImpl::*)(GURL const&,
content::Referrer const&, WindowOpenDisposition, long)>(IPC::Message const*,
content::RenderViewHostImpl*, content::RenderViewHostImpl*, void
(content::RenderViewHostImpl::*)(GURL const&, content::Referrer const&,
WindowOpenDisposition, long)) ???:0
#5 0x46d98b9 in content::RenderViewHostImpl::OnMessageReceived(IPC::Message
const&) ???:0
#6 0x46e004c in non-virtual thunk to
content::RenderViewHostImpl::OnMessageReceived(IPC::Message const&) ???:0
#7 0x46b6253 in content::RenderProcessHostImpl::OnMessageReceived(IPC::Message
const&) ???:0
#8 0x46b700c in non-virtual thunk to
content::RenderProcessHostImpl::OnMessageReceived(IPC::Message const&) ???:0
#9 0x2be1575 in IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message
const&) ???:0
#10 0x36e63ec in MessageLoop::RunTask(base::PendingTask const&) ???:0
#11 0x36e69df in MessageLoop::DeferOrRunPendingTask(base::PendingTask const&)
???:0
#12 0x36e778b in MessageLoop::DoWork() ???:0
#13 0x37b19c8 in (anonymous namespace)::WorkSourceDispatch(_GSource*, int
(*)(void*), void*) ../../base/message_pump_glib.cc:0
#14 0x7f6f977488c1 in g_main_dispatch
/build/buildd/glib2.0-2.24.1/glib/gmain.c:1960
0x7f6f7e227a68 is located 488 bytes inside of 1144-byte region
[0x7f6f7e227880,0x7f6f7e227cf8)
freed by thread T0 here:
#0 0xafff850 in operator delete(void*) ??:0
#1 0xd90082 in
extensions::MockExternalUrlController::OpenURLFromTab(content::WebContents*,
content::OpenURLParams const&) ???:0
#2 0x47ca2b4 in WebContentsImpl::OpenURL(content::OpenURLParams const&) ???:0
#3 0x47d6a88 in WebContentsImpl::RequestTransferURL(GURL const&,
content::Referrer const&, WindowOpenDisposition, long, content::GlobalRequestID
const&) ???:0
#4 0x47d674b in non-virtual thunk to
WebContentsImpl::RequestOpenURL(content::RenderViewHost*, GURL const&,
content::Referrer const&, WindowOpenDisposition, long) ???:0
#5 0x46dd909 in content::RenderViewHostImpl::OnMsgOpenURL(GURL const&,
content::Referrer const&, WindowOpenDisposition, long) ???:0
#6 0x46dd71e in bool ViewHostMsg_OpenURL::Dispatch<content::RenderViewHostImpl,
content::RenderViewHostImpl, void (content::RenderViewHostImpl::*)(GURL const&,
content::Referrer const&, WindowOpenDisposition, long)>(IPC::Message const*,
content::RenderViewHostImpl*, content::RenderViewHostImpl*, void
(content::RenderViewHostImpl::*)(GURL const&, content::Referrer const&,
WindowOpenDisposition, long)) ???:0
#7 0x46d98b9 in content::RenderViewHostImpl::OnMessageReceived(IPC::Message
const&) ???:0
#8 0x46e004c in non-virtual thunk to
content::RenderViewHostImpl::OnMessageReceived(IPC::Message const&) ???:0
#9 0x46b6253 in content::RenderProcessHostImpl::OnMessageReceived(IPC::Message
const&) ???:0
#10 0x46b700c in non-virtual thunk to
content::RenderProcessHostImpl::OnMessageReceived(IPC::Message const&) ???:0
#11 0x2be1575 in IPC::ChannelProxy::Context::OnDispatchMessage(IPC::Message
const&) ???:0
#12 0x36e63ec in MessageLoop::RunTask(base::PendingTask const&) ???:0
#13 0x36e69df in MessageLoop::DeferOrRunPendingTask(base::PendingTask const&)
???:0
#14 0x36e778b in MessageLoop::DoWork() ???:0
#15 0x37b19c8 in (anonymous namespace)::WorkSourceDispatch(_GSource*, int
(*)(void*), void*) ../../base/message_pump_glib.cc:0
#16 0x7f6f977488c1 in g_main_dispatch
/build/buildd/glib2.0-2.24.1/glib/gmain.c:1960
previously allocated by thread T0 here:
#0 0xafff6d0 in operator new(unsigned long) ??:0
#1 0x47c7ca9 in WebContentsImpl::CreateNewWindow(int,
ViewHostMsg_CreateWindow_Params const&, content::SessionStorageNamespace*) ???:0
#2 0x36e63ec in MessageLoop::RunTask(base::PendingTask const&) ???:0
#3 0x36e69df in MessageLoop::DeferOrRunPendingTask(base::PendingTask const&)
???:0
#4 0x36e778b in MessageLoop::DoWork() ???:0
#5 0x37b19c8 in (anonymous namespace)::WorkSourceDispatch(_GSource*, int
(*)(void*), void*) ../../base/message_pump_glib.cc:0
#6 0x7f6f977488c1 in g_main_dispatch
/build/buildd/glib2.0-2.24.1/glib/gmain.c:1960