Open external application dialog should not show for Chrome.

Open external application dialog should not show for Chrome.

This change prevents the external application dialog from coming up if Chrome
detects that it would just start itself. To determine this it uses the
shell_integration functionality added for registerProtocolHandler to find out
if Chrome is the default handler.

BUG=90373
TEST=Unit tests added.


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

[benwells, 2011-08-30 08:08:25]

WIP: this still needs a unit test added. This will take a little effort as there
doesn't seem to be any unit testing for this area.

Any feedback in the meantime is welcome.

[tony, 2011-08-30 17:51:26]

http://codereview.chromium.org/7790021/diff/1/chrome/browser/external_protoco...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

http://codereview.chromium.org/7790021/diff/1/chrome/browser/external_protoco...
chrome/browser/external_protocol/external_protocol_handler.cc:129: namespace {
Nit: I would move this to the top of the file (above the other functions in the
file).

http://codereview.chromium.org/7790021/diff/1/chrome/browser/external_protoco...
chrome/browser/external_protocol/external_protocol_handler.cc:148: 
Can we DCHECK that we're on the right thread here?

http://codereview.chromium.org/7790021/diff/1/chrome/browser/external_protoco...
chrome/browser/external_protocol/external_protocol_handler.cc:201: observer =
new ExternalDefaultProtocolObserver(url, render_process_host_id,
Nit: Maybe merge this with the previous line.

http://codereview.chromium.org/7790021/diff/1/chrome/browser/external_protoco...
chrome/browser/external_protocol/external_protocol_handler.cc:204: worker = new
ShellIntegration::DefaultProtocolClientWorker(observer,
Nit: Ditto

[benwells, 2011-09-22 06:59:38]

Sorry about the big delay with this, I got sidetracked on other projects.

I've added unit testing for the changes I've made.

[tony, 2011-09-22 17:34:38]

LGTM, just some style nits.

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
chrome/browser/external_protocol/external_protocol_handler.cc:35: if (delegate
== NULL)
Nit: !delegate and below.

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
File chrome/browser/external_protocol/external_protocol_handler.h (right):

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
chrome/browser/external_protocol/external_protocol_handler.h:41: virtual
~Delegate() {}
Does clang complain about this destructor?  I thought we had to put all
implementations in .cc files.

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
File chrome/browser/external_protocol/external_protocol_handler_unittest.cc
(right):

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
chrome/browser/external_protocol/external_protocol_handler_unittest.cc:13:
FakeExternalProtocolHandlerWorker(
This looks indented too much.

[benwells, 2011-09-23 00:29:51]

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
File chrome/browser/external_protocol/external_protocol_handler.cc (right):

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
chrome/browser/external_protocol/external_protocol_handler.cc:35: if (delegate
== NULL)
On 2011/09/22 17:34:38, tony wrote:
> Nit: !delegate and below.

Done.

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
File chrome/browser/external_protocol/external_protocol_handler.h (right):

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
chrome/browser/external_protocol/external_protocol_handler.h:41: virtual
~Delegate() {}
On 2011/09/22 17:34:38, tony wrote:
> Does clang complain about this destructor?  I thought we had to put all
> implementations in .cc files.

try success on linux_clang and mac trybots, so it looks like this is OK

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
File chrome/browser/external_protocol/external_protocol_handler_unittest.cc
(right):

http://codereview.chromium.org/7790021/diff/3001/chrome/browser/external_prot...
chrome/browser/external_protocol/external_protocol_handler_unittest.cc:13:
FakeExternalProtocolHandlerWorker(
On 2011/09/22 17:34:38, tony wrote:
> This looks indented too much.

Done.

[commit-bot: I haz the power, 2011-09-23 03:42:30]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/benwells@chromium.org/7790021/10001

[commit-bot: I haz the power, 2011-09-23 05:04:40]

Change committed as 102449